Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

[Routing] allow URLs to be generated that include the default param #5410

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
Member

Tobion commented Sep 1, 2012

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 /index which means one could not generate /index.html at all. This has been changed. You can of course still generate just /index by 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.

This pull request passes (merged dc843f5b into c0673d7).

Member

Tobion commented Sep 1, 2012

@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.

Contributor

sstok commented Sep 1, 2012

👍

Member

Tobion commented Sep 2, 2012

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.

Member

stof commented Sep 2, 2012

@Tobion Being in RC phase means that no changes are done anymore expect to fix bugs. So this cannot go in 2.1

Owner

fabpot commented Oct 3, 2012

Can you rebase on master? Also, can you add a note in the CHANGELOG and update the docs accordingly? Thanks.

Tobion added some commits Sep 1, 2012

[Routing] allow URLs that include the default param
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.
Member

Tobion commented Oct 3, 2012

@fabpot done

Owner

fabpot commented Oct 4, 2012

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 new Route('/index.{_format}', array('_format' => 'html'));, when generating a URL from it, you would probably use something like path('index', {'_format': format}) in your template. So, the format is the value of the format variable. In this case, there no way to remove the variable to keep the previous behavior. The only option would be to set the format to null, which is a duplication of the logic (as the default value is already defined in the route definition).

Furthermore, you now have two valid URLs for the same content (/index.html and /index), with no way to standardize on one URL as each URL generation can possibly use one or the other.

Member

Tobion commented Oct 4, 2012

The controller should set the format variable to null when the URL should rely on the route default or set it to e.g. html when the URL should be explicit. This is no duplication of logic but much more explicit and less magic. The old code heavily relies on the route default for that and ignores what was passed as parameter. Say, the route default is changed at some time (the format to 'json'), then all the URLs are changed suddenly.
path('index', {'_format': 'html'}) becomes /index.html (previously /index) and path('index', {'_format': 'json'}) results in /index (previously /index.json). This is really bad for caching and SEO etc. Image this example with the locale.

Let me discuss your other argument. These two URLs do not necessarily have the same content. For example /index can be used as URL for content negotiating to the correct URL with the format. Then you should send the Content-Location header which is currently not possible without hacking the URL as you cannot generate the index.html with the default included.

So this change is necessary for content negotiation and semantic reasons (URI vs. URL).

@stof stof commented on the diff Oct 13, 2012

UPGRADE-2.2.md
@@ -0,0 +1,11 @@
+UPGRADE FROM 2.1 to 2.2
+=======================
+
+### Routing
+
+ * Added possibility to generate URLs that include the default param, whereas before a param
@stof

stof Oct 13, 2012

Member

an upgrade instruction starting with Added looks weird to me.

@Tobion

Tobion Oct 14, 2012

Member

i can reformulate it when fabpot changed his mind about this pr

Contributor

gimler commented Feb 5, 2013

ping

Member

Tobion commented Feb 5, 2013

An example why the current way is problematic, that I just saw in a project: You have 2 simple routes like

  • / homepage that is for example a language switcher that redirects to the appropriate page depending on the HTTP language accept header
  • /{_locale} with default _locale = en

When you now generate a URL for the second route with explicitely _locale = en, it will still generate / which is not the itended route (in this case it might then redirect to /fr because of browser settings which is not what the user wanted because it changes the language suddenly).

Member

Tobion commented Mar 18, 2013

Another solution I have in mind is only enabling this behavior only when a special parameter is given.
Like $generator->generate('name', array('_format' => 'html', '_with_defaults' => true) -> /index.html.
This way, we have the desired flexibility and at the same time no bc break. The question is how to best name the option (_with_defaults, _explicit, other ideas?).
On the one hand, IMO my original proposal is still better because it fixes a design flaw. On the other hand, I see that for some standard cases it might make things a little more verbose in code.

Member

stof commented Mar 18, 2013

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.

Member

Tobion commented Mar 18, 2013

@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.
Btw, of course one could add a new $options array like UrlGeneratorInterface::generate($name, $parameters, $referenceType, $options) but I think it complicates things extremely without benefit. Because you would need to expose these options to twig etc.
Btw#2, I also would like to introduce another "special" parameter $generator->generate('name', array('#' => 'id') to generate URIs with a hash, which is also a missing piece yet.

Member

stof commented Mar 18, 2013

@Tobion but then, you forbid using some names for the route placeholders, which does not make sense IMO (and is a BC break)

Member

Tobion commented Mar 18, 2013

@stof, no because special chars like # are not allowed as placeholders anyway. So it wouldn't break using the right name. And also _explicit would probably not break for 99.99% of all people because it's unlikely somebody used such a parameter.

Member

stof commented Mar 18, 2013

@Tobion _explicit would still be a BC break.
And it would also break the way to create a link to the same page (useful for locale switchers for instance) with path(app.request.attributes.get('_route'), app.request.attributes.get('_route_params')) as it would not be passed again.

Using # to allow setting the hash is OK for me. It is indeed BC, and it respects the semantic of parameters (the # parameter ends in the generated URI). _explicit would not respect it. It controls how the generator behaves, not what it generates. It is similar to the strict_requirements setting, not to parameters.

Member

Tobion commented Mar 18, 2013

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 strict_requirements because that option is global for all url generations. But here we need the ability to control the behavior for each url generation (which $parameters is somewhat fitting too)
So do you have a better appraoch in mind?

Member

stof commented Mar 18, 2013

@Tobion If the param was included in the URL previously, recreating the same url with _route_params should work. If _explicit has to be passed as a param when generating, it is impossible (as the re-generation will skip the param matching the default even if it was matched originally).

Member

Tobion commented Mar 18, 2013

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.

Member

stof commented Mar 18, 2013

@Tobion With the generator only, indeed. But with the request attributes, it is currently possible.

Member

Tobion commented Mar 18, 2013

@stof no excatly not. Let me explain again: new Route('/index.{_format}', array('_format' => 'html')).
Let's say the current requested URL path is /index.html which matches the above route. How would you regenerate this URL with the request attributes dynamically? It's not possible because you can only genrate /index which as you see, is not the current URL.

Member

Tobion commented Mar 18, 2013

btw, I just see that you also cannot regenerate the url with the _route_params having the controller as a placeholder like /{_controller} because the RouterListener unsets this parameter before saving them und thus it would be missing.

luxemate commented Apr 2, 2013

+1 for PR, current behavior causes problems in cases where default _format is set, as example - BazingaExposeTranslationBundle. Can't wait till it's fixed. :)

Contributor

jfsimon commented Jul 16, 2013

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 {name} parameter always visible in URL, even with the default value. We could have something like path: /profile/{!name}/{_locale}, where the name parameter is marked as required with ! prefix. What do you guys think?

Member

lyrixx commented Jul 19, 2013

@jeff In your sample you use _locale and locale. I guess this is only one var ?

About the !: IMHO this char is a bit weird. Not very explicit. At the first read I think it was for "skip" the attribute as ! is used for negative.

I am globally +1 with the idea behind the PR.

Contributor

jfsimon commented Jul 19, 2013

@lyrixx thanks for the typo. I chose ! by thinking to css !important, but indeed it's usually used as not. Maybe {name!} would be okay?

Member

wouterj commented Jul 27, 2015

Let's summarize the above discussion and make a decision (and finish this PR):

The Problem

Assume a route /index.{_format} with _format set to html as default value. Now, when rendering a template:

Code Result
path(..., { format: 'html' }) /index
path(..., { format: 'json' }) /index.json

There should be a way to get /index.html, there isn't one at the moment.

The Solutions

  1. (current implementation) Generate /index.html when { format: 'html' } is passed and only generate /index when { format: null } is passed
    • If you use { format: format } (which is common), the only way to get /index would be: { format: format === 'html' ? null : format }
    • The application might end up linking to both /index and /index.html, meaning 2 URLs serving the same content
  2. (refs) Add a special parameter _with_defaults to determine if default values should be included in the path
    • Parameters don't seem to be the correct way
    • Inconsistent with relative/absolute URLs for instance
    • BC break, as you can no longer use _with_defaults as route placeholder
  3. (refs) Have a special character in the parameter name to determine if it should be included or not (like /index.{!_locale} or /index.{_locale!}).
Owner

fabpot commented Jul 28, 2015

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)

Member

javiereguiluz commented Jul 28, 2015

We could change the ! character by ? to make it consistent with other parts of Symfony (optional Assetic filters, optional services dependencies, etc.): /index.{?_format}

Member

wouterj commented Jul 28, 2015

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 ! seems like a nice choice, however it can be mixed up with not as noticed by @lyrixx.

Member

Tobion commented Jul 28, 2015

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).

Owner

fabpot commented Jul 28, 2015

I don't understand why one would want to generate different URLs for the same resource, it looks like a bad use case.

Member

Tobion commented Jul 28, 2015

For example you want to match /index but then redirect to /index.html. For redirecting you need to generate this route with the format (otherwise you will end up in an endless loop).

This is common practice for example in the semantic web where /index is the real-world resource and /index.html would then be the page about this resource where a semantic client gets redirected via HTTP See Other.

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.

Member

javiereguiluz commented Jul 28, 2015

For example you want to match /index but then redirect to /index.html

I'm just asking: if you want to redirect ALL URLs to their .html counterparts, shouldn't you do that at proxy or web server level?

Contributor

SofHad commented Jul 23, 2016

👍

Member

Tobion commented Jul 23, 2016

Closing as there is no consensus.

@Tobion Tobion closed this Jul 23, 2016

@Tobion Tobion deleted the Tobion:url-default-param branch Jul 23, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment