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

fixes #94 Throws a RouteNotFoundException is a match is found where t… #95

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

bartmcleod
Copy link

Throws a RouteNotFoundException when a match is found where the matched locale from the prefix is different from that in the postfix.

…he matched locale from the prefix is different from that in the postfix
@bartmcleod
Copy link
Author

bartmcleod commented Oct 19, 2016

Do not merge this yet, there are two problems with the solution, that need to be addresses first:

  1. If for a number of locales, the same path has been defined in the route, the first locale that matches wins and is not necessarily the same as the prefixed locale. It looks hard to solve, but possibly iterating over the route collection can solve this. It will need an extra unit test to say the least.
  2. If people use code like generate('routename.nl') instead of generate('routename', ['_locale' => 'nl']) they will be in trouble. As far as I'm concerned, this is wrong usage, but a change list should warn for this.

@bartmcleod
Copy link
Author

My latest commit fixes concern #1, but relies on a postfix inflector being used. The conditions for exact matching of configured routes should probably be delegated to the inflector. And oh, yeah, I still need to provide the test that confirms fixing concern #1.

… matching (the locale postfix should agree with the matched locale prefix)
@bartmcleod
Copy link
Author

Now the tests are there and the logic has been moved to the inflector, where it belongs. Also, the infix introduced has been moved to a class constant.

@boekkooi boekkooi self-requested a review February 18, 2017 08:44
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.

2 participants