WP-CLI commands trigger undefined index errors

[nlemoine]

https://github.com/keycdn/cache-enabler/blob/master/inc/cache_enabler.class.php#L36-L38

The Cache_Engine instantiation isn't bound to any hook and thus is triggered on every WordPress load in every context (backend, wp-cli, etc.).

WP-CLI commands for example returns some notices (wp post list or any command):

PHP Notice:  Undefined index: HTTP_HOST in .../cache-enabler/inc/cache_enabler_disk.class.php on line 483
PHP Notice:  Undefined index: SERVER_PORT in .../cache-enabler/inc/cache_enabler_disk.class.php on line 515

Maybe this call should be adjusted?

[coreykn]

Oops! You're right. Thanks for bringing this to my attention. I'll come up with a fix shortly.

[nlemoine]

WP-CLI isn't the only issue with this very early instantiation (hence the original issue title).

This also means that Cache_Engine will trigger those methods even if they're not needed (back-end, AJAX, etc.). For performance reason, It would be better if those could be avoided.

I'm having another issue with this pattern. I think it's in the same scope. Settings are retrieved very early too: https://github.com/keycdn/cache-enabler/blob/master/inc/cache_enabler.class.php#L1667

I use option filters (pre_option_{$option}, option_${option}) to ensure specific settings are always set and right. Now, I have to create a plugin/mu-plugin and hook very early (plugins_loaded with a high priority) to get those hooks working.

Could you use a regular get_option instead of using Cache_Enabler_Engine::settings?

[coreykn]

The only bug that I have been able to replicate from this is what you provided with the WP-CLI (hence the title change). I'll update this or add new issues if I find anything else. If you find anything new please let me know.

The engine is required in the backend. The engine is required in certain types of POST requests, like when any post type is submitted or a comment is posted. I initially had tried to introduce more limitations around this but it's needed in many different scenarios, so this is how it's been introduced and can be adjusted with time as I see the results.

The engine only needs the settings from the disk when trying to deliver the cache. The engine only needs the settings from the database (which is using get_option()) if starting the engine late, which is used for plugin handling or generating the cache. If a cached page exists and the cache is not delivered then the settings from the disk will be used (like visiting a page that is cached but the user is logged in and has the cookies exclusion setting bypassing the cache for logged in users). This prevents loading the settings twice.

[nlemoine]

Thanks for your feedback.

The early instantiation of cache engine isn't a bug. I'm just wondering if calling these methods on each WP loads, where it's sometimes uneeded (I don't really get why you have to start it so early on the back end for example), could be made in a more efficient manner.

Sorry, there's a misunderstanding about the settings part. I already figured the process out. I have probably not been clear enough. I'll try to make my request clearer. Can you please add a $settings = get_option('cache_enabler') here? https://github.com/keycdn/cache-enabler/blob/master/inc/cache_enabler.class.php#L1622

And replace Cache_Enabler_Engine::$settings by $settings? options are stored in object cache, calling them twice isn't a problem.

[coreykn]

I agree starting the engine (creating its instance) can be improved. As mentioned previously I think this can be done with more time and data. Please keep in mind that there is only so much that I can do at one time, @nlemoine.

Why would we call both the settings from the disk and the database in one request? I find calling the database when we already have the settings available unnecessary.

[nlemoine]

Why would we call both the settings from the disk and the database in one request? I find calling the database when we already have the settings available unnecessary.

When you're requesting the settings page (back-end), your settings won't be grabbed from the disk. Here's the call stack:

• https://github.com/keycdn/cache-enabler/blob/master/inc/cache_enabler_engine.class.php#L48-L50 Cache_Enabler_Disk::cache_exists() will return false because you are in wp-admin.
• So settings will be grabbed from database: https://github.com/keycdn/cache-enabler/blob/master/inc/cache_enabler_engine.class.php#L52

Calling them multiple times isn't an issue:

1. Options are cached in memory during the same request
2. Options are only called on the settings page

[coreykn]

The changes you've proposed are unnecessary. To achieve what you're wanting the late engine start would just need to be tweaked in the Cache_Enabler constructor. I'll look into this as well.

[nlemoine]

Thanks! I'll keep an eye on future updates and try to be more patient.

[coreykn]

When and where are you calling the filter hooks above? In the PR above I've changed the late engine start to be on init instead of when the new instance of Cache_Enabler is created on plugins_loaded. I'm unsure if this is still too early for what you're trying to do. If it is too early, is filtering Cache_Enabler_Engine::$settings directly an option instead?

[nlemoine]

When and where are you calling the filter hooks above?

On after_setup_theme, this is all good now. Thanks!