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

Static caching fallback for long query strings #3864

Merged
merged 3 commits into from
Jun 18, 2021

Conversation

arthurperton
Copy link
Contributor

I've been following #3210 and #3860.

This PR adds a fallback for requests with long query strings by hashing them.

The retrieval of the related responses from the cache will be slower than for regular requests, because Statamic needs to be fired up. I guess the performance is comparable to half measure static caching. Still beats not caching the response at all though.

I also made max_filename_length configurable for those who know what they're doing and want to tweak things to the max.

@jasonvarga
Copy link
Member

This is great. Definitely better to have something than nothing.

@jasonvarga
Copy link
Member

I tweaked this implementation a bit.

  • Removed the "could not write file" log, since it would never be reached as you always write the file now.
  • Removed the max_filename_length from the manager class and just put it as a fallback in the method
  • Gave the filenames a _lqs_ (long query string) identifier in it, so we can continue to just show the "your server rewrites arent set up properly" log when necessary. You won't see it just because of a long url.
  • Tweaked the file path building logic so there's less repeating.
  • Updated the tests to make sure we don't accidentally check the whole path against the max length, instead of just the basename. (Which I noticed I did - not you - at one point during the refactor)

@jasonvarga jasonvarga merged commit 08b9e05 into statamic:3.1 Jun 18, 2021
@arthurperton
Copy link
Contributor Author

Great tweaks!

Careful here though:

  • Removed the "could not write file" log, since it would never be reached as you always write the file now.

Even with a hashed query string the entire filename could still be too long (and wreak 500 havoc). That would only happen with very long slugs and/or short max_filename_length's of course.

I left this check in place on purpose for this unlikely but possible scenario.

@jasonvarga
Copy link
Member

Fair enough. Thanks. 👍

@arthurperton
Copy link
Contributor Author

np

@arthurperton
Copy link
Contributor Author

@jasonvarga Is it your intention to put it back in, or do you want me to create a PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants