Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Feature] Full-Page Caching for 404s #46
[Feature] Full-Page Caching for 404s #46
Changes from 48 commits
81b09b9
4d57b69
4598a31
bc52963
1c97d49
b6e709f
730dd63
c8c9f8b
466282f
38ddf23
6e3946d
951296c
71b88bb
20ad3e8
a51bcc9
4a2ae4a
b776b39
789eb19
15364ae
681cc8e
70fff9a
9b30221
b91b3da
900cb71
b31104f
cde8591
c9210e4
b53b3de
56b4921
d36f87f
7782eb8
93c4b69
13c94f1
0fcef02
6b166c9
b326f71
ff06717
d905e53
df26556
6336e62
83b95d2
5b2021b
6bacb48
6f9c270
7a8c1ee
f6f985f
cd33707
f628f26
1590143
ac984cd
8017c6a
a2c04df
370860d
006f900
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, while testing this in a client site, I noticed PHP can run multiple buffers in a request. In my testing environment, there was only one, but in the client site, by the time the code gets here, it is buffer number 2. So it returns the wrong value (empty).
I'm inclined to remove this line since it will
exit
anyway.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get the following error while running the unit tests, but not while testing on a site. 🤔
Test code or tested code did not (only) close its own output buffers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I learned that buffers can actually be nested and it is pretty hard to match the status with the initial value.
So here, we are clearing any previous buffers before starting our own.
8017c6a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be closing the output buffer that we're creating. We could also, potentially, keep the output buffer that already exists if output buffering is already active.
ob_get_status will tell us the current status of output buffering. If there is an active buffer, it will return a non-empty array. If there is not an active buffer, it will return an empty array. We could use that function to detect whether an output buffer is currently active and set a flag, which could control whether we spin up a new output buffer or use the current one, or whether we capture anything that's already in an existing output buffer for use later.
Also, the current implementation relies on a callback for when the buffer is naturally flushed, but we could perhaps hook into
shutdown
and capture the output there and add it to the cache.Alternately, this doesn't need to be considered an Alleyvate bug, and could instead be incumbent upon sites that use this plugin to ensure they don't have an active output buffering session on the 404 page, as it would break this feature (as written). Having an active output buffering session on the 404 page that doesn't come from this plugin is likely to be an edge case and could better be handled in that codebase rather than trying to code around it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I revisited this morning and I had 3 buffers already present before this one in a regular site and client site. No idea yet how. ¯_(ツ)_/¯
But I'll take your notes and do a bit more digging and hopefully apply a solution that works by default.
🤔 I'd expect more buffers to be available at this point, added by plugins, etc. I'll test this approach just in case.
I'll try to find a way to make it work for "any" sites.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick feedback after some testing:
shutdown
hook: the buffer always returns empty. My guess is that it is already cleared by the time it gets there. It's not that easy to track buffers.ob_get_status
is useful, but any attempt to clear the buffer after that, clears the html and it returns empty instead of the 404 page buffer. I tried a mixture of flushing the previous buffer and flushing ours later. I'm at a lost at why it is empty.So far, any attempt to clear the buffer after
ob_start( [ self::class, 'finish_output_buffering' ] );
, it is essentially clearing the HTML from the buffer and returning""
. Meaning that we are not caching the html page.