Skip to content
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

[DX] Improve route requirements definition: allow to define requirements inline with placeholders itself #26481

Closed
Strate opened this issue Mar 10, 2018 · 19 comments
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature Routing

Comments

@Strate
Copy link
Contributor

Strate commented Mar 10, 2018

Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? yes
Symfony version 4.x

It would be great to inline requirements to pattern itself:
/route/path/{parameter:\d+} instead of /route/path/{parameter}, requirements={{parameter: "\d+"}}

@javiereguiluz javiereguiluz added Feature Routing DX DX = Developer eXperience (anything that improves the experience of using Symfony) labels Mar 10, 2018
@javiereguiluz
Copy link
Member

For reference purposes, Django framework allows something similar:

'articles/<int:year>/<int:month>/<slug:slug>/'

The special converters are int, slug, str, uuid and path, but you can use regex too:

r'^articles/(?P<year>[0-9]{4})/(?P<month>[0-9]{2})/$'

@fabpot
Copy link
Member

fabpot commented Mar 10, 2018

It's been on my list of things to do forever. I would love to see somebody work on this one. Would definitely be a huge DX improvement.

@javiereguiluz
Copy link
Member

javiereguiluz commented Mar 10, 2018

We can consider the previous comment from Fabien as an approval of this idea 🎉, so now let's talk about the details before someone can try to implement it. The syntax proposed by @Strate looks nice to me:

/route/path/{param-name}
/route/path/{param-name:param-requirement-as-regex}

I'd use that simple syntax and wouldn't complicate things with something like the Django's converters.

Now, two things to think about:

  1. What if the regex contains a :? Should we escape it ... or should we be smart and split the param by the first : and ignore the other possible occurrences of :?
  2. What if we want to extend this idea to also define default values for params? We can't use another : as separator (/{page:\d+:1}) if we are not careful in 1).

@sstok
Copy link
Contributor

sstok commented Mar 11, 2018

How about using a ;? That's what I did in RollerworksAppSectioning.
https://github.com/rollerworks/app-sectioning/blob/master/docs/install.md#dynamic-host-requirements

The ; is seen as separator by most URI's but not used any reges while : could be used a regex for none-catching groups (which is almost a requirement for usage in the Route path).

@linaori
Copy link
Contributor

linaori commented Mar 12, 2018

Just throwing out some ideas here, because I really love this proposal:

// |
@Route("/foo/{bar|\d+|1}/")
@Route("/foo/{bar|.*}/")
@Route("/foo/{bar|[a-zA-Z0-9]{2,3}-[a-zA-Z0-9]{2,3}|co-uk}/")

// ;
@Route("/foo/{bar;\d;1}/")
@Route("/foo/{bar;.*}/")
@Route("/foo/{bar;[a-zA-Z0-9]{2,3}-[a-zA-Z0-9]{2,3};co-uk}/")

// " "
@Route("/foo/{bar \d 1}/")
@Route("/foo/{bar .*}/")
@Route("/foo/{bar [a-zA-Z0-9]{2,3}-[a-zA-Z0-9]{2,3} co-uk}/")

// ,
@Route("/foo/{bar,\d,1}/")
@Route("/foo/{bar,.*}/")
@Route("/foo/{bar,[a-zA-Z0-9]{2,3}-[a-zA-Z0-9]{2,3},co-uk}/")

// ", "
@Route("/foo/{bar, \d, 1}/")
@Route("/foo/{bar, .*}/")
@Route("/foo/{bar, [a-zA-Z0-9]{2,3}-[a-zA-Z0-9]{2,3}, co-uk}/")

// "; "
@Route("/foo/{bar; \d; 1}/")
@Route("/foo/{bar; .*}/")
@Route("/foo/{bar; [a-zA-Z0-9]{2,3}-[a-zA-Z0-9]{2,3}; co-uk}/")

// "| "
@Route("/foo/{bar| \d| 1}/")
@Route("/foo/{bar| .*}/")
@Route("/foo/{bar| [a-zA-Z0-9]{2,3}-[a-zA-Z0-9]{2,3}| co-uk}/")

@nibsirahsieu
Copy link

+1 for this. Remind me a few years back when i was using play framework. Just for reference https://www.playframework.com/documentation/2.6.x/ScalaRouting#dynamic-parts-with-custom-regular-expressions

@nicolas-grekas
Copy link
Member

What about this?

{bar} -- no requirement, no default value
{bar<.*>} -- with requirement, no default value
{bar?default_value} -- no requirement, with default value
{bar<.*>?default_value} -- with requirement and default value

I think the implementation shouldn't be very complex, we could put the parsing logic in Route::setPath() (and replace setDefaults/setRequirements() by addDefaults/addRequirements() in the constructor)

Anyone willing to give it a try?

@fmata
Copy link
Contributor

fmata commented Mar 13, 2018

I can try :)

@Tobion
Copy link
Contributor

Tobion commented Mar 14, 2018

I like @nicolas-grekas syntax the most. But I would propose a small change

{bar=default_value}

I think this looks more what people are used to and makes it clear what the name and what the value is.

