[Asset] Starting slash should indicate no basePath wanted #22528
Conversation
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
Member
|
thus, it only affects users deploying in a subdirectory and using leading slashes. And the recommendation was to avoid leading slashes precisely to allow deploying in subdirectories easily. |
javiereguiluz
approved these changes
Apr 26, 2017
Member
javiereguiluz
left a comment
javiereguiluz
left a comment
There was a problem hiding this comment.
👍
To me the real BC break was the first one which changed the original behavior. Besides, as you said, we didn't receive issue reports about this break, so it could be safe to reverse it now.
Member
|
rebase needed |
Member
|
@weaverryan Can you rebase? |
weaverryan
force-pushed
the
fix-base-path-slash
branch
from
18d7304 to
91664fe
Compare
Member
Author
|
Rebased! |
stloyd
suggested changes
Apr 27, 2017
| $versionedPath = $this->getVersionStrategy()->applyVersion($path); | ||
|
|
||
| if ($this->isAbsoluteUrl($versionedPath)) { | ||
| if ($this->isAbsoluteUrl($versionedPath) || '/' === substr($versionedPath, 0, 1)) { |
Contributor
There was a problem hiding this comment.
0 === strpos($versionedPath, '/')
stof
reviewed
Apr 27, 2017
| $versionedPath = $this->getVersionStrategy()->applyVersion($path); | ||
|
|
||
| if ($this->isAbsoluteUrl($versionedPath)) { | ||
| if ($this->isAbsoluteUrl($versionedPath) || 0 === strpos($versionedPath, '/')) { |
Member
There was a problem hiding this comment.
using '/' === $versionedPath[0] is even faster (and already used above). We just need to handle the case of the empty string.
strpos still requires looking at the whole string for the common case of needing the base path
Member
Author
|
@stof I should have your implementation in there now :) |
Member
|
Thank you @weaverryan. |
fabpot
added a commit
that referenced
this pull request
Apr 28, 2017
…(weaverryan) This PR was squashed before being merged into the 2.7 branch (closes #22528). Discussion ---------- [Asset] Starting slash should indicate no basePath wanted | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes-ish... and no-ish | New feature? | no | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a **Important** View the second commit for an accurate diff. The first commit just renames some strings in a test for clarity. When we moved `PathPackage` from `Templating` to `Asset`, we actually changed its behavior. Assume that we're deployed under a `/subdir` subdirectory: **Before** `{{ asset('/main.css') }}` would *not* have the base path prefixed -> `/main.css` **After** `{{ asset('/main.css') }}` *does* have the base path prefixed -> `/subdir/main.css` https://github.com/symfony/symfony/blob/3adff11d729ccdfc4eb4b189417ec04491c6eaad/src/Symfony/Component/Templating/Asset/PathPackage.php#L61-L63 This PR simply reverses that, to the *previous* behavior. This *is* a BC break... and also arguably a bug fix :). Interestingly, when we changed the behavior the first time (i.e. broke BC), I don't think that anyone noticed. It should only affect users deployed under a subdirectory. Why do I care? I'm using the new `JsonManifestVersionStrategy` with a library that is outputting paths that *already* include my subdirectory: ```json { "build/main.css": "/subdir/build/main.abc123.css" } ``` So, I do not want Symfony to detect the `/subdir` and apply it a second time. Commits ------- 3cc096b [Asset] Starting slash should indicate no basePath wanted
This was referenced May 1, 2017
Merged
Merged
Merged
fabpot
added a commit
to silexphp/Silex
that referenced
this pull request
May 2, 2017
This PR was merged into the 2.1.x-dev branch. Discussion ---------- Fix AssetServiceProviderTest::testGenerateAssetUrl The test broke when symfony/symfony#22528 got merged. I added one check without the starting slash (the returned path should be relative to `base_path`) and modified the expectation of the check with the starting slash (the returned path should not be relative). ``` There was 1 failure: 1) Silex\Tests\Provider\AssetServiceProviderTest::testGenerateAssetUrl Failed asserting that two strings are equal. --- Expected +++ Actual @@ @@ -'/whatever-makes-sense/foo.css?css2' +'/foo.css?css2' ``` Commits ------- ce8e41b Fix AssetServiceProviderTest::testGenerateAssetUrl
|
BC break in a hotfix version release… hum hum
…And unit tests that mimic a subdirectory setup. 😞 |
Member
|
@ambroisemaupate which is reverting a BC break in a non-major version too |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
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.
Important View the second commit for an accurate diff. The first commit just renames some strings in a test for clarity.
When we moved
PathPackagefromTemplatingtoAsset, we actually changed its behavior. Assume that we're deployed under a/subdirsubdirectory:Before
{{ asset('/main.css') }}would not have the base path prefixed ->/main.cssAfter
{{ asset('/main.css') }}does have the base path prefixed ->/subdir/main.csssymfony/src/Symfony/Component/Templating/Asset/PathPackage.php
Lines 61 to 63 in 3adff11
This PR simply reverses that, to the previous behavior. This is a BC break... and also arguably a bug fix :). Interestingly, when we changed the behavior the first time (i.e. broke BC), I don't think that anyone noticed. It should only affect users deployed under a subdirectory.
Why do I care? I'm using the new
JsonManifestVersionStrategywith a library that is outputting paths that already include my subdirectory:{ "build/main.css": "/subdir/build/main.abc123.css" }So, I do not want Symfony to detect the
/subdirand apply it a second time.