-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Asset] Starting slash should indicate no basePath wanted #22528
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
Conversation
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. |
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.
👍
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.
rebase needed |
@weaverryan Can you rebase? |
18d7304
to
91664fe
Compare
Rebased! |
@@ -58,7 +58,7 @@ public function getUrl($path) | ||
|
||
$versionedPath = $this->getVersionStrategy()->applyVersion($path); | ||
|
||
if ($this->isAbsoluteUrl($versionedPath)) { | ||
if ($this->isAbsoluteUrl($versionedPath) || '/' === substr($versionedPath, 0, 1)) { |
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.
0 === strpos($versionedPath, '/')
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.
👍
@@ -58,7 +58,7 @@ public function getUrl($path) | ||
|
||
$versionedPath = $this->getVersionStrategy()->applyVersion($path); | ||
|
||
if ($this->isAbsoluteUrl($versionedPath)) { | ||
if ($this->isAbsoluteUrl($versionedPath) || 0 === strpos($versionedPath, '/')) { |
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.
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
@stof I should have your implementation in there now :) |
Thank you @weaverryan. |
…(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 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. 😞 |
@ambroisemaupate which is reverting a BC break in a non-major version too |
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
fromTemplating
toAsset
, 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
symfony/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
JsonManifestVersionStrategy
with a library that is outputting paths that already include my subdirectory:So, I do not want Symfony to detect the
/subdir
and apply it a second time.