Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
stof commentedApr 26, 2017
thus, it only affects users deploying in a subdirectoryand using leading slashes. And the recommendation was to avoid leading slashes precisely to allow deploying in subdirectories easily. |
javiereguiluz left a comment
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.
nicolas-grekas commentedApr 26, 2017
rebase needed |
fabpot commentedApr 26, 2017
@weaverryan Can you rebase? |
18d7304 to91664feCompareweaverryan commentedApr 26, 2017
Rebased! |
| $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.
👍
| $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
weaverryan commentedApr 27, 2017
@stof I should have your implementation in there now :) |
fabpot commentedApr 28, 2017
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-L63This 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::testGenerateAssetUrlThe test broke whensymfony/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::testGenerateAssetUrlFailed asserting that two strings are equal.--- Expected+++ Actual@@ @@-'/whatever-makes-sense/foo.css?css2'+'/foo.css?css2'```Commits-------ce8e41b Fix AssetServiceProviderTest::testGenerateAssetUrl
ambroisemaupate commentedMay 5, 2017
BC break in a hotfix version release… hum hum
…And unit tests that mimic a subdirectory setup. 😞 |
stof commentedMay 5, 2017
@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
PathPackagefromTemplatingtoAsset, we actually changed its behavior. Assume that we're deployed under a/subdirsubdirectory:Before
{{ asset('/main.css') }}wouldnot 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 in3adff11
This PR simply reverses that, to theprevious behavior. Thisis 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 thatalready 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.