[2.2] [Routing] add support for path-relative URL generation #3958
Conversation
|
|
||
| /** | ||
| * Calculate a relative target path based on the source path. | ||
| * Only the URIs path component (no schema, hostname etc.) is relavent and must be given, starting with a slash. |
|
@Tobion For completeness, you should add the option to the |
|
@jalliot adding the option to |
| $scheme = $this->context->getScheme(); | ||
| if (isset($requirements['_scheme']) && ($req = strtolower($requirements['_scheme'])) && $scheme != $req) { | ||
| $scheme = $relative ? '' : $this->context->getScheme(); | ||
| if (isset($requirements['_scheme']) && ($req = strtolower($requirements['_scheme'])) && $scheme !== $req) { |
There was a problem hiding this comment.
This seems wrong. If there is a scheme requirement, you will always detect it as missmatching here
|
@stof I guess jalliot meant we could then generate scheme-relative URLs with |
|
The $relative parameter I added besides the existing $absolute parameter of the |
|
ping @schmittjoh considering your https://github.com/schmittjoh/JMSI18nRoutingBundle/blob/master/Router/I18nRouter.php would need a tiny change |
|
Can you elaborate the necessary change? |
|
This PR changes the signature of |
|
If I understand correctly, the old parameter still works, no? edit: Ah, ok I see what you mean now. |
|
Yeah the old parameter still works but $absolute would also evaluate to true (a string) in your case for non-absolute URLs, i.e. paths. |
| { | ||
| // for BC reasons: $referenceType was a Boolean called $absolute in Symfony 2.0 and only supported two reference types |
There was a problem hiding this comment.
Maybe extract this logic into a static method, so it can be re-used by other implementations which want to be BC.
There was a problem hiding this comment.
Hm I don't think any implementation would need it. The generation will work as before. Custom implemention might only break if they work with the parameter as you did. But then they won't need this conversion. You won't need it either.
There was a problem hiding this comment.
If someone passes "false/true" to the generate function, I would need to convert that in I18nRouter, no?
There was a problem hiding this comment.
Ok I see what you mean.
|
ping @fabpot |
|
Let's discuss that feature for 2.2. |
|
What are your objections against it? It's already implemented, it works and it adds support for things that are part of a web standard. The BC break is tiny at the moment (almost nobody is affected) because the core UrlGenerator works as before. But if we waited for 2.2 it will be much harder to make the transition because 2.1 is LTS. So I think is makes sense to add it now. Furthermore it makes it much more future-proof as custom generators can more easiliy add support for other link types like CURIE. At the moment a Boolean for absolute URLs is simply too limited and also somehow inconsistent because $absolute = false stands for an absolute path. You see the awkwardness in this naming. Btw, I added a note in the changelog. And I will add documentation of this feature in symfony-docs once this is merged. |
|
nobody has ever said that 2.1 would be LTS. Actually, I think we are going to wait for 2.3 for LTS. |
|
Well what I meant is, the longer we wait with this, the harder to apply it. I'd like to know your reason to wait for 2.2. Not enough time to review it, or afraid of breaking something, or marketing for 2.2? |
|
@Tobion the issue is that merging new features forces to postpone the release so that it is tested by enough devs first to be sure there is no blocking bug in it. Big changes cannot be merged when we are hunting the remaining bugs to be able to release. |
|
Considering the changes that have been made to the Form component, and are still being made, I think this is in comparison to that a fairly minor change. Maybe a clearer guideline on the release process, or the direction would help, and avoid confusion, or wrong expectations on contributors' part. |
| * is still possible. | ||
| */ | ||
| const ABSOLUTE_URL = 'url'; | ||
| const ABSOLUTE_PATH = 'path'; |
There was a problem hiding this comment.
I could make ABSOLUTE_URL = true and ABSOLUTE_PATH = false, so there would be no BC break at all and the conversion would not be necessary. The disadvantage is that the referenceType param could be both a boolean and a string which doesnt look that nice from API point of view.
There was a problem hiding this comment.
Hm since most people simply check for Boolean with if ($absolute) it would also evaluate to true for the string values. So this change might not worth it, as people who implemented their own generator need to be aware of the new values anyway. It would simply generate an absolute url just as before and the new values would simply have no effect for them. So it makes it fully BC.
There was a problem hiding this comment.
Implemented this change, so there is not BC break at all anymore.
|
@fabpot this is ready. So if you agree with it, I would create a documentation PR. |
| @@ -39,5 +53,5 @@ | |||
| * | |||
| * @api | |||
| */ | |||
| public function generate($name, $parameters = array(), $absolute = false); | |||
| public function generate($name, $parameters = array(), $referenceType = self::ABSOLUTE_PATH); | |||
There was a problem hiding this comment.
Besides the new features that this changed function signature makes possible, I also think it's much more expressive. The true/false param didn't say much to a reader. But using the constants, it's very clear what it generates.
|
@fabpot what do you think about this PR ? |
|
This feels like it's overloading the generate() method to do double duty: One, make a URl based on a route. Two, make a URI based on a URI snippet. Those are two separate operations. Why not just add a second method that does the second operation and avoid the conditionals? (We're likely to do that in Drupal for our own generator as well.) |
|
@Crell: No, you must have misunderstood something. The generate method still only generates a URI based on a route. The returned URI reference can now also be a relative path and a network path. Thats all. |
This PR was merged into the master branch. Commits ------- 17f51a1 Merge pull request #6 from Tobion/hostname-routes e120a7a fix API of RouteCollection 26e5684 some type fixes 514e27a [Routing] fix PhpMatcherDumper that returned numeric-indexed params that are returned besides named placeholders by preg_match 7ed3013 switch to array_replace instead of array_merge 94ec653 removed irrelevant string case in XmlFileLoader 9ffe3de synchronize the fixtures in different formats and fix default for numeric requirement 6cd3457 fixed CS 8366b8a [Routing] fixed validity check for hostname params in UrlGenerator a8ce621 [Routing] added support for hostname in the apache matcher dumper 562174a [Routing] fixed indentation of dumped collections 1489021 fixed CS a270458 [Routing] added some more unit tests 153fcf2 [Routing] added some unit tests for the PHP loader 68da6ad [Routing] added support for hostname in the XML loader 3dfca47 [Routing] added some unit tests for the YAML loader 92f9c15 [Routing] changed CompiledRoute signature to be more consistent d91e5a2 [Routing] fixed Route annotation for hostname (should be hostname_pattern instead of hostnamePattern) 62de881 [Routing] clarified a variable content 11b4378 [Routing] added hostname support in UrlMatcher fc015d5 [Routing] fixed route generation with a hostname pattern when the hostname is the same as the current one (no need to force the generated URL to be absolute) 462999d [Routing] display hostname pattern in router:debug output 805806a [Routing] added hostname matching support to UrlGenerator 7a15e00 [Routing] added hostname matching support to AnnotationClassLoader cab450c [Routing] added hostname matching support to YamlFileLoader 85d11af [Routing] added hostname matching support to PhpMatcherDumper 402359b [Routing] added hostname matching support to RouteCompiler add3658 [Routing] added hostname matching support to Route and RouteCollection 23feb37 [Routing] added hostname matching support to CompiledRoute Discussion ---------- [2.2][Routing] hostname pattern for routes Bug fix: no Feature addition: yes Fixes the following tickets: #1762, #3276 Backwards compatibility break: no Symfony2 tests pass: yes This adds a hostname_pattern property to routes. It works like the pattern property (hostname_pattern can have variables, requirements, etc). The hostname_pattern property can be set on both routes and route collections. Yaml example: ``` yaml # Setting the hostname_pattern for a whole collection of routes AcmeBundle: resource: "@AcmeBundle/Controller/" type: annotation prefix: / hostname_pattern: {locale}.example.com requirements: locale: en|fr # Setting the hostname_pattern for single route some_route: pattern: /hello/{name} hostname_pattern: {locale}.example.com requirements: locale: en|fr name: \w+ defaults: _controller: Foo:bar:baz ``` Annotations example: ``` php <?php /** * Inherits requirements and hostname pattern from the collection * @route("/foo") */ public function fooAction(); /** * Set a specific hostnamePattern for this route only * @route("/foo", hostnamePattern="{_locale}.example.com", requirements={"_locale="fr|en"}) */ public function fooAction(); ``` Performance: Consecutive routes with the same hostname pattern are grouped, and a single test is made against the hostname for this group, so the overhead is very low: ``` @route("/foo", hostnamePattern="a.example.com") @route("/bar", hostnamePattern="a.example.com") @route("/baz", hostnamePattern="b.example.com") ``` is compiled like this: ``` if (hostname matches a.example.com) { // test route "/foo" // test route "/bar" } if (hostname matches b.example.com) { // test route "/baz" } ``` The PR also tries harder to optimize routes sharing the same prefix: ``` @route("/cafe") @route("/cacao") @route("/coca") ``` is compiled like this: ``` if (url starts with /c) { if (url starts with /ca) { // test route "/cafe" // test route "/cacao" } // test route "/coca" } ``` --------------------------------------------------------------------------- by Koc at 2012-02-16T14:14:19Z Interesting. Have you looked at #3057, #3002? Killer feature of #3057 : multiple hostnames per route. --------------------------------------------------------------------------- by arnaud-lb at 2012-02-16T14:21:28Z @Koc yes, the main difference is that this PR allows variables in the hostname pattern, with requirements, etc just like the path pattern. The other PRs use a `_host` requirement, which works like the `_method` requirement (takes a list of allowed hostnames separated by `|`). > Killer feature of #3057 : multiple hostnames per route. If you have multiple tlds you can easily do it like this: ``` yaml hostbased_route: pattern: / hostname_pattern: symfony.{tld} requirements: tld: org|com ``` Or with completely different domain names: ``` yaml hostbased_route: pattern: / hostname_pattern: {domain} requirements: domain: example\.com|symfony\.com ``` Requirements allow DIC %parameters%, so you can also put you domains in your config.yml. --------------------------------------------------------------------------- by Koc at 2012-02-16T15:52:16Z wow, nice! So looks like this PR closes my #3276 ticket? --------------------------------------------------------------------------- by arnaud-lb at 2012-02-16T15:53:55Z Yes, apparently :) --------------------------------------------------------------------------- by Koc at 2012-02-16T15:56:53Z I cann't find method `ParameterBag::resolveValue` calling in this PR, like here https://github.com/symfony/symfony/pull/3316/files --------------------------------------------------------------------------- by arnaud-lb at 2012-02-16T16:03:48Z I think it's in core already --------------------------------------------------------------------------- by Koc at 2012-02-16T16:11:38Z looks like yes https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Routing/Router.php#L81 --------------------------------------------------------------------------- by dlsniper at 2012-02-16T19:37:57Z This PR looks great, it's something like this I've been waiting for. I know @fabpot said he's working on something similar but I think if he agrees with this it could be a great addition to the core. @fabpot , @stof any objections about this PR if gets fully done? --------------------------------------------------------------------------- by stof at 2012-02-16T20:00:21Z Well, we already have 2 other implementations for this stuff in the PRs. @fabpot please take time to look at them --------------------------------------------------------------------------- by stof at 2012-02-16T20:03:17Z This one is absolutely not tested and seems to break the existing tests according to the description. So it cannot be reviewed as is. --------------------------------------------------------------------------- by dlsniper at 2012-02-16T22:00:24Z @stof I understand it's a WIP but the other PRs where ignored as well and like you've said, there's a bunch of PRs already on this issue all doing a thing or another. So an early feedback on this, or any other, could lead it to the right path in order to finally solve this issue. --------------------------------------------------------------------------- by arnaud-lb at 2012-02-17T23:57:28Z Added tests; others are passing now --------------------------------------------------------------------------- by arnaud-lb at 2012-02-22T21:10:20Z I'm going to add support for the Apache dumper and the XML loader; does this PR have a chance to be merged ? cc @fabpot @stof --------------------------------------------------------------------------- by stof at 2012-02-22T22:05:23Z @arnaud-lb We need to wait @fabpot's mind about the way he prefers to implement it to know which one can be merged. --------------------------------------------------------------------------- by IjinPL at 2012-02-27T02:01:57Z Forked @arnaud-lb *hostname_pattern* to add XML parasing support. --------------------------------------------------------------------------- by stof at 2012-04-03T23:59:12Z @arnaud-lb Please rebase your branch. It conflicts with master because of the move of the tests @fabpot @vicb ping --------------------------------------------------------------------------- by dlsniper at 2012-04-13T19:52:23Z Hi, If @arnaud-lb won't be able to rebase this I could help with some work on this but there's still the problem of actually choosing the right PR(s) for this issue. @blogsh says in his last commit that this PR is a bit better in his opinion but @fabpot needs to decide in the end. --------------------------------------------------------------------------- by arnaud-lb at 2012-04-14T17:26:55Z @stof rebased --------------------------------------------------------------------------- by nomack84 at 2012-04-20T13:01:00Z @fabpot Any final word about this pull request? It would be nice to have this feature ready for 2.1. --------------------------------------------------------------------------- by asm89 at 2012-04-24T21:27:50Z Using the `{_locale}` placeholder in the host would set the locale for the request just like it does now? Another thing I'm wondering is how/if it should be possible to set the hostname pattern for all your routes, or at least when importing routes? Otherwise you'll end up repeating the same host pattern over and over again. I think this is also important when importing routes from third party bundles. --------------------------------------------------------------------------- by fabpot at 2012-04-25T01:17:51Z I'm reviewing this PR and I'm going to make some modifications. I will send a PR to @arnaud-lb soon. --------------------------------------------------------------------------- by fabpot at 2012-04-25T03:10:18Z I've sent a PR to @arnaud-lb arnaud-lb#3 that fixes some minor bugs and add support in more classes. --------------------------------------------------------------------------- by fabpot at 2012-04-25T03:12:52Z @asm89: Placeholders in the hostname are managed in the same way as the ones from the URL pattern. You can set a hostname pattern for a collection (like the prefix for URL patterns). --------------------------------------------------------------------------- by Tobion at 2012-04-25T09:31:19Z I think we need to change the contents of $variables, $tokens, and $hostnameTokens in the CompiledRoute. They contain redundant information and the content structure of these variables ist not documentation in any way. If we remove duplicated content and put it in a (single) well defined variable, it would also reduce the information that need to be saved in the generated class by the UrlGeneratorDumper. --------------------------------------------------------------------------- by arnaud-lb at 2012-04-26T08:54:21Z @fabpot thanks :) I've merged it --------------------------------------------------------------------------- by stof at 2012-04-26T12:08:40Z A rebase is needed --------------------------------------------------------------------------- by fabpot at 2012-04-26T13:28:08Z no need to rebase, I will resolve the conflicts when merging. I've still have some minor changes to do before merging though. Anyone willing to have a look at implementing the Apache dumper part? --------------------------------------------------------------------------- by Tobion at 2012-04-26T14:59:00Z @fabpot you want to merge this for 2.1 although it introduces big changes that need extensive review and testing? But #3958 is not considered for 2.1? I thought we are in some sort of feature freeze for the components in order to not postpone the release. --------------------------------------------------------------------------- by fabpot at 2012-04-26T17:21:09Z @Tobion: I never said it will be in 2.1. The plan is to create a 2.1 branch soon so that we can continue working on 2.2. --------------------------------------------------------------------------- by Koc at 2012-04-26T19:46:43Z https://twitter.com/#!/fabpot/status/178502663690915840
|
@fabpot this is ready. It is fully BC! I also improved phpdoc considerably. |
| { | ||
| $variables = array_flip($variables); | ||
| $mergedParams = array_replace($defaults, $this->context->getParameters(), $parameters); | ||
|
|
||
| // all params must be given | ||
| if ($diff = array_diff_key($variables, $mergedParams)) { | ||
| throw new MissingMandatoryParametersException(sprintf('The "%s" route has some missing mandatory parameters ("%s").', $name, implode('", "', array_keys($diff)))); | ||
| throw new MissingMandatoryParametersException(sprintf('Some mandatory parameters are missing ("%s") to generate a URL for route "%s".', implode('", "', array_keys($diff)), $name)); |
There was a problem hiding this comment.
The description was bad because the route itself does not miss mandatory params which misleadingly sounds like the route definition was wrong.
There was a problem hiding this comment.
shouldn't the parameter names be placed after Some mandatory parameters instead of after are missing ?
There was a problem hiding this comment.
I dont think so because it only lists the params that are missing. If its after mandatory params it would suggest that it list all required params.
|
@fabpot Do you want me to write documentation for it? I would also be interested to write about the new features of the routing component in general. I wanted to do that anyway and it would probably be a good fit for your "new in symfony" articles. |
|
Im' going to review this PR in the next coming days. And to answer your second question, more documentation or better documentation is always a good thing, so go for it. |
|
@fabpot ping. I added changelog entries. |
…mprove language of exception message
This PR was merged into the master branch. Commits ------- 6703fb5 added changelog entries 1997e2e fix phpdoc of UrlGeneratorInterface that missed some exceptions and improve language of exception message f0415ed [Routing] made reference type fully BC and improved phpdoc considerably 7db07d9 [Routing] added tests for generating relative paths and network paths 75f59eb [Routing] add support for path-relative and scheme-relative URL generation Discussion ---------- [2.2] [Routing] add support for path-relative URL generation Tests pass: yes Feature addition: yes BC break: <del>tiny (see below)</del> NO deprecations: NO At the moment the Routing component only supports absolute and domain-relative URLs, e.g. `http://example.org/user-slug/article-slug/comments` and `/user-slug/article-slug/comments`. But there are two link types missing: schema-relative URLs and path-relative URLs. schema-relative: e.g. `//example.org/user-slug/article-slug/comments` path-relative: e.g. `comments`. Both of them would now be possible with this PR. I think it closes a huge gap in the Routing component. Use cases are pretty common. Schema-relative URLs are for example used when you want to include assets (scripts, images etc) in a secured website with HTTPS. Path-relative URLs are the only option when you want to generate static files (e.g. documentation) that can be downloaded as an HTML archive. Such use-cases are currently not possible with symfony. The calculation of the relative path based on the request path and target path is hightly unit tested. So it is really equivalent. I found several implemenations on the internet but none of them worked in all cases. Mine is pretty short and works. I also added an optional parameter to the twig `path` function, so this feature can also be used in twig templates. Ref: This implements path-relative URLs as suggested in #3908. <del>[BC BREAK] The signature of UrlGeneratorInterface::generate changed to support scheme-relative and path-relative URLs. The core UrlGenerator is BC and does not break anything, but users who implemented their own UrlGenerator need to be aware of this change. See UrlGenerator::convertReferenceType.</del> --------------------------------------------------------------------------- by jalliot at 2012-04-16T09:56:56Z @Tobion For completeness, you should add the option to the `url` and `asset` twig functions/template helpers. --------------------------------------------------------------------------- by stof at 2012-04-16T10:46:06Z @jalliot adding the option to ``url`` does not make any sense. The difference between ``path`` and ``url`` is that ``path`` generates a path and ``url`` generates an absolute url (thus including the scheme and the hostname) --------------------------------------------------------------------------- by Tobion at 2012-04-16T12:27:49Z @stof I guess jalliot meant we could then generate scheme-relative URLs with `url`. Otherwise this would have no equivalent in twig. --------------------------------------------------------------------------- by jalliot at 2012-04-16T12:34:08Z @stof Yep I meant what @Tobion said :) --------------------------------------------------------------------------- by Tobion at 2012-04-18T11:57:04Z The $relative parameter I added besides the existing $absolute parameter of the `->generate` method was not clear enough. So I merged those into a different parameter `referenceType`. I adjusted all parts of symfony to use the new signature. And also made the default `UrlGenerator` implementation BC with the old style. So almost nobody will recognize a change. The only BC break would be for somebody who implemented his own `UrlGenerator` and did not call the parent default generator. Using `referenceType` instead of a simple Boolean is much more flexible. It will for example allow a custom generator to support a new reference type like http://en.wikipedia.org/wiki/CURIE --------------------------------------------------------------------------- by Tobion at 2012-04-18T13:34:58Z ping @schmittjoh considering your https://github.com/schmittjoh/JMSI18nRoutingBundle/blob/master/Router/I18nRouter.php would need a tiny change --------------------------------------------------------------------------- by schmittjoh at 2012-04-18T13:37:39Z Can you elaborate the necessary change? --------------------------------------------------------------------------- by Tobion at 2012-04-18T13:51:10Z This PR changes the signature of `generate` to be able to generate path-relative and scheme-relative URLs. So it needs to be `public function generate($name, $parameters = array(), $referenceType = self::ABSOLUTE_PATH)` and your implementation would need to change `if ($absolute && $this->hostMap) {` to `if (self::ABSOLUTE_URL === $referenceType && $this->hostMap) {` I can do a PR if this gets merged. --------------------------------------------------------------------------- by schmittjoh at 2012-04-18T13:52:14Z If I understand correctly, the old parameter still works, no? edit: Ah, ok I see what you mean now. --------------------------------------------------------------------------- by Tobion at 2012-04-18T13:56:33Z Yeah the old parameter still works but $absolute would also evaluate to true (a string) in your case for non-absolute URLs, i.e. paths. --------------------------------------------------------------------------- by Tobion at 2012-04-19T21:09:46Z ping @fabpot --------------------------------------------------------------------------- by fabpot at 2012-04-20T04:30:18Z Let's discuss that feature for 2.2. --------------------------------------------------------------------------- by Tobion at 2012-04-20T10:40:59Z What are your objections against it? It's already implemented, it works and it adds support for things that are part of a web standard. The BC break is tiny at the moment (almost nobody is affected) because the core UrlGenerator works as before. But if we waited for 2.2 it will be much harder to make the transition because 2.1 is LTS. So I think is makes sense to add it now. Furthermore it makes it much more future-proof as custom generators can more easiliy add support for other link types like CURIE. At the moment a Boolean for absolute URLs is simply too limited and also somehow inconsistent because $absolute = false stands for an absolute path. You see the awkwardness in this naming. Btw, I added a note in the changelog. And I will add documentation of this feature in symfony-docs once this is merged. --------------------------------------------------------------------------- by fabpot at 2012-04-20T12:14:32Z nobody has ever said that 2.1 would be LTS. Actually, I think we are going to wait for 2.3 for LTS. --------------------------------------------------------------------------- by Tobion at 2012-04-20T12:27:18Z Well what I meant is, the longer we wait with this, the harder to apply it. In 04ac1fd you modified `generate` signature for better extensibility that is not even made use of. I think changing `$abolute` param goes in the same direction and has direct use. I'd like to know your reason to wait for 2.2. Not enough time to review it, or afraid of breaking something, or marketing for 2.2? --------------------------------------------------------------------------- by stof at 2012-04-20T16:28:27Z @Tobion the issue is that merging new features forces to postpone the release so that it is tested by enough devs first to be sure there is no blocking bug in it. Big changes cannot be merged when we are hunting the remaining bugs to be able to release. --------------------------------------------------------------------------- by schmittjoh at 2012-04-20T16:42:11Z Considering the changes that have been made to the Form component, and are still being made, I think this is in comparison to that a fairly minor change. Maybe a clearer guideline on the release process, or the direction would help, and avoid confusion, or wrong expectations on contributors' part. --------------------------------------------------------------------------- by Tobion at 2012-10-05T13:52:11Z @fabpot this is ready. So if you agree with it, I would create a documentation PR. --------------------------------------------------------------------------- by stof at 2012-10-13T16:09:47Z @fabpot what do you think about this PR ? --------------------------------------------------------------------------- by Crell at 2012-11-01T16:05:01Z This feels like it's overloading the generate() method to do double duty: One, make a URl based on a route. Two, make a URI based on a URI snippet. Those are two separate operations. Why not just add a second method that does the second operation and avoid the conditionals? (We're likely to do that in Drupal for our own generator as well.) --------------------------------------------------------------------------- by Tobion at 2012-11-01T16:38:39Z @Crell: No, you must have misunderstood something. The generate method still only generates a URI based on a route. The returned URI reference can now also be a relative path and a network path. Thats all. --------------------------------------------------------------------------- by Tobion at 2012-12-13T18:30:28Z @fabpot this is ready. It is fully BC! I also improved phpdoc considerably. --------------------------------------------------------------------------- by Tobion at 2012-12-14T20:51:38Z @fabpot Do you want me to write documentation for it? I would also be interested to write about the new features of the routing component in general. I wanted to do that anyway and it would probably be a good fit for your "new in symfony" articles. --------------------------------------------------------------------------- by fabpot at 2012-12-14T20:58:16Z Im' going to review this PR in the next coming days. And to answer your second question, more documentation or better documentation is always a good thing, so go for it. --------------------------------------------------------------------------- by Tobion at 2013-01-02T21:50:20Z @fabpot ping. I added changelog entries.
|
Merged!. Can you submit documentation changes to symfony/symfony-docs? Thanks |
|
@fabpot For some reason, you merged a slightly older version. I rebased shortly before you merged because Github told me, the PR cannot be merged automatically. |
|
Merged. |
|
Is everything ok? Or do we need to change something? |
|
It was just a phpdoc fix that is missing. I will resubmit it in another PR. But the real problem is #6634 |
Tests pass: yes
Feature addition: yes
BC break:
tiny (see below)NOdeprecations: NO
At the moment the Routing component only supports absolute and domain-relative URLs, e.g.
http://example.org/user-slug/article-slug/commentsand/user-slug/article-slug/comments.But there are two link types missing: schema-relative URLs and path-relative URLs.
schema-relative: e.g.
//example.org/user-slug/article-slug/commentspath-relative: e.g.
comments.Both of them would now be possible with this PR. I think it closes a huge gap in the Routing component.
Use cases are pretty common. Schema-relative URLs are for example used when you want to include assets (scripts, images etc) in a secured website with HTTPS. Path-relative URLs are the only option when you want to generate static files (e.g. documentation) that can be downloaded as an HTML archive. Such use-cases are currently not possible with symfony.
The calculation of the relative path based on the request path and target path is hightly unit tested. So it is really equivalent. I found several implemenations on the internet but none of them worked in all cases. Mine is pretty short and works.
I also added an optional parameter to the twig
pathfunction, so this feature can also be used in twig templates.Ref: This implements path-relative URLs as suggested in #3908.
[BC BREAK] The signature of UrlGeneratorInterface::generate changed to support scheme-relative and path-relative URLs. The core UrlGenerator is BC and does not break anything, but users who implemented their own UrlGenerator need to be aware of this change. See UrlGenerator::convertReferenceType.