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

Move name inflection into routing. #8091

Closed
wants to merge 1 commit into from
Closed

Conversation

markstory
Copy link
Member

This moves inflection into a more 'correct' place, but causes a bunch of other issues like breaking route symmetry between parse() and url().

I feel like the Controller location is less troublesome as the routing layer cares very little about the casing whereas the controller cares about its name being properly inflected.

⚠️ I think this is a bad idea, but I wanted to discuss this and other options in this pull request.

Refs #8085

This moves inflection into a more 'correct' place, but causes a bunch of
other issues like breaking route symmetry between parse() and url().

I feel like the Controller location is less troublesome as the routing
layer cares very little about the casing whereas the controller cares
about its name being properly inflected.

Refs #8085
@markstory markstory added this to the 3.1.8 milestone Jan 22, 2016
@lorenzo
Copy link
Member

lorenzo commented Jan 24, 2016

Isn't using a lowercase controller name in the router always an error? We could just not inflect it, and at the controller actors level throw an exception or at least trigger an error.

I think this is in the top list of problems in cake, one we try to correct with hi sort of code but that often breaks applications in case sensitive file systems

@markstory markstory modified the milestones: 3.1.8, 3.1.9 Jan 24, 2016
@markstory
Copy link
Member Author

The routing layer doesn't care about the the casing of the controller names, outside of the casing being consistent between parsing/matching which is something that would break with this change. Previously the following would work:

Router::connect('/:controller/:action/*');
$url = '/articles/view/1';
$this->assertSame($url, Router::url(Router::parse($url)));

That would no longer be the case after this change, and that is the kind of breakage I'm worried about.

You are right though that controller construction and usage requires a CamelCase controller name.

@lorenzo
Copy link
Member

lorenzo commented Jan 24, 2016

i agree that it is bad to break the symmetry in the router. That is why I suggested triggering an error in the controller factory dispatcher, having to use the inflector is a clear indicator of a user mistake

@ADmad
Copy link
Member

ADmad commented Jan 24, 2016

Reading the original issue #8081 again I see that the problem occurred due to using lower cased controller name examples in url for a url scheme which was using Route class. Since Route class doesn't do any inflection the url should have Examples isn't it? I think in this case the route shouldn't match and an exception should be thrown.

@ADmad
Copy link
Member

ADmad commented Jan 24, 2016

Hmm throwing an exception at router level might not be feasible since child classes could inflecte the URL parts parsed by Route so I guess doing so in controller factory dispatcher as @lorenzo suggested would be the best.

@ADmad
Copy link
Member

ADmad commented Jan 24, 2016

Since we are striving to reduce inflections I think this camelization of plugin name in controller factory should also be removed. DashedRoute already handles inflection of vendor namespaced plugin names properly so just InflectedRoute needs to be fixed.

@markstory
Copy link
Member Author

I think in this case the route shouldn't match and an exception should be thrown.

The route matched because :controller accepts any controller name.

doing so in controller factory dispatcher as @lorenzo suggested would be the best.

I can get behind that. I'll make another pull request.

@markstory markstory closed this Jan 24, 2016
@markstory markstory deleted the route-inflection branch January 24, 2016 17:30
markstory added a commit that referenced this pull request Jan 24, 2016
Controller names that are not CamelCase cause a few issues downstream:

1. They don't work on case-sensitive filesystems.
2. They cause Controller::$name and Controller::$modelClass to be wrong.

To help reduce these cryptic and environment sensitive issues we reject
all controller names that start with a lower case letter.

Refs #8091
Refs #8085
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants