[Routing] Allow inline definition of requirements and defaults #26518

nicolas-grekas · 2018-03-14T09:05:53Z

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

javiereguiluz · 2018-03-14T09:26:27Z

src/Symfony/Component/Routing/Tests/RouteTest.php

+        $this->assertEquals((new Route('/foo/{bar}'))->setRequirement('bar', '\d+'), new Route('/foo/{bar<.*>}', array(), array('bar' => '\d+')));
+
+        $this->assertEquals((new Route('/foo/{bar}'))->setDefault('bar', null)->setRequirement('bar', '.*'), new Route('/foo/{bar<.*>?}'));
+        $this->assertEquals((new Route('/foo/{bar}'))->setDefault('bar', '<>')->setRequirement('bar', '>'), new Route('/foo/{bar<>>?<>}'));


This one is surprising. Instead of /foo/{bar<>>?<>} I thought this should be like /foo/{bar<\>>? <>} to escape the > value as a requirement.

there is no escaping to keep the parser trivial
I don't think this limits anything in anyway

derrabus · 2018-03-14T09:34:34Z

Regarding the escaping: Could my default value contain curly braces?

nicolas-grekas · 2018-03-14T09:35:33Z

Could my default value contain curly braces?

Not a closing one that's correct.

derrabus · 2018-03-14T09:52:23Z

All right. We can probably live with that limitation, but we should document it.

Tobion · 2018-03-14T10:13:58Z

I would also leave escaping out. If you have such a special regex or default, you can simply use the old long route definition.

ostrolucky · 2018-03-14T10:17:25Z

will !php/const work inside?

nicolas-grekas · 2018-03-14T10:33:12Z

will !php/const work inside?

no, but you could submit a PR on top to make constants work, eg

{bar!SOME_CONST} -- no requirement, with default value
{bar<.*>!SOME_CONST} -- with requirement and default value

nicolas-grekas · 2018-03-14T10:33:53Z

you could submit a PR on top to make constants work, eg

or just use the existing syntax btw... (explicit defaults: ...)

javiereguiluz · 2018-03-14T10:34:35Z

Is it really so important to support PHP constants here? They could make routes really difficult to understand or very easy to override some typo or wrong character.

Destroy666x · 2018-03-14T10:44:48Z

Nice feature. What about a case like {bar<test>?>?test}?

nicolas-grekas · 2018-03-14T10:48:47Z

@Destroy666x see tests, that's covered: the shortest regexp is used
(use e.g. {bar<test[>]?>?test} if you want the other way)

ostrolucky · 2018-03-14T10:49:50Z

It's not so important, was just curious. Seems this syntax can be freely combined with _defaults anyway 👍

Destroy666x · 2018-03-14T10:57:54Z

@nicolas-grekas I see a similar test but without ? inside <>, which is allowed in requirements, right? It's kind of ambigous which ? is the separator

EDIT: I see, could you please add a test for bar<test[>]?>?test too then? Or maybe it's good to mention that in documentation

ostrolucky

I don't think this is gonna work. Route::__construct calls setPath first, then setDefaults which erases those defaults as a first thing

nicolas-grekas · 2018-03-14T11:00:33Z

I don't think this is gonna work. Route::__construct calls setPath first, then setDefaults which erases those defaults first

please review again, addDefaults is used ;)

ostrolucky · 2018-03-14T11:03:47Z

Indeed. Can you add a test case for passing defaults both via path and via defaults tho?

nicolas-grekas · 2018-03-14T11:28:39Z

Indeed. Can you add a test case for passing defaults both via path and via defaults tho?

review again :)

Tobion · 2018-03-14T17:11:03Z

src/Symfony/Component/Routing/Tests/RouteTest.php

+    {
+        $this->assertEquals((new Route('/foo/{bar}'))->setDefault('bar', null), new Route('/foo/{bar?}'));
+        $this->assertEquals((new Route('/foo/{bar}'))->setDefault('bar', 'baz'), new Route('/foo/{bar?baz}'));
+        $this->assertEquals((new Route('/foo/{bar}'))->setDefault('bar', 'baz<buz>'), new Route('/foo/{bar?baz<buz>}'));


How about we make it so that it does not matter if you specify a default or regex first?
So '/foo/{bar?baz<.*>}' == '/foo/{bar<.*>?baz}'

This seems more user-friendly as you don't need to remember the order. Also this syntax is not feature-complete anyway due to missing escaping. So if you really need to specify a default like baz<buz> then just use the explicit config. But having such a default is much less likely than a user mixing up the order.

see #26481 (comment)

I think having two ways to express things just creates more confusion.
Having only one order means everyone will write these in exactly the same way, thus less WTF when switching projects.

Normally I agree consistency is better. But I also want to prevent bugs in the first place. So when we do not accept both ways, it would be cleaner to throw an exception if you do it wrong. I cannot believe somewhat actually uses a default like baz<buz> in a URL.

I cannot believe somewhat actually uses a default like baz in a URL

This is not a reason to technically forbid it. Symfony devs have been proved surprising many time :)

It's not forbidden. You just need to use the explicit syntax.

Symfony devs have been proved surprising many time

Yes they will be surprised with unexpected behavior.

ro0NL · 2018-03-15T22:35:33Z

src/Symfony/Component/Routing/Route.php

@@ -123,6 +123,19 @@ public function getPath()
     */
    public function setPath($pattern)
    {
+        if (false !== strpbrk($pattern, '?<')) {
+            $pattern = preg_replace_callback('#\{(\w++)(<.*?>)?(\?[^\}]*+)?\}#', function ($m) {


what does {bar<>} exactly do?

ah that's sanitized. never mind.. perhaps a test?

this is already tested elsewhere, nothing specific to this feature

nicolas-grekas · 2018-03-16T09:57:56Z

@symfony/deciders votes pending

dmaicher · 2018-03-16T17:08:48Z

src/Symfony/Component/Routing/Tests/RouteTest.php

+
+        $this->assertEquals((new Route('/foo/{bar}'))->setDefault('bar', null)->setRequirement('bar', '.*'), new Route('/foo/{bar<.*>?}'));
+        $this->assertEquals((new Route('/foo/{bar}'))->setDefault('bar', '<>')->setRequirement('bar', '>'), new Route('/foo/{bar<>>?<>}'));
+    }


maybe there should also be a test case with using curly braces in the requirements?

Like '/foo/{_locale<[a-z]{2}>}' for example?

…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

nicolas-grekas added this to the 4.1 milestone Mar 14, 2018

carsonbot added Status: Needs Review Routing Feature labels Mar 14, 2018

nicolas-grekas mentioned this pull request Mar 14, 2018

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

Closed

nicolas-grekas force-pushed the route-inline-reqs branch from 0819bf1 to 464c17e Compare March 14, 2018 09:12

carsonbot added Status: Reviewed and removed Status: Needs Review labels Mar 14, 2018

derrabus approved these changes Mar 14, 2018

View reviewed changes

javiereguiluz reviewed Mar 14, 2018

View reviewed changes

javiereguiluz added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Mar 14, 2018

carsonbot added Status: Needs Work and removed Status: Reviewed labels Mar 14, 2018

ostrolucky suggested changes Mar 14, 2018

View reviewed changes

carsonbot removed the Status: Needs Work label Mar 14, 2018

ostrolucky approved these changes Mar 14, 2018

View reviewed changes

carsonbot added the Status: Reviewed label Mar 14, 2018

linaori mentioned this pull request Mar 14, 2018

[RFC] Predefined conditions in route parameter requirements #26524

Closed

Tobion reviewed Mar 14, 2018

View reviewed changes

ro0NL reviewed Mar 15, 2018

View reviewed changes

Tobion approved these changes Mar 16, 2018

View reviewed changes

Simperfit approved these changes Mar 16, 2018

View reviewed changes

dmaicher approved these changes Mar 16, 2018

View reviewed changes

[Routing] Allow inline definition of requirements and defaults

67559e1

nicolas-grekas force-pushed the route-inline-reqs branch from 464c17e to 67559e1 Compare March 19, 2018 09:06

dmaicher approved these changes Mar 19, 2018

View reviewed changes

nicolas-grekas merged commit 67559e1 into symfony:master Mar 19, 2018

nicolas-grekas deleted the route-inline-reqs branch March 19, 2018 12:13

javiereguiluz mentioned this pull request Mar 20, 2018

[Routing] Allow inline definition of requirements and defaults symfony/symfony-docs#9459

Closed

fabpot mentioned this pull request May 7, 2018

Release v4.1.0-BETA1 #27181

Merged

nicolas-grekas mentioned this pull request Apr 3, 2019

[Routing] Fix: annotation loader ignores method's default values #30825

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Routing] Allow inline definition of requirements and defaults #26518

[Routing] Allow inline definition of requirements and defaults #26518

nicolas-grekas commented Mar 14, 2018 •

edited by javiereguiluz

Loading

javiereguiluz Mar 14, 2018

nicolas-grekas Mar 14, 2018

derrabus commented Mar 14, 2018

nicolas-grekas commented Mar 14, 2018 •

edited

Loading

derrabus commented Mar 14, 2018

Tobion commented Mar 14, 2018

ostrolucky commented Mar 14, 2018

nicolas-grekas commented Mar 14, 2018

nicolas-grekas commented Mar 14, 2018 •

edited

Loading

javiereguiluz commented Mar 14, 2018

Destroy666x commented Mar 14, 2018 •

edited

Loading

nicolas-grekas commented Mar 14, 2018 •

edited

Loading

ostrolucky commented Mar 14, 2018

Destroy666x commented Mar 14, 2018 •

edited

Loading

ostrolucky left a comment •

edited

Loading

nicolas-grekas commented Mar 14, 2018 •

edited

Loading

ostrolucky commented Mar 14, 2018

nicolas-grekas commented Mar 14, 2018

Tobion Mar 14, 2018 •

edited

Loading

nicolas-grekas Mar 14, 2018

nicolas-grekas Mar 14, 2018 •

edited

Loading

Tobion Mar 14, 2018

nicolas-grekas Mar 16, 2018 •

edited

Loading

Tobion Mar 16, 2018 •

edited

Loading

ro0NL Mar 15, 2018 •

edited

Loading

nicolas-grekas Mar 16, 2018

nicolas-grekas commented Mar 16, 2018

dmaicher Mar 16, 2018

nicolas-grekas Mar 19, 2018

[Routing] Allow inline definition of requirements and defaults #26518

[Routing] Allow inline definition of requirements and defaults #26518

Conversation

nicolas-grekas commented Mar 14, 2018 • edited by javiereguiluz Loading

javiereguiluz Mar 14, 2018

Choose a reason for hiding this comment

nicolas-grekas Mar 14, 2018

Choose a reason for hiding this comment

derrabus commented Mar 14, 2018

nicolas-grekas commented Mar 14, 2018 • edited Loading

derrabus commented Mar 14, 2018

Tobion commented Mar 14, 2018

ostrolucky commented Mar 14, 2018

nicolas-grekas commented Mar 14, 2018

nicolas-grekas commented Mar 14, 2018 • edited Loading

javiereguiluz commented Mar 14, 2018

Destroy666x commented Mar 14, 2018 • edited Loading

nicolas-grekas commented Mar 14, 2018 • edited Loading

ostrolucky commented Mar 14, 2018

Destroy666x commented Mar 14, 2018 • edited Loading

ostrolucky left a comment • edited Loading

Choose a reason for hiding this comment

nicolas-grekas commented Mar 14, 2018 • edited Loading

ostrolucky commented Mar 14, 2018

nicolas-grekas commented Mar 14, 2018

Tobion Mar 14, 2018 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas Mar 14, 2018

Choose a reason for hiding this comment

nicolas-grekas Mar 14, 2018 • edited Loading

Choose a reason for hiding this comment

Tobion Mar 14, 2018

Choose a reason for hiding this comment

nicolas-grekas Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

Tobion Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

ro0NL Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas Mar 16, 2018

Choose a reason for hiding this comment

nicolas-grekas commented Mar 16, 2018

dmaicher Mar 16, 2018

Choose a reason for hiding this comment

nicolas-grekas Mar 19, 2018

Choose a reason for hiding this comment

nicolas-grekas commented Mar 14, 2018 •

edited by javiereguiluz

Loading

nicolas-grekas commented Mar 14, 2018 •

edited

Loading

nicolas-grekas commented Mar 14, 2018 •

edited

Loading

Destroy666x commented Mar 14, 2018 •

edited

Loading

nicolas-grekas commented Mar 14, 2018 •

edited

Loading

Destroy666x commented Mar 14, 2018 •

edited

Loading

ostrolucky left a comment •

edited

Loading

nicolas-grekas commented Mar 14, 2018 •

edited

Loading

Tobion Mar 14, 2018 •

edited

Loading

nicolas-grekas Mar 14, 2018 •

edited

Loading

nicolas-grekas Mar 16, 2018 •

edited

Loading

Tobion Mar 16, 2018 •

edited

Loading

ro0NL Mar 15, 2018 •

edited

Loading