8000 [Routing] added route compile check to identify a bad default value by Tobion · Pull Request #5400 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Routing] added route compile check to identify a bad default value #5400

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 31, 2012
Merged

[Routing] added route compile check to identify a bad default value #5400

merged 1 commit into from
Aug 31, 2012

Conversation

Tobion
Copy link
Contributor
@Tobion Tobion commented Aug 31, 2012

BC break: yes but only for strange route definitions

See the exception message in code for the reasoning.

An exception is thrown for a required variable that has a default that doesn't match the requirement.
So optional variables can of course still have a default that don't meet the requirement, which is useful.
This helps to identify useless route definitions at compile time instead of when generating or matching a URL.

@Tobion
[Routing] added route compile check to identify a default value of a …
8000
cb7e3f5 
…required variable that does not match the requirement
@Tobion Tobion mentioned this pull request Aug 31, 2012
Closed
@travisbot
Copy link

This pull request passes (merged cb7e3f5 into 2cf50b7).

fabpot added a commit that referenced this pull request Aug 31, 2012
@fabpot
merged branch Tobion/uselessparamdefaults (PR #5400)
0f61b2e 
Commits
-------

cb7e3f5 [Routing] added route compile check to identify a default value of a required variable that does not match the requirement

Discussion
----------

[Routing] added route compile check to identify a bad default value

BC break: yes but only for strange route definitions

See the exception message in code for the reasoning.

An exception is thrown for a __required__ variable that __has a default__ that __doesn't match__ the requirement.
So optional variables can of course still have a default that don't meet the requirement, which is useful.
This helps to identify useless route definitions at compile time instead of when generating or matching a URL.
@fabpot fabpot merged commit cb7e3f5 into symfony:master Aug 31, 2012
@acasademont
Copy link
Contributor

Mmmm, i have this route

@Route("/profile/orders/{id}", name="order_show", requirements={"id"="\d+"}, defaults={"id"=null})

public function showAction(Request $request, $id) {}

When the user doesn't choose any order, id is null and it displays the last one. With this PR this route is going to throw an Exception which is not very nice, i don't think the route is bad in any way. If the user doesn't chose any id, i want it to be null, not 0 or -1. In the docs example about routing the default for the blog page is set to 1, as this is always going te be the first page, but in my case, i do not know beforehand which is the last order id of the user, i need that param to be set to null.

Can we reconsider this or at least, discuss it a little bit more? I don't think we should be BC breaking things in RC2, this kind of route is not that strange.

The only option which i can think of is having 2 routes for the same controller action with different params

@Route("/profile/orders", name="order_index")
@Route("/profile/orders/{id}", name="order_show", requirements={"id"="\d+"})

public function showAction(Request $request, $id = null) {}

8000
@acasademont
Copy link
Contributor

This is like Validation in forms. All validators accept the null value. Only when the value is not null then they validate it against the validation rules. I guess the behaviour should be the same. Maybe we could add an exception for the null value?

@sstok
Copy link
Contributor
sstok commented Aug 31, 2012

+1 for allowing an null value

@Tobion
Copy link
Contributor Author
Tobion commented Aug 31, 2012

@acasademont Your route with null as default is fine and shouldn't be influenced by this PR because id is an optional variable for you. Does it really throw an exception for your route?

@acasademont
Copy link
Contributor

Nope i couldn't try it yet, sorry. To my knowledge, all variables in the "requirements" section are not optional unless a "defaults" for that variable is set. I see from the code that the first optional variable is not checked agains the requirements regexp. If that is the case, all i said was wrong.

fabpot added a commit that referenced this pull request Sep 5, 2012
@fabpot
Revert "merged branch Tobion/uselessparamdefaults (PR #5400)"
fdf2528 
This reverts commit 0f61b2e, reversing
changes made to 5e7723f.
@fabpot
Copy link
Member
fabpot commented Sep 5, 2012

I've just reverted this pull request as it introduces some regressions? It was a very bad idea to merge this so close to the release of 2.1 and I'm the one to blame. Sorry about that.

@Tobion As apparently it is not possible to reopen a pull request, can you re-submit it again, so that it is not lost? Thanks and sorry again for the trouble.

@Tobion Tobion mentioned this pull request Oct 3, 2012
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Tobion @travisbot @acasademont @sstok @fabpot
0