From 2a086e4e73a212773a6f2addbed868209013169c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= Date: Sun, 31 Aug 2025 00:32:42 +0200 Subject: [PATCH] =?UTF-8?q?opcache:=20Improve=20error=20messages=20when=20?= =?UTF-8?q?=E2=80=9Ctemporarily=20enabling=20OPcache=E2=80=9D=20(#19619)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * opcache: Do not emit “temporary enabling” message when OPcache is already active An easy way to accidentally enable OPcache “temporarily” is by using `php_admin_value[opcache.enable]=1` within a FPM pool’s configuration, since the `php_admin_value` settings mostly behave like settings in php.ini, with many OPcache INI settings being a notable exception. As long as OPcache is already enabled within php.ini (or simply by default), emitting a warning for `php_admin_value[opcache.enable]=1` or similar is going to be confusing, since is not actually temporarily enabling anything. A follow-up commit will also try to detect this kind of incorrect configuration and try to provide better advice for cases where OPcache is actually not yet enabled. * opcache: Improve error message when OPcache is enabled dynamically The error message will now advice on the `php_admin_value[opcache.enable]=1` mistake. It will also send the message to OPcache’s logging facility instead of the regular error handling logic during startup so that it will not be made available to `error_get_last()`, since it is related to a specific request and thus not actionable by a script either. php/php-src#19146 made a related change to `opcache.memory_consumption`. * opcache: Fix typo in warning message * opcache: Use more formal language in warning message --- .../tests/opcache_enable_noop_001.phpt | 24 +++++++++ .../tests/opcache_enable_noop_002.phpt | 18 +++++++ ext/opcache/zend_accelerator_module.c | 17 ++++++- .../fpm/tests/opcache_enable_admin_value.phpt | 49 +++++++++++++++++++ 4 files changed, 106 insertions(+), 2 deletions(-) create mode 100644 ext/opcache/tests/opcache_enable_noop_001.phpt create mode 100644 ext/opcache/tests/opcache_enable_noop_002.phpt create mode 100644 sapi/fpm/tests/opcache_enable_admin_value.phpt diff --git a/ext/opcache/tests/opcache_enable_noop_001.phpt b/ext/opcache/tests/opcache_enable_noop_001.phpt new file mode 100644 index 00000000000..0e7f5bbc987 --- /dev/null +++ b/ext/opcache/tests/opcache_enable_noop_001.phpt @@ -0,0 +1,24 @@ +--TEST-- +Dynamically setting opcache.enable does not warn when noop +--INI-- +opcache.enable=1 +opcache.enable_cli=1 +--EXTENSIONS-- +opcache +--FILE-- + +--EXPECTF-- +Should not warn: +Disabling: +Should warn: + +Warning: Zend OPcache can't be temporarily enabled (it may be only disabled until the end of request) in %s on line %d diff --git a/ext/opcache/tests/opcache_enable_noop_002.phpt b/ext/opcache/tests/opcache_enable_noop_002.phpt new file mode 100644 index 00000000000..994f2d22f4e --- /dev/null +++ b/ext/opcache/tests/opcache_enable_noop_002.phpt @@ -0,0 +1,18 @@ +--TEST-- +Dynamically setting opcache.enable warns when not a noop +--INI-- +opcache.enable=0 +opcache.enable_cli=1 +--EXTENSIONS-- +opcache +--FILE-- + +--EXPECTF-- +Should warn, since the INI was initialized to 0: + +Warning: Zend OPcache can't be temporarily enabled (it may be only disabled until the end of request) in %s on line %d diff --git a/ext/opcache/zend_accelerator_module.c b/ext/opcache/zend_accelerator_module.c index 9a168b38baa..6f668af9b71 100644 --- a/ext/opcache/zend_accelerator_module.c +++ b/ext/opcache/zend_accelerator_module.c @@ -159,10 +159,23 @@ static ZEND_INI_MH(OnEnable) stage == ZEND_INI_STAGE_DEACTIVATE) { return OnUpdateBool(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage); } else { - /* It may be only temporary disabled */ + /* It may be only temporarily disabled */ bool *p = (bool *) ZEND_INI_GET_ADDR(); if (zend_ini_parse_bool(new_value)) { - zend_error(E_WARNING, ACCELERATOR_PRODUCT_NAME " can't be temporary enabled (it may be only disabled till the end of request)"); + if (*p) { + /* Do not warn if OPcache is enabled, as the update would be a noop anyways. */ + return SUCCESS; + } + + if (stage == ZEND_INI_STAGE_ACTIVATE) { + if (strcmp(sapi_module.name, "fpm-fcgi") == 0) { + zend_accel_error(ACCEL_LOG_WARNING, ACCELERATOR_PRODUCT_NAME " can't be temporarily enabled. Are you using php_admin_value[opcache.enable]=1 in an individual pool's configuration?"); + } else { + zend_accel_error(ACCEL_LOG_WARNING, ACCELERATOR_PRODUCT_NAME " can't be temporarily enabled (it may be only disabled until the end of request)"); + } + } else { + zend_error(E_WARNING, ACCELERATOR_PRODUCT_NAME " can't be temporarily enabled (it may be only disabled until the end of request)"); + } return FAILURE; } else { *p = 0; diff --git a/sapi/fpm/tests/opcache_enable_admin_value.phpt b/sapi/fpm/tests/opcache_enable_admin_value.phpt new file mode 100644 index 00000000000..2c1081aed9e --- /dev/null +++ b/sapi/fpm/tests/opcache_enable_admin_value.phpt @@ -0,0 +1,49 @@ +--TEST-- +Setting opcache.enable via php_admin_value fails gracefully +--SKIPIF-- + +--FILE-- +start(iniEntries: [ + 'opcache.enable' => '0', + 'opcache.log_verbosity_level' => '2', +]); +$tester->expectLogStartNotices(); +$tester->expectLogPattern("/Zend OPcache can't be temporarily enabled. Are you using php_admin_value\\[opcache.enable\\]=1 in an individual pool's configuration?/"); +echo $tester + ->request() + ->getBody(); +$tester->terminate(); +$tester->close(); + +?> +--EXPECT-- +NULL +--CLEAN-- +