I would also recommend to do the parsing directly in Route::setPath so to the outside a route is still represented as usual with requirements and defaults. This way a route path is still compatible with URI template standard level 1 (https://tools.ietf.org/html/rfc6570#section-1.2)
I looked at the URI template standard again, but it does not define a syntax for default values or requirements (because it is only meant for generating but not matching).

@Tobion
Copy link
Contributor

Tobion commented Mar 14, 2018

One problem I see with having the default value inside the path is that it will be hard to set a non-string default. Often times you only want to make the param optional but don't care about the default. You just need to be able to distinguish between a given value and no value given.

For example this common case

route:
    path: /blog/{slug}
    defaults:
        slug: null

Translating that to

route:
    path: /blog/{slug=null} # not clear if it means 'null' as string or null
    path: /blog/{slug=~} # has the same problem: null or '~' ?

For this common case I would propose to add a shortcut syntax

route:
    path: /blog/{?slug}

@Tobion
Copy link
Contributor

Tobion commented Mar 14, 2018

Actually that brings be back to what nicolas proposed. With a little addition: You can leave out the default value, which means null. Sorry for dumping by brain here.

{bar} -- no requirement, no default value
{bar<.*>} -- with requirement, no default value
{bar?default_value} -- no requirement, with default value
{bar<.*>?default_value} -- with requirement and default value
{bar?} -- no requirement, with default value of null
{bar<.*>?} -- with requirement and default value of null

@linaori
Copy link
Contributor

linaori commented Mar 14, 2018

What will happen with something like: /foo/{bar?}/baz/ and /foo/{bar<.*>?}/baz/? I know this is currently possible, but it makes it really confusing.

@nicolas-grekas
Copy link
Member

The ? just configures the default value. Being optional means both having a default value and being at a tail position. Nothing new here.

@javiereguiluz
Copy link
Member

javiereguiluz commented Mar 14, 2018

I like Nicolas proposal of using <...> for the requirement, but I'd like to ask why don't you like using a = to define the default value. It looks more natural to me:

{bar} -- no requirement, no default value
{bar<.*>} -- with requirement, no default value
{bar=default_value} -- no requirement, with default value
{bar<.*>=default_value} -- with requirement and default value
{bar=} -- no requirement, with default value of null
{bar<.*>=} -- with requirement and default value of null

And now some real-world route examples to see this in action:

# Symfony Demo app
@Route("/page/{page}", defaults={"_format"="html"}, requirements={"page": "[1-9]\d*"}, name="blog_index_paginated")
@Route("/page/{page<[1-9]\d*>}", defaults={"_format"="html"}, name="blog_index_paginated")

# Symfony Demo app
@Route("/admin/post/{id}", requirements={"id": "\d+"}, name="admin_post_show")
@Route("/admin/post/{id<\d+>}", name="admin_post_show")

# Twig bundle app
@Route("/{code}.{_format}", requirements={"code": "\d+"}, defaults={"_format"="html"}, name="_twig_error_test")
@Route("/{code<\d+>}.{_format=html}", name="_twig_error_test")

@linaori
Copy link
Contributor

linaori commented Mar 14, 2018

I agree that in a real world example, it looks a lot better with =. However, the >= is slightly confusing, but imo not a deal breaker.

@mvrhov
Copy link

mvrhov commented Mar 14, 2018

Maybe both of these cases should be made valid. So You don't have to remember which way it is....

{bar<.*>=default_value} -- with requirement and default value
{bar=default_value<.*>} -- with requirement and default value

{bar<.*>=} -- with requirement and default value of null
{bar=<.*>} -- with requirement and default value of null

@linaori
Copy link
Contributor

linaori commented Mar 14, 2018

{bar=default_value<.*>} slightly reminds me of generics notation wise, kinda "feels right"

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Mar 14, 2018

Here we are: #26518

{bar=default_value<.*>} -- with requirement and default value

The default value is a string, why should it be parsed? Not a good idea to me as it creates more ambiguities than it should.

why don't you like using a = to define the default value

because ? better conveys the idea that there is something conditional there, while = conveys the idea that there is some affectation, which might happen, but only conditionally (thus ? is a more important idea to convey IMHO)

@javiereguiluz
Copy link
Member

because ? better conveys the idea that there is something conditional there, while = conveys the idea that there is some affectation

OK. I just wanted to avoid issues if some day Symfony adopts the URI Template RFC, where ? is used a lot.

nicolas-grekas added a commit that referenced this issue Mar 19, 2018
…defaults (nicolas-grekas)

This PR was merged into the 4.1-dev branch.

Discussion
----------

[Routing] Allow inline definition of requirements and defaults

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

```
{bar} -- no requirement, no default value
{bar<.*>} -- with requirement, no default value
{bar?default_value} -- no requirement, with default value
{bar<.*>?default_value} -- with requirement and default value
{bar?} -- no requirement, with default value of null
{bar<.*>?} -- with requirement and default value of null
```

Details:

* Requirements and default values are not escaped in any way. This is valid -> `@Route("/foo/{bar<>>?<>}")` (requirements = `>`  and default value = `<>`)
* Because of the lack of escaping, you can't use a closing brace (`}`) inside the default value (wrong -> `@Route("/foo/{bar<\d+>?aa}bb}")`) but you can use it inside requirements (correct -> `@Route("/foo/{bar<\d{3}>}")`).
* PHP constants are not supported (use the traditional `defaults` syntax for that)
* ...

Commits
-------

67559e1 [Routing] Allow inline definition of requirements and defaults
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature Routing
Projects
None yet
Development

No branches or pull requests

10 participants