Skip to content

Commit 2a086e4

Browse files
TimWollaTimWolla
authored
opcache: Improve error messages when “temporarily enabling OPcache” (#19619)
* 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. #19146 made a related change to `opcache.memory_consumption`. * opcache: Fix typo in warning message * opcache: Use more formal language in warning message
1 parent 3bf495b commit 2a086e4

File tree

4 files changed

+106
-2
lines changed

4 files changed

+106
-2
lines changed

‎ext/opcache/tests/opcache_enable_noop_001.phpt

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
--TEST--
2+
Dynamically setting opcache.enable does not warn when noop
3+
--INI--
4+
opcache.enable=1
5+
opcache.enable_cli=1
6+
--EXTENSIONS--
7+
opcache
8+
--FILE--
9+
<?php
10+
11+
echo "Should not warn:", PHP_EOL;
12+
ini_set('opcache.enable', 1);
13+
echo "Disabling:", PHP_EOL;
14+
ini_set('opcache.enable', 0);
15+
echo "Should warn:", PHP_EOL;
16+
ini_set('opcache.enable', 1);
17+
18+
?>
19+
--EXPECTF--
20+
Should not warn:
21+
Disabling:
22+
Should warn:
23+
24+
Warning: Zend OPcache can't be temporarily enabled (it may be only disabled until the end of request) in %s on line %d

‎ext/opcache/tests/opcache_enable_noop_002.phpt

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
--TEST--
2+
Dynamically setting opcache.enable warns when not a noop
3+
--INI--
4+
opcache.enable=0
5+
opcache.enable_cli=1
6+
--EXTENSIONS--
7+
opcache
8+
--FILE--
9+
<?php
10+
11+
echo "Should warn, since the INI was initialized to 0:", PHP_EOL;
12+
ini_set('opcache.enable', 1);
13+
14+
?>
15+
--EXPECTF--
16+
Should warn, since the INI was initialized to 0:
17+
18+
Warning: Zend OPcache can't be temporarily enabled (it may be only disabled until the end of request) in %s on line %d

‎ext/opcache/zend_accelerator_module.c

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -159,10 +159,23 @@ static ZEND_INI_MH(OnEnable)
159159
stage == ZEND_INI_STAGE_DEACTIVATE) {
160160
return OnUpdateBool(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage);
161161
} else {
162-
/* It may be only temporary disabled */
162+
/* It may be only temporarily disabled */
163163
bool *p = (bool *) ZEND_INI_GET_ADDR();
164164
if (zend_ini_parse_bool(new_value)) {
165-
zend_error(E_WARNING, ACCELERATOR_PRODUCT_NAME " can't be temporary enabled (it may be only disabled till the end of request)");
165+
if (*p) {
166+
/* Do not warn if OPcache is enabled, as the update would be a noop anyways. */
167+
return SUCCESS;
168+
}
169+
170+
if (stage == ZEND_INI_STAGE_ACTIVATE) {
171+
if (strcmp(sapi_module.name, "fpm-fcgi") == 0) {
172+
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?");
173+
} else {
174+
zend_accel_error(ACCEL_LOG_WARNING, ACCELERATOR_PRODUCT_NAME " can't be temporarily enabled (it may be only disabled until the end of request)");
175+
}
176+
} else {
177+
zend_error(E_WARNING, ACCELERATOR_PRODUCT_NAME " can't be temporarily enabled (it may be only disabled until the end of request)");
178+
}
166179
return FAILURE;
167180
} else {
168181
*p = 0;

‎sapi/fpm/tests/opcache_enable_admin_value.phpt

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
--TEST--
2+
Setting opcache.enable via php_admin_value fails gracefully
3+
--SKIPIF--
4+
<?php include "skipif.inc"; ?>
5+
--FILE--
6+
<?php
7+
8+
require_once "tester.inc";
9+
10+
$cfg = <<<EOT
11+
[global]
12+
error_log = {{FILE:LOG}}
13+
[unconfined]
14+
listen = {{ADDR}}
15+
pm = static
16+
pm.max_children = 1
17+
catch_workers_output = yes
18+
php_admin_value[opcache.enable] = On
19+
php_admin_flag[display_errors] = On
20+
php_admin_flag[display_startup_errors] = On
21+
php_admin_flag[log_errors] = On
22+
EOT;
23+
24+
$code = <<<EOT
25+
<?php
26+
var_dump(error_get_last());
27+
EOT;
28+
29+
$tester = new FPM\Tester($cfg, $code);
30+
$tester->start(iniEntries: [
31+
'opcache.enable' => '0',
32+
'opcache.log_verbosity_level' => '2',
33+
]);
34+
$tester->expectLogStartNotices();
35+
$tester->expectLogPattern("/Zend OPcache can't be temporarily enabled. Are you using php_admin_value\\[opcache.enable\\]=1 in an individual pool's configuration?/");
36+
echo $tester
37+
->request()
38+
->getBody();
39+
$tester->terminate();
40+
$tester->close();
41+
42+
?>
43+
--EXPECT--
44+
NULL
45+
--CLEAN--
46+
<?php
47+
require_once "tester.inc";
48+
FPM\Tester::clean();
49+
?>

0 commit comments

Comments
 (0)