Fix Zend OPcache API warning when upgrading MODX

[JoshuaLuckers]

What does it do?

Check if we are allowed to call OPcache API functions.

Why is it needed?

To fix an OPcache API functions warning during upgrades.

How to test?

See issue #15897

Related issue(s)/PR(s)

Closes #15897

[JoshuaLuckers]

phpcs is failing because of this:

Error: <error line="14" column="1" severity="error" message="Each class must be in a namespace of at least one level (a top-level vendor name)" source="PSR1.Classes.ClassDeclaration.MissingNamespace"/>

Error: Each class must be in a namespace of at least one level (a top-level vendor name)

[opengeek]

I'm afraid we are oversimplifying the problem here. If opcache is enabled and calling the function is restricted in setup, I believe we need to warn the user and stop setup. Otherwise, we are going to load stale settings here and problems will arise. Or we need to find another way to make sure opcache is not used for setup.

[Omeryl]

My recommendation is to ini_set() opcache.enable to false for the set up, which is something that can be done at runtime.

opcache.enable bool
Enables the opcode cache. When disabled, code is not optimised or cached. The setting opcache.enable can not be enabled at runtime through ini_set(), it can only be disabled. Trying to enable it in a script will generate a warning.

[JoshuaLuckers]

My recommendation is to ini_set() opcache.enable to false for the set up, which is something that can be done at runtime.

opcache.enable bool
Enables the opcode cache. When disabled, code is not optimised or cached. The setting opcache.enable can not be enabled at runtime through ini_set(), it can only be disabled. Trying to enable it in a script will generate a warning.

At runtime this will only disable OPCache for the current request (it disables caching of the remaining scripts in your current request).
Source: https://stackoverflow.com/a/21590668

And if I understand correctly, the files already in cache are not invalidated. Other requests might still use the cached scripts/files. Therefore we need to invalidate it by calling opcache_invalidate.

[Omeryl]

@JoshuaLuckers As far as I understand, as long as the script file isn't yet included it will bypass the OPcache entirely. You'd definitely want to short circuit it as soon as possible.

[JoshuaLuckers]

I'm afraid we are oversimplifying the problem here. If opcache is enabled and calling the function is restricted in setup, I believe we need to warn the user and stop setup. Otherwise, we are going to load stale settings here and problems will arise. Or we need to find another way to make sure opcache is not used for setup.

I was looking at the PHP source code and if I'm not mistaken, calling opcache_invalidate will only mark the cached file as invalid. We might still end up with a cached version of the file until a certain threshold is met and the cache is refreshed. Correct me if I'm wrong.

[opengeek]

I'm afraid we are oversimplifying the problem here. If opcache is enabled and calling the function is restricted in setup, I believe we need to warn the user and stop setup. Otherwise, we are going to load stale settings here and problems will arise. Or we need to find another way to make sure opcache is not used for setup.

I was looking at the PHP source code and if I'm not mistaken, calling opcache_invalidate will only mark the cached file as invalid. We might still end up with a cached version of the file until a certain threshold is met and the cache is refreshed. Correct me if I'm wrong.

We've gone from over-simplifying to over-complicating. 😂 This was added because the cached settings values that were just written to the file were not getting loaded in this exact spot, causing old values to be used during this step of the upgrade. Adding this method fixed that problem, absolutely. If we prevent the settings file from ever being cached by turning this off for all of the setup, we can avoid the problem completely, I believe. This will of course require testing.

[JoshuaLuckers]

If we prevent the settings file from ever being cached by turning this off for all of the setup, we can avoid the problem completely, I believe. This will of course require testing.

I agree, we should turn off OPCache for all of the setup. I propose to always turn off OPCache if it's enabled, even if we are not restricted to call opcache_invalidate.

@Omeryl recommends to ini_set() opcache.enable to false for the set up, which is something that can be done at runtime, at the earliest as possible.

If it's ok with all of you to disable OPCache I will add a check to see if OPCache is enabled and disable it in both cli-install.php and index.php.

[opengeek]

This has never affected cli-install, to my knowledge. I believe it is unique to web install. Let's start with web and if the issue comes up, we can push the changes to cli.

[JoshuaLuckers]

@opengeek done!