Skip to content

OPcache bypasses the user-defined error handler for deprecations #17422

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

Open
dtdesign opened this issue Jan 9, 2025 · 10 comments
Open

OPcache bypasses the user-defined error handler for deprecations #17422

dtdesign opened this issue Jan 9, 2025 · 10 comments

Comments

@dtdesign
Copy link

dtdesign commented Jan 9, 2025

Description

The OPcache bypasses the user-defined error handler for deprecations for the first request with a cold cache. This happens regardless of opcache.record-warnings which only has an impact on all following requests. Enabling opcache.record-warnings will yield the expected behavior for every request after the first one which populated the cache.

This does not happen when OPcache is disabled, it will behave properly for every request.

test.php

<?php

set_error_handler(static fn (...$params) => var_dump($params));

require "./dependency.php";

implicitly_nullable_parameter(new stdClass());

dependency.php

<?php
function implicitly_nullable_parameter(stdClass $foo = null) {
    echo "Called\n";
}

This can be reproduced on a web server whenever opcache_reset() reset is invoked but for simplicity this was also verified to be triggered via CLI (thanks to @TimWolla for this suggestion):

$> php -dopcache.enable_cli=1 --repeat 2 test.php
Executing for the first time...
PHP Deprecated:  implicitly_nullable_parameter(): Implicitly marking parameter $foo as nullable is deprecated, the explicit nullable type must be used instead in /opt/homebrew/var/www/test/opcache-deprecations/dependency.php on line 2

Deprecated: implicitly_nullable_parameter(): Implicitly marking parameter $foo as nullable is deprecated, the explicit nullable type must be used instead in /opt/homebrew/var/www/test/opcache-deprecations/dependency.php on line 2
Called
Finished execution, repeating...
Called
$> php --version
PHP 8.4.2 (cli) (built: Dec 17 2024 15:31:31) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.4.2, Copyright (c) Zend Technologies
    with Zend OPcache v8.4.2, Copyright (c), by Zend Technologies

(Note: There is a bonus bug hiding here, the deprecation message is printed twice. However, the first message starts with PHP Deprecated: (…) but the second message only reads Deprecated: (…).)

The script was also invoked manually via CLI without OPcache to verify that the behavior is identical to FPM:

$> php test.php
array(4) {
  [0]=>
  int(8192)
  [1]=>
  string(141) "implicitly_nullable_parameter(): Implicitly marking parameter $foo as nullable is deprecated, the explicit nullable type must be used instead"
*snip*

PHP Version

PHP 8.4.2

Operating System

macOS 15.1.1

@TimWolla
Copy link
Member

OPcache explicitly clears the user error handler in:

ZVAL_UNDEF(&EG(user_error_handler));

before compiling the file. This is done since the first version of the open source OPcache, so it's not clear to me, why it does this.

@tomasjanicek
Copy link

tomasjanicek commented Jan 16, 2025

I have same problem on php version 8.4.1

@kkmuffme
Copy link

Works correctly for me (Rocky Linux)

@jmglsn
Copy link

jmglsn commented Jan 20, 2025

Bug is still present on PHP 8.4.3.

@dtdesign
Copy link
Author

(Note: There is a bonus bug hiding here, the deprecation message is printed twice. However, the first message starts with PHP Deprecated: (…) but the second message only reads Deprecated: (…).)

I stand corrected, this is caused by log_errors being enabled, thanks to @TimWolla for pointing this out:

$> php -dopcache.enable_cli=1 -dlog_errors=0 --repeat 2 test.php
Executing for the first time...

Deprecated: implicitly_nullable_parameter(): Implicitly marking parameter $foo as nullable is deprecated, the explicit nullable type must be used instead in /opt/homebrew/var/www/test/opcache-deprecations/dependency.php on line 2
Called
Finished execution, repeating...
Called

TimWolla added a commit to TimWolla/php-src that referenced this issue Jan 29, 2025
…ile()`

The logic to disable userland error handlers existed since the first commit
including OPcache in php-src. The reason unfortunately is not explained. No
existing tests require that userland error handlers do not trigger in
`opcache_compile_file()`. The newly added tests are intended to exercise all
possible kinds of edge-cases that might arise and cause issues (e.g. throwing
Exceptions, performing a bailout, or loading a different file from within the
error handler).

They all pass for both `--repeat 1` and `--repeat 2`, as well as with OPcache
enabled and without OPcache enabled (in the latter case with the exception of
the `opcache_get_status()` verification). The list of cached files at the end
of execution also matches my expectations.

Therefore it seems that disabling the userland error handler when compiling a
file with OPcache is not (or at least no longer) necessary.

Fixes php#17422.
TimWolla added a commit to TimWolla/php-src that referenced this issue Jan 29, 2025
…ile()`

The logic to disable userland error handlers existed since the first commit
including OPcache in php-src. The reason unfortunately is not explained. No
existing tests require that userland error handlers do not trigger in
`opcache_compile_file()`. The newly added tests are intended to exercise all
possible kinds of edge-cases that might arise and cause issues (e.g. throwing
Exceptions, performing a bailout, or loading a different file from within the
error handler).

They all pass for both `--repeat 1` and `--repeat 2`, as well as with OPcache
enabled and without OPcache enabled (in the latter case with the exception of
the `opcache_get_status()` verification). The list of cached files at the end
of execution also matches my expectations.

Therefore it seems that disabling the userland error handler when compiling a
file with OPcache is not (or at least no longer) necessary.

Fixes php#17422.
@DylanGD
Copy link

DylanGD commented Feb 25, 2025

The bug is still present on PHP 8.4.4

@kbalashov1
Copy link

Confirming seeing this issue, that specific deprecation ignoring our error handlers messes up our json outputs and blocks us from upgrading to 8.4. Fixing all deprecations isn't an option, as we can do so in our own code but not in the packages that we use.

@umpirsky
Copy link

umpirsky commented Apr 9, 2025

The bug is still present on PHP 8.4.5.

@aszenz
Copy link

aszenz commented Apr 9, 2025

I can confirm this issue is present on our symfony app

@lewischadwick
Copy link

Confirmed this still happens on PHP 8.4.6 too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests