-
Notifications
You must be signed in to change notification settings - Fork 3
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
Feat/checksum media #139
Open
gwillz
wants to merge
22
commits into
master
Choose a base branch
from
feat/checksum-media
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Feat/checksum media #139
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Only ever really used in tag().
This means we get to the real URL faster. With the current nginx/apache rewrites we'll see a double redirect before we get to the real file.
Mostly broken.
Now uses an parse -> instance -> action architecture. Everything should read a bit easier now and we do less work (and it actually works).
Because we've dumped the output buffer, kohana will make a mess after the method has finished.
aitken85
approved these changes
Feb 4, 2025
Note on testing
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Our first foray into the media controller was perhaps naive. We quickly discovered poor cache invalidation and other architecture mistakes.
This cleans all of that up using a per-section checksum technique.
Gone are the days of "oh this site has cached assets from the previous deploy, that didn't happen on QA". The behaviour is now consistent and any caching controls only affects recalculation of checksums, which also naturally expire with a TTL.
Whereas before if the cache was not cleared - it could never expire. Sprout was also unable to intercept these requests because the URL was static and existing media was only served by nginx/apache. Changing the
?timestamp
param cannot force a request through to Sprout.When creating an media asset URL, Sprout will now create a checksum for the whole section directory (core, sprout, skin, or module). This serves as the cache-buster for browser caches. Upon deploying new files we get a new checksum.
There's also a general tidy up of some alternate formats for specifying media format. Sometimes we'd omit the
/js|css/
directory or themodules/
prefix for a section. Somehow we thought we could reliably infer and fix if a skin URL was missing the subsite code. That's all standardised and documented in the Media helper now.Key things
parse()
andgenerateChecksum()
generate()
andredirect()
actionsTesting
Run the tests!
Also patch it into a Sprout 4 site and see how it goes. Edit some files, see the checksums change. Be sure to test for all media sections - core, sprout, skin, modules.