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

[Routing] added route compile check to identify a default value... #5664

Closed
wants to merge 1 commit into from

Conversation

Tobion
Copy link
Member

@Tobion Tobion commented Oct 3, 2012

... of a required variable that does not match the requirement

reopened version of #5400

@Tobion
Copy link
Member Author

Tobion commented Oct 4, 2012

@fabpot ping

@fabpot
Copy link
Member

fabpot commented Oct 4, 2012

It breaks for instance symfony.com. We have a route like this:

 * @Route("/doc/{version}/{section}/{page}/", name="doc2_shortcut", defaults={"locale"="en", "section"="", "page"=""}, requirements={"page"=".*", "version"="(current|[\d\.]+(?:\-DEV|\-RC\d+|\-BETA\d+)?|master)"})

As you can see, there is a requirement on section and page, but they are both optional.

With this PR, I have the following exception:

"The default value "" of the required variable "section" in pattern "/doc/{version}/{section}/{page}/" does not match the requirement "[^/]+". This route definition makes no sense because this default can neither be used as default for generating URLs nor can it ever be returned by the matching process. You should change the default to something that meets the requirement or remove it."

@Tobion
Copy link
Member Author

Tobion commented Oct 4, 2012

No, neither section nor page are optional because there is static text at the end (the trailing slash). If they were optional just /doc/current would match this route, but it's not true.

And the exception is right. Because the default for section makes no sense (as the message explains). The default for page is ok because the requirement is .*.

@Tobion
Copy link
Member Author

Tobion commented Oct 4, 2012

This whole thing with implicit optional variables is pretty hard to grasp as they are only optional when they have a default AND are at the end of the pattern (so a single trailing slash makes a huge difference). If even you are confused it shows the big problem of this magic. This is one more reason for implementing #5424 as it is much more flexible and at the same time more explicit.

@Tobion
Copy link
Member Author

Tobion commented Dec 6, 2012

@fabpot as the exception explains, you can safely remove the default for your section variable because otherwise you would have gotten an exception when generating an URL with the default section anyway.

…required variable that does not match the requirement
@Tobion
Copy link
Member Author

Tobion commented Feb 25, 2013

@fabpot ping

@Tobion
Copy link
Member Author

Tobion commented Jul 23, 2016

This would help DX alot to understand strange route definitions. But as people already have route definitions that make no sense, this breaks BC for them. So closing.

@Tobion Tobion closed this Jul 23, 2016
@Tobion Tobion deleted the uselessparamdefaults2 branch July 23, 2016 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants