Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[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

Closed
wants to merge 4 commits into from

Conversation

weaverryan
Copy link
Member

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

if ('/' !== substr($url, 0, 1)) {
$url = $this->basePath.$url;
}

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:

{
    "build/main.css": "/subdir/build/main.abc123.css"
}

So, I do not want Symfony to detect the /subdir and apply it a second time.

@stof
Copy link
Member

stof commented Apr 26, 2017

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.

Copy link
Member

@javiereguiluz javiereguiluz left a 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
Copy link
Member

rebase needed

@fabpot
Copy link
Member

fabpot commented Apr 26, 2017

@weaverryan Can you rebase?

@weaverryan weaverryan force-pushed the fix-base-path-slash branch from 18d7304 to 91664fe Compare April 26, 2017 21:11
@weaverryan
Copy link
Member Author

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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 === strpos($versionedPath, '/')

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Apr 27, 2017
@@ -58,7 +58,7 @@ public function getUrl($path)

$versionedPath = $this->getVersionStrategy()->applyVersion($path);

if ($this->isAbsoluteUrl($versionedPath)) {
if ($this->isAbsoluteUrl($versionedPath) || 0 === strpos($versionedPath, '/')) {
Copy link
Member

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
Copy link
Member Author

@stof I should have your implementation in there now :)

@fabpot
Copy link
Member

fabpot commented Apr 28, 2017

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
@fabpot fabpot closed this Apr 28, 2017
@weaverryan weaverryan deleted the fix-base-path-slash branch April 29, 2017 03:03
This was referenced May 1, 2017
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
@ambroisemaupate
Copy link

BC break in a hotfix version release… hum hum

It should only affect users deployed under a subdirectory.

…And unit tests that mimic a subdirectory setup. 😞

roadiz/roadiz#257

@stof
Copy link
Member

stof commented May 5, 2017

@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.