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

[Routing] allow no-slash root on imported routes #26284

Merged
merged 1 commit into from Mar 22, 2018

Conversation

@nicolas-grekas
Member

nicolas-grekas commented Feb 23, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #12141
License MIT
Doc PR -

With this change, a collection is imported, its root can have no slash appended. e.g.:

import:
    resource: ...
    trailing_slash_on_root: false

Works also for XML and PHP-DSL.

@@ -6,6 +6,8 @@
<route id="app_homepage" path="/" controller="AppBundle:Homepage:show" />
<route id="app_homepage_no_slash" path="" controller="AppBundle:Homepage:show" />

This comment has been minimized.

@iltar

iltar Feb 23, 2018

Contributor

Isn't this notation deprecated as of 4.1? Would make sense to add the non-deprecated notation while at it

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 23, 2018

Member

There are more in the fixtures, but we don't really care as that's not parsed anywhere in Routing, where it's just a string.

This comment has been minimized.

@Tobion

Tobion Feb 23, 2018

Member

Well this is just a routing fixture and the controller is not used as a controller. So you can write anything in there.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 23, 2018

Member

we can even remove it, done :)

@Tobion

This comment has been minimized.

Member

Tobion commented Feb 23, 2018

I agree that this is the cleanest and simplest solution. But I fear it's a BC break for some people.
I've seen route definitions like path: ~ and path: '' alot which then get imported with a prefix. So for all of them it will change the URL.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Feb 23, 2018

@Tobion yep.

Here is a plan:

  • for redirecteable URL matchers, #26283 fixes the issue for GET/HEAD. For other methods, we would make the two routes equivalent, ie no redirections and no 404.
  • for non-redirecteable matchers, we would disable this behavior and keep the current one

Since the behavior is in RouteCollection, and since RouteCollection doesn't know about the types of the matcher, we would in fact add a constructor argument to it, to opt-in low-level (and we would always pass true via FrameworkBundle.)

@sebastianblum

This comment has been minimized.

sebastianblum commented Feb 23, 2018

@nicolas-grekas your code looks so easy, I tried it last year and stranded.

Thank you very much.

@Tobion

This comment has been minimized.

Member

Tobion commented Feb 23, 2018

for redirecteable URL matchers, #26283 fixes the issue for GET/HEAD

There is stll alot of potential for BC breaks. Like if someone used hardcoded relative links to assets, it doesn't work anymore. Or if you developed an API and the client does not support redirect, it breaks.
So I don't think it makes sense to base the behavior on the matcher.
The only solution I see is to create a new config on framework.router level that defaults to the old behavior. And for new projects we set the config to the new value in the recipes (which will be the new default in sf 5)

@sebastianblum

This comment has been minimized.

sebastianblum commented Feb 23, 2018

@Tobion I don't understand what you mean.

At the moment, we have this behaviour:
routes:

  • /
  • /api/
  • /api/query

these urls match: /, /api/, /api/query
this url is redirected: /api -> /api/
this url throws an 404: /api/query/

#26283 will fix the last case, in my opinion it is a new feature and a good feature.


The case why I had the problem is, that for api url reasons and / or SEO reasons, I want to replace my custom api controller with a bundle (or subfolder) and leave the url the same.

api_query:
    prefix: /api/query
    resource: ../src/Controller/Api/Query
    type: annotation

and the controller

    /**
     * @Route("")
     */
    public function queryIndexAction() 

doesn't work at the moment.

In a second application (api.domain.com) I can reuse this controller / bundle without prefix and the url will be /

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Feb 23, 2018

I've seen route definitions like path: ~

Good news, that's actually not true!: loaders do not allow setting null, eg YamlFileLoader::validate() forbids it. And instantiating a Route object directly with a null path fails since 4.0, because the argument is type-hinted string.
Which means we just have to make null behave as we want and done.

PR updated.

$route = new Route($node->getAttribute('path'), $defaults, $requirements, $options, $node->getAttribute('host'), $schemes, $methods, $condition);
$route = new Route('null' !== $path ? $path : null, $defaults, $requirements, $options, $node->getAttribute('host'), $schemes, $methods, $condition);

This comment has been minimized.

@Tobion

This comment has been minimized.

@Tobion

Tobion Feb 23, 2018

Member

Not sure how that interacts with use="required"

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 23, 2018

Member

actually that's not possible on attributes, so we have to do as implemented I suppose, deal with null as a special case

This comment has been minimized.

@Tobion

Tobion Feb 23, 2018

Member

Yeah I also figured. But I think it would be better to make path attribute optional in xml instead. null as a string is a hack and potential bc break.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 23, 2018

Member

make path attribute optional in xml instead

I fear this will hurt DX, as people will more easilly forget to add the path without having any reminder

null as a string is a hack and potential bc break

let's assume nobody configured a route on path "null", and that those who did wrote it as "/null", which is what we all do, isn't it?

This comment has been minimized.

@Tobion

Tobion Feb 23, 2018

Member

I fear this will hurt DX, as people will more easilly forget to add the path without having any reminder

