PHP 8.4 | Fix passing E_USER_ERROR to trigger_error() x 3 by jrfnl · Pull Request #881 · simplepie/simplepie

jrfnl · 2024-09-12T06:03:13Z

Notes:

Each change is in a separate commit as each has its own reasoning and decision point.
For the two calls changed to exceptions, it could be considered to introduce SimplePie native dedicated exceptions. This is a design choice for the maintainers of SimplePie. This PR simply fixes the PHP 8.4 compatibility issue.

PHP 8.4 | Fix passing E_USER_ERROR to trigger_error() [1]

PHP 8.4 deprecates passing E_USER_ERROR to trigger_error(), with the recommendation being to replace these type of calls with either an Exception or an exit statement.

This commit fixes an instance found in the autoloader by changing it to a call to exit, which considering it is an error related to the autoloader not being registered seems the most appropriate.

Ref: https://wiki.php.net/rfc/deprecations_php_8_4#deprecate_passing_e_user_error_to_trigger_error

PHP 8.4 | Fix passing E_USER_ERROR to trigger_error() [2]

PHP 8.4 deprecates passing E_USER_ERROR to trigger_error(), with the recommendation being to replace these type of calls with either an Exception or an exit statement.

This commit fixes another instance of this issue. In this case, the call to trigger_error() is replaced by throwing a generic SimplePie\Exception for the same.

Ref: https://wiki.php.net/rfc/deprecations_php_8_4#deprecate_passing_e_user_error_to_trigger_error

PHP 8.4 | Fix passing E_USER_ERROR to trigger_error() [3]

PHP 8.4 deprecates passing E_USER_ERROR to trigger_error(), with the recommendation being to replace these type of calls with either an Exception or an exit statement.

This commit fixes the last instance of this issue. In this case, the call to trigger_error() is also replaced by throwing a generic SimplePie\Exception for the same.

Ref: https://wiki.php.net/rfc/deprecations_php_8_4#deprecate_passing_e_user_error_to_trigger_error

QA: replace another call to trigger_error() with exit

... the call to trigger_error() would now throw a PHP notice, but with the die() after it, with still kill the program anyhow, so we may as well, exit with the error message and remove the trigger_error() function call completely.

jrfnl · 2024-09-12T06:04:08Z

Note: build failure is unrelated to this PR and should be fixed via #878

jtojnar · 2024-09-12T07:35:38Z

src/Gzdecode.php

+     *
+     * @throws \Exception
     */
    public function __set(string $name, $value)


This looks to me like poor man’s https://wiki.php.net/rfc/deprecate_dynamic_properties.

We could probably just drop it altogether as modern PHP versions will complain out of the box, and it can be trivially caught by PHPStan.

I'd be fine with that, but kept my changes to the minimum needed to fix the PHP 8.4 deprecations. Removing the method is a change which feels to me like something which should be decided by maintainer, not by a drive-by contributor like me.

On second thought, in this case, it's better to keep the method as it is a more stringent safeguard against dynamic properties than relying on users of this package to be running static analysis tools.

On second thought, in this case, it's better to keep the method as it is a more stringent safeguard against dynamic properties than relying on users of this package to be running static analysis tools.

I doubt anyone sets any properties on this, they are mostly internal state only read by the parse method and initialized at the start.

We could just as well mark them private since they are already @access private but that’s also for another PR.

Indeed, but that still won't protect against dynamic properties (even making a class final won't).
Would be interesting to see what the reason was the method got added. I imagine it could be something to do with security and/or serialization ?

The class is just a pure PHP implementation of gzdecode function (which was only introduced with PHP 5.4). The class is used to encapsulate the state of the stateful gzip decompression algorithm.

My best guess is Sam got bitten by a typo so she added it to avoid confusing debugging in the future 0aa49e5

We should probably deprecate it in favour of gzdecode too.

Sounds like a good idea.

jtojnar · 2024-09-12T07:36:49Z

src/SimplePie.php

        $file = $trace[0]['file'];
        $line = $trace[0]['line'];
-        trigger_error("Call to undefined method $class::$method() in $file on line $line", E_USER_ERROR);
+        throw new Exception("Call to undefined method $class::$method() in $file on line $line");


Looks reasonable.

@throws probably unnecessary, as exceptional case should be immediately statically obvious – though I wonder how does it interact with PHPStan.

This might need to be SimplePieException? Or does the local Exception class shadow \Exception?

Or maybe it should even be an Error, as the purpose is just to provide more specific messages for missing methods:

php > (new class {})->foo(); Warning: Uncaught Error: Call to undefined method class@anonymous::foo() in php shell code:1 Stack trace: #0 {main} thrown in php shell code on line 1

Or just remove it. The methods have been removed for over twelve years.

@throws probably unnecessary

Oops, forgot to remove the \ at the start of the name in the throws anyway. I'll remove the tag.

This might need to be SimplePieException? Or does the local Exception class shadow \Exception?

This uses the existing SimplePie\Exception. As the classes are in the same namespace, no import use statement is needed and the SimplePie Exception will be used.

Or just remove it. The methods have been removed for over twelve years.

Well, yes and no. Those methods have effectively not been removed, but deprecated. Removing them should probably be done in a major version and my intention with this PR is that it will be backported to the 1.8 branch for release ahead of PHP 8.4, so even though __call() could be removed in master, that wouldn't solve PHP 8.4 compatibility for 1.8.

