-
Notifications
You must be signed in to change notification settings - Fork 22
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
Remove usage of $php_errormsg #42
Remove usage of $php_errormsg #42
Conversation
Version info: (PHP 5 >= 5.2.0, PHP 7) |
typo in commit message: |
@Megatherium there is some side effect on this, have you considered this in the changes you made? there is error_clear_last(), but that's php7, but there's polyfill for that: we would not need that error clearing method if the "use error_get_last() only on errors" principle is followed. |
@Megatherium there are some totally new tests added with this PR. where they originate from? perhaps submit them as separate PR and leave credits/info? |
@Megatherium also, please rebase your branch against current |
No, I was unaware of this distinction. I will look into it.
I added them. I assume to test my changes but it's been a while...
Will try. |
Ah okay, It's okay then |
The track_errors directive as well as the $php_errormsg global variable have been completely removed in PHP8.0. To make the framework compatible with 8.0 while keeping the original behaviour the message is now obtained by basically accessing get_last_error()['message']. Tests where added, where possible, to ensure that the throwing behaviour has not changed.
As this branch does not contain the major fixes for PHP8 ( cf. zf1s#32) the tests will abort with a fatal. Thus Travis has beeb allowed to fail for PHP8.
@Megatherium @falkenhawk please add "closes #35" to PR body. I don't have administrative privileges to do so myself. |
4e8000d
to
9c45fae
Compare
@Megatherium you need to add "symfony/polyfill-php70" to each subpackage |
9c45fae
to
359b9b8
Compare
Done. Also: 5.4 and 5.5 finish flawlessly over on Travis. Github seems to have mangled their images. |
Reported about ext-ldap again: |
@Megatherium so here's an example that I had in mind. this code block does not need error_clear_last(), as the error_get_last() is called only if the error happens. error_clear_last();
$feed = @file_get_contents($filename);
if ($feed === false) {
/**
* @see Zend_Feed_Exception
*/
$err = error_get_last();
$phpErrormsg = isset($err['message'][0]) ? $err['message'] : null;
throw new Zend_Feed_Exception("File could not be loaded: $phpErrormsg");
}
return self::importString($feed); But as the new dependency was introduced, I guess it's best to just call it anyay, to be sure the code doesn't break on future code changes. |
@Megatherium can you re-run the CI, as it seems to be a temporary issue: |
Simply replacing any usage of $php_errormsg (as this has been removed in PHP8.0) with error_get_last() could potentially lead to fetching an error that has nothing to do with the code we actually want to inspect. Thus error_clear_last() is called before executing the lines prone to cause an error. As error_clear_last() is PHP7+ Symfony's polyfill was added as a dependency for backward compatbility. The return value of error_get_last() is checked in accordance with the recommendation of the polyfill.
359b9b8
to
6a7cd80
Compare
FYI The last commit just contained an extra blank like to trigger the CI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for your ongoing commitment @Megatherium and @glensc!
oh. perhaps I should have waited for #32 to be merged first 🙈 |
|
True but not for me on this repo. Probably a rights thing.
Good to know. |
You need to go to your fork Actions tab. jobs from there are retry-able. |
The track_errors directive and the global $php_errormsg variable have been removed in PHP8. The same functionality can be achieved via error_get_last() thereby making the framework compatible with PHP8 in this respect.
closes #35