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

Remove usage of $php_errormsg #42

Merged

Conversation

Megatherium
Copy link

@Megatherium Megatherium commented Dec 31, 2020

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

@glensc
Copy link
Contributor

glensc commented Dec 31, 2020

Version info:

(PHP 5 >= 5.2.0, PHP 7)

@glensc
Copy link
Contributor

glensc commented Dec 31, 2020

typo in commit message: beeb

@glensc
Copy link
Contributor

glensc commented Dec 31, 2020

@Megatherium there is some side effect on this, $php_errormsg is local to function/method, but error_get_last() is global, so if previous code was able to check $php_errormsg if error has happened, then in new code, error_get_last() must be called only if it is certain that error has occoured.

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.

@glensc
Copy link
Contributor

glensc commented Dec 31, 2020

@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?

@glensc
Copy link
Contributor

glensc commented Dec 31, 2020

@Megatherium also, please rebase your branch against current master. this is to ensure CI stays green even after the merge.

@Megatherium
Copy link
Author

have you considered this in the changes you made?

No, I was unaware of this distinction. I will look into it.

there are some totally new tests added with this PR. where they originate from?

I added them. I assume to test my changes but it's been a while...

please rebase your branch against current master. this is to ensure CI stays green even after the merge.

Will try.

@glensc
Copy link
Contributor

glensc commented Jan 28, 2021

there are some totally new tests added with this PR. where they originate from?

I added them. I assume to test my changes but it's been a while...

Ah okay, It's okay then

Alexander Wozniak added 2 commits January 29, 2021 11:06
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.
@glensc glensc mentioned this pull request Feb 3, 2021
32 tasks
@glensc
Copy link
Contributor

glensc commented Feb 3, 2021

@Megatherium @falkenhawk please add "closes #35" to PR body. I don't have administrative privileges to do so myself.

@falkenhawk falkenhawk linked an issue Feb 3, 2021 that may be closed by this pull request
@Megatherium Megatherium force-pushed the 35_track_errors_has_been_removed branch 2 times, most recently from 4e8000d to 9c45fae Compare February 4, 2021 12:15
@glensc
Copy link
Contributor

glensc commented Feb 4, 2021

@Megatherium you need to add "symfony/polyfill-php70" to each subpackage composer.json, where you used error_clear_last().

@Megatherium Megatherium force-pushed the 35_track_errors_has_been_removed branch from 9c45fae to 359b9b8 Compare February 4, 2021 14:09
@Megatherium
Copy link
Author

Megatherium commented Feb 4, 2021

@Megatherium you need to add "symfony/polyfill-php70" to each subpackage composer.json, where you used error_clear_last().

Done.

Also: 5.4 and 5.5 finish flawlessly over on Travis. Github seems to have mangled their images.

@glensc
Copy link
Contributor

glensc commented Feb 4, 2021

Reported about ext-ldap again:

@glensc
Copy link
Contributor

glensc commented Feb 4, 2021

@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.

@glensc
Copy link
Contributor

glensc commented Feb 4, 2021

@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.
@Megatherium Megatherium force-pushed the 35_track_errors_has_been_removed branch from 359b9b8 to 6a7cd80 Compare February 5, 2021 08:33
@Megatherium
Copy link
Author

FYI The last commit just contained an extra blank like to trigger the CI.

@falkenhawk falkenhawk self-requested a review February 5, 2021 09:02
Copy link
Member

@falkenhawk falkenhawk left a 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!

@falkenhawk falkenhawk merged commit 38ecfea into zf1s:master Feb 5, 2021
@falkenhawk
Copy link
Member

falkenhawk commented Feb 5, 2021

oh. perhaps I should have waited for #32 to be merged first 🙈

@glensc
Copy link
Contributor

glensc commented Feb 5, 2021

FYI The last commit just contained an extra blank like to trigger the CI.

  1. you don't need to commit. there's re-run all jobs in actions web ui
  2. you can also git commit --amend && git push --force-with-lease, which is sufficient if the commit date changes to make a new commit hash.

@glensc
Copy link
Contributor

glensc commented Feb 5, 2021

oh. perhaps I should have waited for #32 to be merged first 🙈

these are standalone changesets. The problem with #32 is that it's a too big bite to take a time, and is still not reviewed/merged.

I thought to extract changes from there into separate PR if I have time, but apparently, I don't.

@Megatherium
Copy link
Author

FYI The last commit just contained an extra blank like to trigger the CI.

1. you don't need to commit. there's re-run all jobs in actions web ui

True but not for me on this repo. Probably a rights thing.

2. you can also `git commit --amend && git push --force-with-lease`, which is sufficient if the commit date changes to make a new commit hash.

Good to know.

@glensc
Copy link
Contributor

glensc commented Feb 5, 2021

  1. you don't need to commit. there's re-run all jobs in actions web ui

True but not for me on this repo. Probably a rights thing.

You need to go to your fork Actions tab. jobs from there are retry-able.

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.

php 8: The track_errors ini directive has been removed
3 participants