Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throws a validation exception now if attempting to cast an object to string #42

Merged
merged 6 commits into from
Sep 12, 2020

Conversation

ronnied
Copy link

@ronnied ronnied commented Aug 24, 2020

Overview

PHP 7.4 was failing unit tests (PHPUnit v4.8.36).

Please preserve this line to notify @courtney-miles (lead of this repository)

@courtney-miles
Copy link
Collaborator

I can see this is due to a undocumented BC breakage with PHP 7.4 where previously it would throw an instance of \Exception and now it will throw an instance of \Error.

I believe that the correct solution is that it should expect to catch \Throwable rather than \Exception. However, this would break compatibility with PHP 5.

Let me see if I can get acceptance for dropping support for PHP 5.

Copy link
Collaborator

@courtney-miles courtney-miles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @ronnied ,

Where this PR is for compatibility with PHP 7.4, can you also modify .travis.yml to add PHP 7.4 to be tested, and modify composer.json to remove the upper cap on PHP versions? (I.e. change the version constraint from >=7.1.0 <7.4.0 to >=7.1.0.)

Comment on lines 28 to 33
try {
if (is_object($val)) {
throw $this->getValidationException('Could not cast object to string', $val);
}
$val = (string) $val;
} catch (\Exception $e) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest set of changes in frictionlessdata:master drop support for PHP 5.6.

With support for 5.6 dropped, I believe a better correction for PHP 7.4 will the following.

Suggested change
try {
if (is_object($val)) {
throw $this->getValidationException('Could not cast object to string', $val);
}
$val = (string) $val;
} catch (\Exception $e) {
try {
$val = (string) $val;
} catch (\Throwable $e) {

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@courtney-miles courtney-miles mentioned this pull request Sep 1, 2020
7 tasks
@courtney-miles
Copy link
Collaborator

Hey @ronnied ,

If you can allow me to contribute to this PR (and #43 ) I am happy to make the final corrections to get them to pass.

Thanks

@courtney-miles courtney-miles merged commit 23eb7d4 into frictionlessdata:master Sep 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants