Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Routing] Allow inline definition of requirements and defaults#26518
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
0819bf1 to464c17eCompare| $this->assertEquals((newRoute('/foo/{bar}'))->setRequirement('bar','\d+'),newRoute('/foo/{bar<.*>}',array(),array('bar' =>'\d+'))); | ||
| $this->assertEquals((newRoute('/foo/{bar}'))->setDefault('bar',null)->setRequirement('bar','.*'),newRoute('/foo/{bar<.*>?}')); | ||
| $this->assertEquals((newRoute('/foo/{bar}'))->setDefault('bar','<>')->setRequirement('bar','>'),newRoute('/foo/{bar<>>?<>}')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This one is surprising. Instead of/foo/{bar<>>?<>} I thought this should be like/foo/{bar<\>>? <>} to escape the> value as a requirement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
there is no escaping to keep the parser trivial
I don't think this limits anything in anyway
derrabus commentedMar 14, 2018
Regarding the escaping: Could my default value contain curly braces? |
nicolas-grekas commentedMar 14, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Not a closing one that's correct. |
derrabus commentedMar 14, 2018
All right. We can probably live with that limitation, but we should document it. |
Tobion commentedMar 14, 2018
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 commentedMar 14, 2018
will !php/const work inside? |
nicolas-grekas commentedMar 14, 2018
no, but you could submit a PR on top to make constants work, eg |
nicolas-grekas commentedMar 14, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
or just use the existing syntax btw... (explicit |
javiereguiluz commentedMar 14, 2018
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 commentedMar 14, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Nice feature. What about a case like |
nicolas-grekas commentedMar 14, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@Destroy666x see tests, that's covered: the shortest regexp is used |
ostrolucky commentedMar 14, 2018
It's not so important, was just curious. Seems this syntax can be freely combined with _defaults anyway 👍 |
Destroy666x commentedMar 14, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@nicolas-grekas I see a similar test but without EDIT: I see, could you please add a test for |
ostrolucky left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 commentedMar 14, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
please review again, addDefaults is used ;) |
ostrolucky commentedMar 14, 2018
Indeed. Can you add a test case for passing defaults both via path and via defaults tho? |
nicolas-grekas commentedMar 14, 2018
review again :) |
| { | ||
| $this->assertEquals((newRoute('/foo/{bar}'))->setDefault('bar',null),newRoute('/foo/{bar?}')); | ||
| $this->assertEquals((newRoute('/foo/{bar}'))->setDefault('bar','baz'),newRoute('/foo/{bar?baz}')); | ||
| $this->assertEquals((newRoute('/foo/{bar}'))->setDefault('bar','baz<buz>'),newRoute('/foo/{bar?baz<buz>}')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 likebaz<buz> then just use the explicit config. But having such a default is much less likely than a user mixing up the order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nicolas-grekasMar 14, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 likebaz<buz> in a URL.
nicolas-grekasMar 16, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
| publicfunctionsetPath($pattern) | ||
| { | ||
| if (false !==strpbrk($pattern,'?<')) { | ||
| $pattern =preg_replace_callback('#\{(\w++)(<.*?>)?(\?[^\}]*+)?\}#',function ($m) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
what does{bar<>} exactly do?
ah that's sanitized. never mind.. perhaps a test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this is already tested elsewhere, nothing specific to this feature
nicolas-grekas commentedMar 16, 2018
@symfony/deciders votes pending |
| $this->assertEquals((newRoute('/foo/{bar}'))->setDefault('bar',null)->setRequirement('bar','.*'),newRoute('/foo/{bar<.*>?}')); | ||
| $this->assertEquals((newRoute('/foo/{bar}'))->setDefault('bar','<>')->setRequirement('bar','>'),newRoute('/foo/{bar<>>?<>}')); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
maybe there should also be a test case with using curly braces in the requirements?
Like'/foo/{_locale<[a-z]{2}>}' for example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
case added
464c17e to67559e1Compare…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
Uh oh!
There was an error while loading.Please reload this page.
Details:
@Route("/foo/{bar<>>?<>}")(requirements =>and default value =<>)}) inside the default value (wrong ->@Route("/foo/{bar<\d+>?aa}bb}")) but you can use it inside requirements (correct ->@Route("/foo/{bar<\d{3}>}")).defaultssyntax for that)