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

Fix error suppression for PHP 8+ #16615

Merged
merged 2 commits into from
Sep 18, 2024

Conversation

halftrainedharry
Copy link
Contributor

What does it do?

Adds a check to the custom error handler function, to see if a warning has to be handled.

Why is it needed?

MODX has a custom error handler function. When the error control operator (@) is used (e.g. @exif_read_data()), the custom error handler still gets called.

With PHP 7.4, when the error was suppressed by the @ operator, error_reporting() called inside the custom error handler always returned 0. This gets checked here:

if (error_reporting() == 0) {
return false;
}

With PHP 8, this is no longer the case:

As of PHP 8.0.0, it returns the value of this (bitwise) expression: E_ERROR | E_CORE_ERROR | E_COMPILE_ERROR | E_USER_ERROR | E_RECOVERABLE_ERROR | E_PARSE.

https://www.php.net/manual/en/language.operators.errorcontrol.php

How to test

For example:

  • Use PHP 8.0 (or higher)
  • Open the media browser and navigate to a folder with a picture that doesn't have EXIF headers.
  • Make sure, that the warning PHP warning: exif_read_data(...): File not supported doesn't get logged to the MODX error log anymore.

Related issue(s)/PR(s)

#16420

@modxcommunity
Copy link
Collaborator

This pull request has been mentioned on MODX Community. There might be relevant details there:

https://community.modx.com/t/exif-read-data-file-not-supported/6572/17

@opengeek
Copy link
Member

Excellent contribution, @halftrainedharry! I'll get this reviewed and integrated as soon as possible. I'm guessing this would also be helpful as a bug fix in 2.x.

@opengeek
Copy link
Member

@halftrainedharry — Is this suppressing all E_WARNING errors, even if the @ operator is not used?

@halftrainedharry
Copy link
Contributor Author

Is this suppressing all E_WARNING errors, even if the @ operator is not used?

No.

Here is a test snippet:

<?php
include MODX_CORE_PATH . "FileThatDoesntExist_01.php";
@include MODX_CORE_PATH . "FileThatDoesntExist_02.php";

include without the @ in the first line, still adds a warning to the MODX error log.


With error_reporting = E_ALL in the "php.ini", when include is used for a file that doesn't exist, the error handler function is called and error_reporting() in the handler function returns 32767 (= E_ALL).
When @include is used, error_reporting() in the handler function returns 4437 (= E_ERROR | E_CORE_ERROR | E_COMPILE_ERROR | E_USER_ERROR | E_RECOVERABLE_ERROR | E_PARSE).

@rthrash rthrash added this to the v3.1.0 milestone Sep 18, 2024
@opengeek opengeek added bug The issue in the code or project, which should be addressed. area-core pr/port-to-2 Pull request has to be ported to 2.x. labels Sep 18, 2024
@opengeek opengeek changed the title [3.x] Fix MODX error handler when the error control operator (@) is used in PHP 8 Fix error suppression for PHP 8+ Sep 18, 2024
@opengeek opengeek merged commit 3c0acde into modxcms:3.x Sep 18, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core bug The issue in the code or project, which should be addressed. pr/port-to-2 Pull request has to be ported to 2.x.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants