keycdn / cache-enabler

A lightweight caching plugin for WordPress that makes your website faster by generating static HTML files.
https://wordpress.org/plugins/cache-enabler/
123 stars 46 forks source link

Incompatible with the AMP plugin #78

Closed westonruter closed 4 years ago

westonruter commented 4 years ago

I've received a few reports regarding Cache Enabler breaking AMP pages as generated by the AMP plugin:

There seems to be some fundamental conflict with the output buffering being done by the AMP plugin and the output buffering done by Cache Enabler. The AMP plugin starts output buffering at the template_redirect action. I don't see why this would be a conflict, nevertheless.

Anyway, I wanted to alert you to the issue. I'm a developer of the AMP plugin, and I'm happy to help investigate why it is incompatible.

There is a workaround for sites using the AMP plugin in Transitional mode or Reader mode, but not Standard mode.

coreykn commented 4 years ago

Cache Enabler is also using the template_redirect to start our output buffer (handle_cache). When replicating this issue it looks like Cache Enabler is caching the page before AMP has completed its output.

Can you use our cache_enabler_before_store filter to modify the page before its cached?

westonruter commented 4 years ago

Thanks for getting back to me. Given that Cache Enabler is starting output buffering at priority 0 it makes sense that it is caching unprocessed output from the AMP plugin since the AMP plugin starts output buffering at the minimum priority:

https://github.com/ampproject/amp-wp/blob/183be02c259284d2d95fc2a6d017805bf93fba5a/includes/class-amp-theme-support.php#L989

So that explains the conflict.

The reason for the AMP plugin using the minimum priority to ensure that output buffering starts for custom templates that get rendered at template_redirect rather than later via template_include. Granted, the minimum priority is probably excessive. If the AMP plugin started output buffering at priority 5 then that would fix compatibility with your plugin.

I'll need to think about the ramifications of doing that, however.

I can see that WP Super Cache starts output buffering earlier, at init, so that is why it hasn't had this same problem.

westonruter commented 4 years ago

I've opened a PR to address this in the AMP plugin: https://github.com/ampproject/amp-wp/pull/5204

westonruter commented 4 years ago

@coreykn Question: Is there a reason why output buffering is started at template_redirect priority 0? Is the intention that it be started as soon as possible? If that's the case, priorities can also be negative. So this is why the AMP plugin is not compatible. If I compare Cache Enabler with other caching plugins, I see that they start output buffering much earlier than template_redirect:

Also, while Autoptimize starts output buffer normally at template_redirect priority 10, if the AUTOPTIMIZE_INIT_EARLIER constant is defined then it starts at the init action. On such sites, the Autoptimize process would not be included in the cached content served by Cache Enabler.

All this to say, it seems that Cache Enabler would better align its behavior with other caching plugins if it would start output buffering earlier. This would also improve compatibility with other plugins in the ecosystem, such as the AMP plugin.

How about replacing the current template_redirect action with init? I am now answering my own question starting out this comment: I see this isn't a matter of simply swapping out the action since Cache_Enabler::handle_cache() calls Cache_Enabler::_bypass_cache() which depends on the WP_Query being populated. So that must be why the late initialization is being done (although it could be done at the wp action as well). If init were used instead, handle_cache would need to start output buffering earlier, but then at template_redirect (or the wp action) it could then flush the output buffer if _bypass_cache returns true.

We evaluated changing the action priorities in the AMP plugin but determined it was likely going to cause other issues: https://github.com/ampproject/amp-wp/pull/5204#issuecomment-672268056. So I'm closing that PR in favor of requesting that Cache Enabler start output buffering earlier, for the reasons above. Thanks!

coreykn commented 4 years ago

Thank you for the detailed information, @westonruter. The information and insight you've provided is sincerely appreciated. Let me dig into this more to see what would be the best solution going forward. Further updates to follow.

janvitos commented 4 years ago

Hi @coreykn, I've stumbled upon this issue myself and had to implement the RegEx /(\?amp(=.*))|\/amp\/?(\?.*)?$/ to get it working, but this means that AMP pages are not cached anymore.

It would be great if AMP pages could be cached. Did you have time to work out a solution for this? Or will a solution be implemented in the near future?

Thank you.