8000 [RFC][Routing] fix or trick ? by vicb · Pull Request #3651 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[RFC][Routing] fix or trick ? #3651

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
Closed
wants to merge 2 commits into from
Closed

[RFC][Routing] fix or trick ? #3651

wants to merge 2 commits into from

Conversation

vicb
Copy link
Contributor
@vicb vicb commented Mar 20, 2012

With this PR

/foo-{bar}-{baz} matches /foo-b/ar-b-az

I thought I was a great idea when I wrote it, I am not sure any more:

  • matching b/ar is a BC break, it should probably not be included in 2.0. However I think this is the expected behavior (but it also suffers from the problem? mentioned below)
  • matching b-az is a new feature (could it be considered as a BC break as such ?)

One issue I can see with matching b-az is that it would hide a /foo-{bar}-{baz}-{buz} route declared afterwards. However a it would a masked a /foo-{bar}-{baz}_{buz} even with the current code. So one "benefit" here is to be consistent.

@stof
Copy link
Member
stof commented Mar 20, 2012

I don't think matching b-az by default is a good idea as it is a BC rbeak and can lead to WTF. It is already possible to allow matching it by forcing the requirement instead of relying on the guessed one.

@vicb
Copy link
Contributor Author
vicb commented Mar 21, 2012

I was a bit unclear yesterday and forgot to add some details

This PR was first created to solve two "bugs" in the routing:

First bug:

  • the route /foo-{a}-{b}-{c}
  • would generate this regexp: '#^/misc\\-(?P<a>[^/\\-]+?)\\-(?P<b>[^\\-]+?)\\-(?P<c>[^\\-]+?)$#xs'
    *bug: the regexp for {a} should not disallow /

Second bug:

  • if a route pattern has a{var}b
  • a, b are considered as separators and the regexp will be ...a(?P<var>[^ab]+?)b...
  • "a" should not be disallowed in the regexp

Other:

In the first example the c does not include "-" because of the the second bug but it allows "/" which is think is yet an other bug.

So I think we should be clear on how the Routing should work and update accordingly.

@Seldaek
Copy link
Member
Seldaek commented Mar 21, 2012

Yup I think having a clear spec and tests backing that would be great, right now it's a bit fuzzy what happens when you don't use simple /{foo}/{bar} patterns. The changes look good other than that, from your description at least.

@denderello
Copy link
Contributor

+1 for the clear specs and tests.

However if this solves several issues at once and breaks BC it should not be included in 2.0.

@vicb vicb mentioned this pull request Mar 21, 2012
Closed
@vicb
Copy link
Contributor Author
vicb commented Mar 22, 2012

Closed in favor of PR #3678 targeting 2.1

@vicb vicb closed this Mar 22, 2012
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.
4 participants
@vicb @stof @Seldaek @denderello
0