Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Experimental Request Handler breaks WP Rocket #406

Open
RafaelKr opened this issue Oct 7, 2024 · 4 comments · May be fixed by #423
Open

Experimental Request Handler breaks WP Rocket #406

RafaelKr opened this issue Oct 7, 2024 · 4 comments · May be fixed by #423
Labels
bug Something isn't working

Comments

@RafaelKr
Copy link
Contributor

RafaelKr commented Oct 7, 2024

Version

4.3.0

What did you expect to happen?

In a Radicle project we have the experimental Request handler enabled via ACORN_ENABLE_EXPERIMENTAL_WORDPRESS_REQUEST_HANDLER=true.

We now installed WP Rocket (Previously we had WP Optimize where it also didn't work).

It should create static html files inside public/content/cache/wp-rocket/example.com

What actually happens?

It only creates those static html files when the experimental request handler is disabled.

It's related to output buffering. WP Rocket receives an empty output buffer at https://github.com/wp-media/wp-rocket/blob/09708aa55d362a9535ad4d45cb68280325efaecb/inc/classes/Buffer/class-cache.php#L259

The order of calls is:

When we disable the experimental request handler the buffer contains the correct content and the static file is generated.

Steps to reproduce

  • Add ACORN_ENABLE_EXPERIMENTAL_WORDPRESS_REQUEST_HANDLER=true to .env
  • composer install wp-media/wp-rocket
  • Activate WP Rocket (By default it will add define( 'WP_CACHE', true ); // Added by WP Rocket to wp-config.php, but that should be fine for testing. Can be disabled via https://docs.wp-rocket.me/article/958-how-to-use-wp-rocket-with-bedrock-wordpress-boilerplate)
  • Go to public/content/cache/wp-rocket/your-domain.localhost (or the wp-content path which is configured without Radicle)
  • Load a page and see that there are no generated files
  • Remove ACORN_ENABLE_EXPERIMENTAL_WORDPRESS_REQUEST_HANDLER=true from .env
  • Load a page and now it should generate static files

System info

No response

Log output

No response

Please confirm this isn't a support request.

Yes

@RafaelKr RafaelKr added the bug Something isn't working label Oct 7, 2024
@RafaelKr
Copy link
Contributor Author

RafaelKr commented Oct 7, 2024

I created a patch which at least works with WP Rocket. I'll post it with some info later this week.

@RafaelKr
Copy link
Contributor Author

RafaelKr commented Oct 8, 2024

The problem I identified was that the code is running in the following order:

  1. WP Rocket is initialized very early via advanced-cache.php. It calls ob_start( [ $this, 'maybe_process_buffer' ] );.
  2. Acorn Bootloader is initialized and does ob_start().
  3. Acorn removes the default shutdown function with priority 1 which is added by WordPress. It adds an own shutdown function with priority 100 where the Response is handled by the Laravel Router.
  4. WordPress did all its work and the Acorn shutdown action is called.
  5. Inside the Acorn Router it does an ob_get_clean() for every ob_get_level().
  6. WP Rockets maybe_process_buffer method is called and receives an empty buffer because it was cleaned by Acorn.

I now modified the the previous steps as follows:

The code diff below is based on Acorn 4.3.0. The comments starting with // [X] are added afterwards. X is indicating the related item from list found below the diff. To apply the patch to Acorn the comments need to be removed.

diff --git a/src/Roots/Acorn/Bootloader.php b/src/Roots/Acorn/Bootloader.php
index de0a3ca..ffdcbe4 100644
--- a/src/Roots/Acorn/Bootloader.php
+++ b/src/Roots/Acorn/Bootloader.php
@@ -3,6 +3,7 @@
 namespace Roots\Acorn;
 
 use Illuminate\Contracts\Foundation\Application as ApplicationContract;
+use Illuminate\Http\Request;
 use Illuminate\Http\Response;
 use Illuminate\Support\Env;
 use Illuminate\Support\Facades\Facade;
@@ -203,7 +204,7 @@ class Bootloader
     protected function registerDefaultRoute(): void
     {
         $this->app->make('router')
-            ->any('{any?}', fn () => tap(response(''), function (Response $response) {
+          // [5] Pass in request
+            ->any('{any?}', fn (Request $request) => tap(response(''), function (Response $response) use ($request) {
                 foreach (headers_list() as $header) {
                     [$header, $value] = explode(': ', $header, 2);
 
@@ -218,12 +219,12 @@ class Bootloader
                     $response->header('X-Powered-By', $this->app->version());
                 }
 
-                $content = '';
+               // [5] Get content attached to the request
+                $content = $request->request->get('wp_ob_content');
 
                 $levels = ob_get_level();
 
                 for ($i = 0; $i < $levels; $i++) {
-                    $content .= ob_get_clean();
+                   // [5] Don't clean the output_buffer
+                    $content .= ob_get_contents();
                 }
 
                 $response->setContent($content);
@@ -286,9 +288,20 @@ class Bootloader
 
         $route->middleware($isApi ? $config['api'] : $config['web']);
 
+        // [2] Save ob starting level
+        $startLevel = ob_get_level();
         ob_start();
 
         remove_action('shutdown', 'wp_ob_end_flush_all', 1);
+        // [3] Add shutdown handler to end output buffering with the same priority as default WordPress does it
+        add_action('shutdown', function () use ($request, $startLevel) {
+            // [4] Clean buffer only until start level
+            $levels = ob_get_level() - $startLevel;
+            $content = '';
+
+            for ( $i = 0; $i < $levels; $i++ ) {
+                $content .= ob_get_clean();
+            }
+
+            // [4] Save content onto request to have it available in [5]
+            $request->request->set('wp_ob_content', $content);
+        }, 1);
         add_action('shutdown', fn () => $this->handleRequest($kernel, $request), 100);
     }
  1. [No change]
  2. Before Acorn does ob_start() we save the current level.
  3. We add an additional shutdown action with the same priority as the WordPress original action (priority 1).
  4. Inside our new shutdown action we run ob_get_clean() until the level we saved before ob_start(). The content is attached to the $request. This makes sure the WP Rocket output buffer which started before Acorn is not touched. Afterwards the original Acorn shutdown action with priority 100 is called, passing the request to the Laravel Router.
  5. We now get the content attached to the request. We now loop ob_get_level() again and attach it to the content. Instead of using ob_get_clean() we use ob_get_contents().
  6. Because the buffer is not cleaned WP Rocket receives the contents and can cache them.

This change works for WP Rocket and should also work for all other plugins which have an "outer" output buffer (a buffer started before Acorn). At least when the outer buffer is handled after a shutdown action with priority 100.

I'm not 100% sure if this change could lead to duplicated output. But I wouldn't expect that as original WordPress already flushes the buffer at shutdown with priority.

@Log1x
Copy link
Member

Log1x commented Dec 5, 2024

Care to get a PR up for testing?

RafaelKr added a commit to RafaelKr/acorn that referenced this issue Dec 16, 2024
Previously, Acorn with the experimental request handler enabled broke WordPress static caching plugins like WP Rocket. This happened because the caching plugins work at a higher level than Acorn, but Acorn cleared all output buffers before the caching plugin could handle the contents.

Fixes roots#406
@RafaelKr RafaelKr linked a pull request Dec 16, 2024 that will close this issue
RafaelKr added a commit to RafaelKr/acorn that referenced this issue Dec 16, 2024
Previously, Acorn with the experimental request handler enabled broke WordPress static caching plugins like WP Rocket. This happened because the caching plugins work at a higher level than Acorn, but Acorn cleared all output buffers before the caching plugin could handle the contents.

Fixes roots#406
@RafaelKr
Copy link
Contributor Author

Hey @Log1x I just created #423

Can you please also test this thoroughly with your current state of Acorn 5? I can only confirm that my patches work correctly in our production project with v4.3.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants