Skip to content

fibers: fix GH-23921 (Fibers start with error_reporting = 0 when the error_reporting INI directive is not set) - #23930

Closed
Girgias wants to merge 2 commits into
php:masterfrom
Girgias:8.6-gh23921
Closed

Girgias wants to merge 2 commits into
php:masterfrom
Girgias:8.6-gh23921

Conversation

@Girgias

@Girgias Girgias commented Sep 26, 2026

Copy link
Copy Markdown
Member

The way zend_fiber_execute() was querying the error reporting was rather strange, as it didn't rely on the value of EG(error_reporting). This was done as the @ silence operator could leak inside the body of fibers, however this situation can be detected more simply if the value of EG(error_reporting) and the value of the INI setting differ.

I'm not fully confident this is the correct fix but the only test that fails if we don't query the INI setting is Zend/tests/fibers/silence-operator-outside-fiber.phpt.

Note: the new test always succeeded as the test runner (I think) has a default INI setting.

…he error_reporting INI directive is not set)

The way zend_fiber_execute() was querying the error reporting was rather strange, as it didn't rely on the value of EG(error_reporting).
This was done as the @ silence operator could leak inside the body of fibers, however this situation can be detected more simply if the value of EG(error_reporting) and the value of the INI setting differ.
@Girgias
Girgias requested a review from arnaud-lb September 26, 2026 18:25
@Girgias
Girgias marked this pull request as ready for review September 27, 2026 11:26
Comment thread Zend/zend_fibers.c Outdated
zend_long error_reporting = EG(error_reporting);
/* A silence operator @ may modify the error_reporting value without changing the underlying INI value */
if (error_reporting != zend_ini_long_literal("error_reporting")) {
error_reporting = E_ALL;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pre-existing, but E_ALL looks wrong here as it may widen the original error reporting:

error_reporting(E_WARNING);

@(new Fiber(function () {
    var_dump(error_reporting()); // expected: E_WARNING, actual: E_ALL
}))->start();

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a new test, as indeed this was being messed with.
Turns out we can just use zend_ini_long_literal() (at least on 8.6+)

@Girgias
Girgias requested a review from arnaud-lb September 29, 2026 13:51

@arnaud-lb arnaud-lb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@Girgias Girgias closed this in 3bb50c1 Sep 29, 2026
@Girgias
Girgias deleted the 8.6-gh23921 branch September 29, 2026 14:29
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.

Fibers start with error_reporting = 0 when the error_reporting INI directive is not set

2 participants