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

Make Route normalize HTTP method conditions #14908

Merged
merged 7 commits into from
Aug 19, 2020
Merged

Make Route normalize HTTP method conditions #14908

merged 7 commits into from
Aug 19, 2020

Conversation

markstory
Copy link
Member

By normalizing the http method name to the correct case resource routes are a bit more ergonomic as case mistakes don't cause routes to fail matching.

Fixes #14902

By normalizing the http method name to the correct case resource routes
are a bit more ergonomic as case mistakes don't cause routes to fail
matching.

Fixes #14902
@markstory markstory modified the milestones: 4.1.3, 4.1.4 Aug 15, 2020
Copy link
Member

@othercorey othercorey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok

@ADmad
Copy link
Member

ADmad commented Aug 17, 2020

Don't we have constants for request method types?

@othercorey
Copy link
Member

Are you suggesting we validate the names instead of making them work?

@ADmad
Copy link
Member

ADmad commented Aug 17, 2020

Normally I would say "yes", but in this particular case I guess we don't need to be too pedantic about the casing user uses. Though I was surprised to see strings being used for method types in our codebase too instead of constants.

@othercorey
Copy link
Member

Well. it is bad that users can pass invalid methods. Forget casing, which is east to fix. What if they misspell "post"?

@ADmad
Copy link
Member

ADmad commented Aug 17, 2020

That's why we should provide constants and let those using strings suffer their typos 😛.

@markstory
Copy link
Member Author

We could add constants, and validate that the methods are within the accepted range, but that is a different change that would need to go into 4.next. I would likely put the constants on RouteBuilder so that users wouldn't need to do additional imports if they already are using typehinting.

@dereuromark
Copy link
Member

Those constants already exist for user land code to be used by the way in Cake\Http\Client\Message:

/**
 * HTTP GET method
 *
 * @var string
 */
public const METHOD_GET = 'GET';

/**
 * HTTP POST method
 *
 * @var string
 */
public const METHOD_POST = 'POST';

etc

@markstory
Copy link
Member Author

Those constants already exist for user land code to be used by the way in Cake\Http\Client\Message:

Its a bit odd to need to import the HTTP client when defining routes though.

@dereuromark
Copy link
Member

Sure, just saying: if u wanted you could already use constants.

@ADmad
Copy link
Member

ADmad commented Aug 18, 2020

I added a new normalizeAndValidateMethods() method to get rid of the multiple strtoupper calls scattered across methods and also validate methods names in all arguments.

@ADmad
Copy link
Member

ADmad commented Aug 18, 2020

The test failures are because signature of RedirectRoute::parse(string $url, string $method = '') doesn't match Route::parse(string $url, string $method).

Is there any reason why RedirectRoute has $method as empty string by default? If not we can just remove the default value and update tests to pass a value.

@markstory
Copy link
Member Author

Is there any reason why RedirectRoute has $method as empty string by default? If not we can just remove the default value and update tests to pass a value.

Probably an accident/omission.

@ADmad
Copy link
Member

ADmad commented Aug 18, 2020

Apparently all child methods of Route::parse() which override parse() have a default value for $method and so does RouteCollection::parse(). So removing the default argument would cause lot of test failures.

So I am going to keep the signatures as they are.

Creating the array changes the behavior in ways that make the test suite
sad.
@markstory markstory merged commit b989107 into master Aug 19, 2020
@markstory markstory deleted the fix-14902 branch August 19, 2020 15:18
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.

.json ext giving Missing Method
4 participants