Give a minify builder its own file list - #731
Merged
Merged
Conversation
builder() returns clone $this, and PHP's shallow copy left the clone sharing the source's SplObjectStorage — so a file added to the builder was added to whatever it was built from as well, which is the opposite of what asking for a builder means. Two builders from one service shared with each other for the same reason. Nothing trips over it today because each request builds a fresh service and asks for one builder; the moment a service is reused, the stylesheet route would serve the script route's files alongside its own. __clone() gives the copy an empty storage. Covered both ways: the source is untouched by what its builder collects, and two builders do not see each other's files. The bundles the login page links were fetched from the running application afterwards — both stylesheets and both scripts still serve at full size.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
The bug
Minify::builder()returnsclone $this, and PHP's shallow copy left the clone sharing the source'sSplObjectStorage. So a file added to the builder was added to whatever it was built from as well — and two builders taken from one service shared with each other, for the same reason.That is the opposite of what asking for a builder means.
Nothing trips over it today: each request builds a fresh service and calls
builder()once. The moment a service is reused, the stylesheet route would serve the script route's files alongside its own.The fix
__clone()gives the copy an empty storage.Testing
Both directions, and each fails against the old code:
Reading the list back needs reflection — it is private, deliberately — which is the only way to tell one builder's contents from another's without rendering both.
And because a change to how assets are gathered deserves more than a green suite, the bundles the login page actually links were fetched from the running application afterwards:
Unit suite green: 3083. PHPStan and PHPCS clean.