This uses the existing SimplePie\Exception. As the classes are in the same namespace, no import use statement is needed and the SimplePie Exception will be used.

I would expect that but then why do we have

simplepie/src/SimplePie.php

Line 22 in 9385c54

use SimplePie\Exception as SimplePieException;

Well, yes and no. Those methods have effectively not been removed, but deprecated.

If they were just deprecated, the __call would proxy the call to the original methods. But they were actually both deprecated and removed and the warning was not updated.

This uses the existing SimplePie\Exception. As the classes are in the same namespace, no import use statement is needed and the SimplePie Exception will be used.

I would expect that but then why do we have

simplepie/src/SimplePie.php

Line 22 in 9385c54

use SimplePie\Exception as SimplePieException;

Very good question, might just be to prevent confusion for contributors ?
It doesn't make a (functional) difference for this PR anyhow, though for consistency, I should maybe update the throw new to use the alias.

If they were just deprecated, the __call would proxy the call to the original methods. But they were actually both deprecated and removed and the warning was not updated.

Fair enough. 👍 Sounds like a good one to clean up in 2.0 in that case (after this PR has been merged and backported to 1.8).

Updating for consistency sounds good.

jtojnar · 2024-09-12T07:47:58Z

src/SimplePie.php

        if (version_compare(PHP_VERSION, '7.2', '<')) {
-            trigger_error('Please upgrade to PHP 7.2 or newer.');
-            die();
+            exit('Please upgrade to PHP 7.2 or newer.');


jtojnar

Good enough for me. We can follow up with removal for master later.

Alkarex · 2024-09-21T20:51:49Z

#884

PHP 8.4 deprecates passing `E_USER_ERROR` to `trigger_error()`, with the recommendation being to replace these type of calls with either an Exception or an `exit` statement. This commit fixes an instance found in the autoloader by changing it to a call to `exit`, which considering it is an error related to the autoloader not being registered seems the most appropriate. Ref: https://wiki.php.net/rfc/deprecations_php_8_4#deprecate_passing_e_user_error_to_trigger_error

PHP 8.4 deprecates passing `E_USER_ERROR` to `trigger_error()`, with the recommendation being to replace these type of calls with either an Exception or an `exit` statement. This commit fixes another instance of this issue. In this case, the call to `trigger_error()` is replaced by throwing a generic `SimplePie\Exception` for the same. Ref: https://wiki.php.net/rfc/deprecations_php_8_4#deprecate_passing_e_user_error_to_trigger_error

PHP 8.4 deprecates passing `E_USER_ERROR` to `trigger_error()`, with the recommendation being to replace these type of calls with either an Exception or an `exit` statement. This commit fixes the last instance of this issue. In this case, the call to `trigger_error()` is also replaced by throwing a generic `SimplePie\Exception` for the same. Ref: https://wiki.php.net/rfc/deprecations_php_8_4#deprecate_passing_e_user_error_to_trigger_error

... the call to `trigger_error()` would now throw a PHP notice, but with the `die()` after it, with still kill the program anyhow, so we may as well, exit with the error message and remove the `trigger_error()` function call completely.

pull-request-size bot added the size/S label Sep 12, 2024

jrfnl mentioned this pull request Sep 12, 2024

[1.8] Backport PHP 8.4 compat fixes #875

Merged

jtojnar reviewed Sep 12, 2024

View reviewed changes

jrfnl force-pushed the feature/php-8.4-fix-trigger-user-error branch from 4bd2712 to c4d0230 Compare September 12, 2024 08:46

pull-request-size bot added size/XS and removed size/S labels Sep 12, 2024

jtojnar approved these changes Sep 12, 2024

View reviewed changes

jrfnl force-pushed the feature/php-8.4-fix-trigger-user-error branch from c4d0230 to 164cbbb Compare September 12, 2024 20:17

jtojnar approved these changes Sep 12, 2024

View reviewed changes

This was referenced Sep 14, 2024

Gzdecode: Deprecate our gzip implementation #882

Merged

Fix CI for PHPStan 1.12 #878

Merged

Alkarex approved these changes Sep 14, 2024

View reviewed changes

Alkarex mentioned this pull request Sep 14, 2024

Feature/php 8.4 fix trigger user error FreshRSS/simplepie#25

Merged

Art4 approved these changes Sep 25, 2024

View reviewed changes

jrfnl added 4 commits September 25, 2024 21:47

jtojnar force-pushed the feature/php-8.4-fix-trigger-user-error branch from 164cbbb to 688617d Compare September 25, 2024 19:47

jtojnar merged commit 2a05361 into simplepie:master Sep 25, 2024

jrfnl deleted the feature/php-8.4-fix-trigger-user-error branch September 25, 2024 21:22

jtojnar mentioned this pull request Sep 25, 2024

PHP 8.4 | Fix implicitly nullable parameters #880

Merged

Art4 added this to the 1.9.0 milestone Sep 30, 2024

Conversation

jrfnl commented Sep 12, 2024

PHP 8.4 | Fix passing E_USER_ERROR to trigger_error() [1]

PHP 8.4 | Fix passing E_USER_ERROR to trigger_error() [2]

PHP 8.4 | Fix passing E_USER_ERROR to trigger_error() [3]

QA: replace another call to trigger_error() with exit

Uh oh!

jrfnl commented Sep 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtojnar left a comment

Choose a reason for hiding this comment

Uh oh!

Alkarex commented Sep 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants