[Routing] allow URLs to be generated that include the default param #5410
Conversation
|
@fabpot I'd appreciate if that goes into 2.1 so the old, odd behavior does not manifest in 2.1. If you agree, I'll write a note for the changelog and upgrade. |
|
👍 |
|
Btw, this PR makes it more consistent as currently a param that has the same value as a path variable is left out, but an extra param in the query string is not left out when it's the same as a default. |
|
@Tobion Being in RC phase means that no changes are done anymore expect to fix bugs. So this cannot go in 2.1 |
|
Can you rebase on master? Also, can you add a note in the CHANGELOG and update the docs accordingly? Thanks. |
This can be achieved by explicitly passing the parameter. Previously it just ignored the parameter that equals the default value. You can still omit optional variables by not passing it at all or passing null as param value.
|
@fabpot done |
|
I've thought about this a bit more. I understand where you come from, but I think it is not the right way to fix it. Let me explain why. Given the route Furthermore, you now have two valid URLs for the same content ( |
|
The controller should set the format variable to null when the URL should rely on the route default or set it to e.g. Let me discuss your other argument. These two URLs do not necessarily have the same content. For example So this change is necessary for content negotiation and semantic reasons (URI vs. URL). |
|
|
||
| ### Routing | ||
|
|
||
| * Added possibility to generate URLs that include the default param, whereas before a param |
There was a problem hiding this comment.
an upgrade instruction starting with Added looks weird to me.
There was a problem hiding this comment.
i can reformulate it when fabpot changed his mind about this pr
|
ping |
|
An example why the current way is problematic, that I just saw in a project: You have 2 simple routes like
When you now generate a URL for the second route with explicitely |
|
Another solution I have in mind is only enabling this behavior only when a special parameter is given. |
|
Using a special parameter to control the generation is wrong IMO. It would make the API inconsistent (we are not using a special parameter to choose between relative path, absolute path or absolute urls). So -1 from me. |
|
@stof. I don't think it's inconsistent and makes perfect sense. The $referenceType parameter is at a different level. It's part of the URI spec which defines how URIs can be represented. But the parameter I'm talking about here, to control whether default should be included, is just for internal control. So it's ok to distinguish them. |
|
@Tobion but then, you forbid using some names for the route placeholders, which does not make sense IMO (and is a BC break) |
|
@stof, no because special chars like |
|
@Tobion Using |
|
Huh? It would exactly not break the link creation for the same page. Because it SHOULD not be passed when not explicitly given by the developer. And I agree it's not optimal in the parameter because it's not directly related. But it's the ONLY feasable solution. It's not really similar to |
|
@Tobion If the param was included in the URL previously, recreating the same url with |
|
I already said in the comments of #3965 that it's basically impossible to regenerate the current url with the url generator. This is problematic currently too and has nothing to do with this PR. So let's not depart from the topic. |
|
@Tobion With the generator only, indeed. But with the request attributes, it is currently possible. |
|
@stof no excatly not. Let me explain again: |
|
btw, I just see that you also cannot regenerate the url with the |
|
+1 for PR, current behavior causes problems in cases where default _format is set, as example - BazingaExposeTranslationBundle. Can't wait till it's fixed. :) |
|
Could we imagine to set some parameters as always present in the URL? Let take the following example: profile:
path: /profile/{name}/{_locale}
defaults: { _locale: en, name: me }I'd like to get |
|
@jeff In your sample you use About the I am globally +1 with the idea behind the PR. |
|
@lyrixx thanks for the typo. I chose |
|
Let's summarize the above discussion and make a decision (and finish this PR): The ProblemAssume a route
There should be a way to get The Solutions
|
|
I tend to prefer option 3 as it's the only proposal that makes the choice in the definition of the route where it belongs; that also avoids any possibility to generate both forms (and I really dislike option 2) |
|
We could change the |
|
Btw, I agree with @fabpot :) @javiereguiluz the character indentifies that the parameter is not optional (parameters with a default value are optional by default). So I think |
|
The use-case for me was exactly to be able to generate BOTH forms of URIs as detailed in the original ticket. So option 3 would not solve that problem at all. And option 3 would not be useful at all since I can simply remove the default value if I want to generate a URI with the param (you don't want to have matching without the param but generating with it which would be inconsistent). |
|
I don't understand why one would want to generate different URLs for the same resource, it looks like a bad use case. |
|
For example you want to match This is common practice for example in the semantic web where There are currently workarounds like using two routes or changing the default value of the format to something that does not correspond to an existing format. |
I'm just asking: if you want to redirect ALL URLs to their |
|
👍 |
|
Closing as there is no consensus. |
BC break: yes (even if no test needed to be changed, so the old behavior was probably not even intended as no test for it existed)
Fixes #5180
This can be achieved by explicitly passing the parameter. Previously it just ignored the parameter that equals the default value. You can still omit optional variables by not passing it at all or passing null as param value.
Example:
Given the route
new Route('/index.{_format}', array('_format' => 'html'));Previously generating this route with params
array('_format' => 'html')resulted in/indexwhich means one could not generate/index.htmlat all. This has been changed. You can of course still generate just/indexby passing array('_format' => null) or not passing the param at all.I'm certain this is the best solution to the problem even if it slightly breaks bc. But IMO this is how it is supposed to work. Because when I pass a parameter explicitly, I want it to be included in the URL.