Good point. Another brainstorming idea: Add a new attribute like empty-path=true|false that defaults to false for BC. So one could use path="" empty-path=true to achive that. Does not look nice but path="null" is not transform meaning at all and if you know xml (likely when you use xml config), you'd know it's not a real null value.

This comment has been minimized.

@sebastianblum

sebastianblum Feb 23, 2018

brainstorming: is it possible to use ~ also in xml like: ?
The meaning of this char would be / or null with prefix.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 23, 2018

Member

Done using empty-path="true"

@@ -107,16 +107,20 @@ public function supports($resource, $type = null)
*/
protected function parseRoute(RouteCollection $collection, \DOMElement $node, $path)
{
if ('' === ($id = $node->getAttribute('id')) || !$node->hasAttribute('path')) {
throw new \InvalidArgumentException(sprintf('The <route> element in file "%s" must have an "id" and a "path" attribute.', $path));
if ('' === ($id = $node->getAttribute('id')) || (!$node->hasAttribute('path') && 'true' !== $node->getAttribute('empty-path'))) {

This comment has been minimized.

@Tobion

Tobion Feb 23, 2018

Member

an xml boolean can also be 1

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 23, 2018

Member

let's make it accept only "true" (done)

@@ -37,7 +37,8 @@
<xsd:group ref="configs" minOccurs="0" maxOccurs="unbounded" />
<xsd:attribute name="id" type="xsd:string" use="required" />
<xsd:attribute name="path" type="xsd:string" use="required" />
<xsd:attribute name="path" type="xsd:string" />
<xsd:attribute name="empty-path" type="xsd:boolean" />

This comment has been minimized.

@Tobion

Tobion Feb 23, 2018

Member

You could make a restriction on the boolean that it can only be true. Putting false there does not make sense.

<xsd:attribute name="empty-path" type="onlyTrue" />
  
  <xsd:simpleType name="onlyTrue">
    <xsd:restriction base="xsd:boolean">
      <xsd:enumeration value="true"/>
      <xsd:enumeration value="1"/>
    </xsd:restriction>
  </xsd:simpleType>

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 23, 2018

Member

with type="only-true" and this, I get type 'only-true': The facet 'enumeration' is not allowed on types derived from the type atomic type 'xs:boolean'.

This comment has been minimized.

@nicolas-grekas
throw new \InvalidArgumentException(sprintf('The <route> element in file "%s" must have an "id" and a "path" (or "empty-path") attribute.', $path));
}
if ($node->hasAttribute('path') && 'true' === $node->getAttribute('empty-path')) {

This comment has been minimized.

@Tobion

Tobion Feb 23, 2018

Member

Since empty-path=false is useless, I would forbid specifing both ($node->hasAttribute('path') && $node->hasAttribute('empty-path'))

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 23, 2018

Member

I think that's already covered by the previous check

This comment has been minimized.

@sebastianblum

sebastianblum Feb 23, 2018

The solution with empty-path or path attribute is very nice

@Tobion

Tobion approved these changes Feb 23, 2018

@Tobion

This comment has been minimized.

Member

Tobion commented Feb 25, 2018

#12141 (comment) made me realize that for annotations path="" works if you specify the prefix on the class. But for other use-cases path="" means something else. That inconsistency is unfortunate.

@nicolas-grekas the difference between null and "" is probably hard to see for people. I think it would better in the long run to just use "" for this. Maybe we should reconsider adding a config/env variable to enable the new bahavior for path="". And when the config is not set and someone used an empty path so far, trigger a deprecation.
Implementation wise it would be very easy as we don't need to touch any of the loaders.

@sebastianblum

This comment has been minimized.

sebastianblum commented Feb 25, 2018

All examples for @route annotation in the documentation has atleast @route(„/„), so it is possible to trigger an deprecation warning in 3.4 / 4.0 if the path in the annotation is empty.

Imo it is bug in 4.0. Is it a bc break?

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Feb 25, 2018

Let's use "empty-path" for all configuration formats. We should certainly not create any new global flag for this.

@sebastianblum

This comment has been minimized.

sebastianblum commented Feb 25, 2018

@nicolas-grekas empty-path is the best solution.

Can / should we deprecate the use of @Route("").
This configuration is not possible with yams nor with xml.
I can try to create an pr for that if you agree that this is useful.

@Tobion

This comment has been minimized.

Member

Tobion commented Feb 25, 2018

I think explaining the difference between path="" and empty-path="true" is more confusing than having a configuration that nobody cares about because it's either coming automatically with flex or you don't need the config because you didn't use path="" at all yet.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Feb 28, 2018

Status: needs work
(waiting for #26143 and #26304 for now)

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Mar 12, 2018

I changed the approach: instead of being configured at the route level, the trailing slash is now configured while importing routes, thanks to a new "trim-root" option.
This is already rebased on top of #26143, so reviews should focus on the 3rd commit only.

@javiereguiluz

This comment has been minimized.

Member

javiereguiluz commented Mar 12, 2018

Can we rename the trim-root option to something like add_trailing_slash and set it to true as default?

@Tobion

This comment has been minimized.

Member

Tobion commented Mar 12, 2018

This goes into the direction is originally proposed in #12141.

What about add_trailing_slash_to_root: true (the default)

trailing_slash_on_root: true should be enough.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Mar 12, 2018

trailing_slash_on_root: true should be enough.

done, now waiting forr #26143 and a few tests.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Mar 19, 2018

PR rebased, ready.

@javiereguiluz

It looks really nice! Thanks for this Nicolas. Just asking: do we need new tests for this feature?

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Mar 19, 2018

Now tested, ready;

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Mar 21, 2018

(failures unrelated, PR ready)

@stof

This comment has been minimized.

Member

stof commented Mar 21, 2018

This looks good to me.

But this needs a doc PR. And it would also be good to update the PR description with the final solution to reduce confusion.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Mar 21, 2018

description updated

@@ -50,6 +51,14 @@ public function __destruct()
}
if (!\is_array($prefix)) {
$this->route->addPrefix($prefix);
if (!$trailingSlashOnRoot) {
$rootPath = (new Route(trim($prefix).'/'))->getPath();

This comment has been minimized.

@Tobion

Tobion Mar 21, 2018

Member

The RouteCollection does a little more

$prefix = trim(trim($prefix), '/');

You need to do the same thing otherwise it does not work for /prefix//. The same applies to the loaders.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 21, 2018

Member

fixed

@@ -40,7 +41,7 @@ public function __destruct()
*
* @return $this
*/
final public function prefix($prefix, string $namePrefix = '')
final public function prefix($prefix, string $namePrefix = '', bool $trailingSlashOnRoot = true)

This comment has been minimized.

@Tobion

Tobion Mar 21, 2018

Member

bool arguments to change behavior are usually a bad design. an alternative would be to create a separate method like removeTrailingSlash(string $path) that does exactly what is currently implemented but in a separate method that reads like prose instead of some non-saying argument.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Mar 21, 2018

create a separate method like removeTrailingSlash(string $path)

That is not possible: $trailingSlashOnRoot is really a parameter that configures the prefixing logic. Once prefixing is done, we cannot know which routes where root.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Mar 21, 2018

create a separate method like removeTrailingSlash(string $path)

I somehow missed that your proposal has the path as argument. But that requires prior knowledge of which paths are the root, and doesn't work with localized paths (or requires knowledge of all the variants, which is bad coupling.)

@Tobion

Tobion approved these changes Mar 21, 2018

@fabpot

fabpot approved these changes Mar 22, 2018

@fabpot

This comment has been minimized.

Member

fabpot commented Mar 22, 2018

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 5a98515 into symfony:master Mar 22, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 22, 2018

feature #26284 [Routing] allow no-slash root on imported routes (nico…
…las-grekas)

This PR was merged into the 4.1-dev branch.

Discussion
----------

[Routing] allow no-slash root on imported routes

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #12141
| License       | MIT
| Doc PR        | -

With this change, a collection is imported, its root can have no slash appended. e.g.:

```yaml
import:
    resource: ...
    trailing_slash_on_root: false
```

Works also for XML and PHP-DSL.

Commits
-------

5a98515 [Routing] allow no-slash root on imported routes

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:route-no-slash branch Mar 22, 2018

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

@RobinDev

This comment has been minimized.

RobinDev commented Nov 15, 2018

Hello,

I am trying to understand why when we have perfixed routes (for example /{_locale} with a default locale) and in this routes, this one :

page:
    path:     /{slug}
    defaults:
        slug: 'homepage'
    ...

The generator remove the ending slash when I try $this->get('router')->generate('page') (and locale is setted to a non-default value).
Eg: _locale is set to en, the router generate /en (and not my|the? expected /en/).

I go deep into the code (in RouteCollection and UrlGenerator) but can't find the explanations.

I am using Symfony since 4.1.

@Tobion

This comment has been minimized.

Member

Tobion commented Nov 15, 2018

It's because you set a default value for slug. In case you want the trailing slash, you can remove the default value and instead change the requirement for slug to allow an empty value like [^/]*

@RobinDev

This comment has been minimized.

RobinDev commented Nov 15, 2018

Thank you for your reply.

It may work, but if I remove the defaults then I can't generate the url. Generate('page') throw "Some mandatory parameters are missing".

Re-reading the doc, I understand the behavior. It is what is expected (last line).

@Tobion

This comment has been minimized.

Member

Tobion commented Nov 15, 2018

Yes you need to provide the slug param then. Another solution to get the trailing slash is to create two routes:

  • /en/
  • /en/{slug}
@RobinDev

This comment has been minimized.

RobinDev commented Nov 15, 2018

If I do that, I loose my domain page with default locale : / redirect to /default-locale/. And I want to keep it to. And I want to keep it.

I try a few workaround, but I can't find a Solution (my best was to get / and /default-locale/ working and generate /default-locale/).

I will end to do a twig function to correct the URL generator (I am not sur I can override directly the UrlGenerator class from a bundle).

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