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

dev/core#2730 - Replace fopen call in CRM_Utils_File::isIncludable with one that doesn't need error-supression to avoid problems in php8 #21060

Merged
merged 1 commit into from
Aug 9, 2021

Conversation

demeritcowboy
Copy link
Contributor

@demeritcowboy demeritcowboy commented Aug 7, 2021

Overview

https://lab.civicrm.org/dev/core/-/issues/2730

Before

One (extreme) way to see the before situation is #21064. Lots of stuff like:

Failure in api call for UFGroup create: fopen(api/v3/UFGroup/Getfields.php): failed to open stream: No such file or directory'

While this is an E_WARNING and doesn't change anything in normal usage in php 8, what does change in php 8 is when you have an error handler that takes the incoming $errno and acts when it has a value of E_WARNING. In php 7, errno is always set to 0 when the error_supression (@) operator is used. In php 8 it passes the actual value in.
(There are other changes, like it also changes the value of error_reporting(), but that doesn't apply to this specifically).

After

Quiet

Technical Details

This should be an equivalent call but at the time the function was written it's possible not all sites had a high enough php version. There's more in the lab ticket if interested.

Comments

Has test, which passed before on the current code so is at least a partial check that there's some equivalence: https://test.civicrm.org/job/CiviCRM-Core-PR/42883/

@civibot
Copy link

civibot bot commented Aug 7, 2021

(Standard links)

@civibot civibot bot added the master label Aug 7, 2021
@demeritcowboy demeritcowboy changed the title [WIP] dev/core#2730 - Unit test for isIncludable dev/core#2730 - Replace fopen call in CRM_Utils_File::isIncludable with one that doesn't need error-supression to avoid problems in php8 Aug 8, 2021
@seamuslee001
Copy link
Contributor

This seems fine to me MOP

@demeritcowboy
Copy link
Contributor Author

Hmm. The fail is the part of my test I couldn't test on windows. So I need to check is_readable or something separately if it resolves.

@eileenmcnaughton eileenmcnaughton merged commit c5d9fe3 into civicrm:master Aug 9, 2021
@demeritcowboy demeritcowboy deleted the test-includable branch August 9, 2021 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants