URL manipulations as a Twig extension #13264
Conversation
| * | ||
| * @return string The relative target path | ||
| */ | ||
| public function getRelativeUriForPath($path) |
There was a problem hiding this comment.
This code is exactly the same as the code in UrlGenerator.
There was a problem hiding this comment.
Maybe instead of duplicating that code add new class (same like there is IpUtils class) i.e. UrlUtils and re-use it in places it's needed?
There was a problem hiding this comment.
@stloyd That would be in a new component as Routing and HttpFoundation are independent. IMO, it's not worth it.
There was a problem hiding this comment.
No it does not. As you can see, the requirement is only for dev. Anyway, the code is now slightly different.
| */ | ||
| public function generateAbsoluteUrl($path) | ||
| { | ||
| if (false !== strpos($path, '://') || '//' === substr($path, 0, 2)) { |
There was a problem hiding this comment.
The case of URLs starting with // being unchanged is wrong IMO. If it starts with //, it is not an absolute URL. It is a scheme-relative URL (and scheme relative URLs don't work in non-webmail email clients for instance, as the email is not on an HTTP or HTTPS scheme).
|
This requires tests |
aa4a26d to
a3ca868
Compare
|
Tests added, behavior more well defined now. |
| $last = strlen($prefix) - 1; | ||
| if ($last !== $pos = strrpos($prefix, '/')) { | ||
| $prefix = substr($prefix, 0, $pos).'/'; | ||
| } |
There was a problem hiding this comment.
The logic above should probably be moved to Request::getUriForPath() but that would be a BC break.
|
ping @symfony/deciders What do you think of this new feature? What about the name of the functions? |
a3ca868 to
c9e30a2
Compare
| public function getRelativeUriForPath($path) | ||
| { | ||
| // be sure that we are dealing with an absolute path | ||
| if (!$path || '/' !== $path[0]) { |
There was a problem hiding this comment.
'' === $path to be symmetric with below?
There was a problem hiding this comment.
or better : !isset($path[0])
|
missing composer.json update: ~2.7 required now:
|
d035997 to
364f89f
Compare
|
Last call for comments before merge. |
| * | ||
| * @return string The absolute URL | ||
| * | ||
| * @see Request::getUriForPath() |
There was a problem hiding this comment.
Request should be probably added to the use statements, or full namespace should be used. We've got two Request classes in Symfony. Similar case in the next docblock.
|
👍 |
|
+1 from a OO developer point of view |
364f89f to
0ec852d
Compare
|
@wouterj Can you explain why you think it's not good for DX? |
|
@fabpot I was actually still debating with myself of that point (that's why I didn't say a hard -1). My thoughts:
Can't we add a shortcut to |
|
@wouterj Thanks for your thoughts. I added those functions because I was working on the asset functions, but their usage are much wider than just with asset. As a matter of fact, I think that asking for an absolute URL when using asset is a edge case. |
|
@fabpot there is a common use case for that: rendering an HTML email (AFAIK, this one the main reason to ask for this feature). But we can indeed use the new function in this case |
This PR was merged into the 2.7 branch. Discussion ---------- URL manipulations as a Twig extension | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | symfony/symfony-docs#4805 While working on the new asset component, I realized that the "absolute URL" feature was misplaced and would benefit from being exposed as a Twig function (composition is always a good thing). Then, I wondered if having a Twig function to generate a relative path (like done by the Routing component would also make sense). And here is the corresponding PR. ```jinja {# generate an absolute URL for the given absolute path #} {{ absolute_url('/me.png') }} {# generate a relative path for the given absolute path (based on the current Request) #} {{ relative_path('/foo/me.png') }} {# compose as you see fit #} {{ absolute_url(asset('me.png')) }} ``` As you can see, we require an absolute path for both functions (and we even add the leading slash if it is omitted), not sure if we want to do otherwise. ping @Tobion Commits ------- 0ec852d added a relative_path Twig function ee27ed8 added an absolute_url() Twig function
…tive_path() Twig functions (fabpot) This PR was merged into the master branch. Discussion ---------- added documentation for the new absolute_url() and relative_path() Twig functions | Q | A | ------------- | --- | Doc fix? | no | New docs? | symfony/symfony#13264 | Applies to | 2.7 | Fixed tickets | n/a Commits ------- e4d22f0 added documentation for the new absolute_url() and relative_path() Twig functions
This PR was merged into the 2.7 branch. Discussion ---------- [Asset] added the component | Q | A | ------------- | --- | Bug fix? | yes | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #10973, #11748, #11876, #4883, #12474 | License | MIT | Doc PR | not yet TODO: - [ ] submit documentation PR The current Asset sub-namespace in Templating has several (major) problems: * It does not cover all use cases (see #10973 and #4883 for some example) * It has some design issues (relies on the Request instance and so requires the request scope, very coupled with the PHP templating sub-system, see #11748 and #11876) To decouple this feature and make it reusable in Silex for instance, and to fix the design issues and make it more extensible, I've decided to extract and rework the features provided into a new Asset component. Basically, this component allows the developer to easily manage asset URLs: versioning, paths, and hosts. Both the new and the old asset management features are kept in this PR to avoid breaking BC; the old system is of course deprecated and automatically converted to the new one. Even if the features are quite similar, and besides the flexilibity of the new system, here are some differences: * `PathPackage` always prepend the path (even if the given path starts with `/`). * Usage is stricter (for instance, `PathPackage` requires a basePath to be passed and `UrlPackage` requires that at least on URL is passed). * In the configuration, named packages inherits from the version and version format of the default package by default. * It is not possible to override the version when asking for a URL (instead, you can define your own version strategy implementation -- the use cases explained in #6092 are easily implemented this way). * It's not possible to generate absolute URLs (see #13264 for a better alternative using composition; so using `absolute_url(asset_path('me.png')) should work)`. #10973 was about adding shortcuts for bundles, which is a good idea; but given that you rarely reference built-in or third-party bundle assets and because we now have a one-bundle default approach named AppBundle, the same can be achieved with just a simple piece of configuration with the new assets feature: ```yml framework: assets: packages: app: base_path: /bundles/app/ img: base_path: /bundles/app/images/ ``` Then: ```jinja {{ asset('images/me.png', 'app') }} # /bundles/app/images/me.png {{ asset('me.png', 'img') }} # /bundles/app/images/me.png ``` #12474 discussed the possibility to add a version for absolute URL. It's not possible to do that in a generic way as the version strategy involves both the version and the path, which obviously cannot work when the path is an absolute URL already. Instead, one should use the `asset_version` Twig function to add the version manually. Commits ------- 0750d02 removed usage of the deprecated forms of asset() in the core framework f74a1f2 renamed asset_path() to asset() and added a BC layer 4d0adea [Asset] added a NullContext class d33c41d [Asset] added the component
…weaverryan) This PR was merged into the 2.7 branch. Discussion ---------- Adding a section to emailing showing off absolute_url Hi guys! | Q | A | ------------- | --- | Doc fix? | no | New docs? | no (but follows from symfony/symfony#13264) | Applies to | 2.7+ | Fixed tickets | n/a I thought we should show off `absolute_url` in a place where you will commonly need it. Thanks! Commits ------- 67efb2b minor tweak thanks to Javier 5ab1adf Adding a section to emailing showing off absolute_url
While working on the new asset component, I realized that the "absolute URL" feature was misplaced and would benefit from being exposed as a Twig function (composition is always a good thing). Then, I wondered if having a Twig function to generate a relative path (like done by the Routing component would also make sense). And here is the corresponding PR.
As you can see, we require an absolute path for both functions (and we even add the leading slash if it is omitted), not sure if we want to do otherwise.
ping @Tobion