Skip to content

Conversation

@llvdl
Copy link
Contributor

@llvdl llvdl commented May 22, 2016

No description provided.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.007%) to 96.44% when pulling 7e204ed on llvdl:small-fixes into dbd275b on slimphp:3.x.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.007%) to 96.44% when pulling ca50316 on llvdl:small-fixes into dbd275b on slimphp:3.x.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 96.502% when pulling 593280b on llvdl:small-fixes into dbd275b on slimphp:3.x.

@danielgsims
Copy link

There may be a conflict with type hinting the methods as an array. The FastRoute RouteCollector::addRoute method accepts a string or array of strings of http methods. This is the only place I think we use the Route::getMethods function. So technically, even though the phpdoc documents an array of strings, if you use a string everything should work. This could be a BC break.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 96.502% when pulling 1ac6460 on llvdl:small-fixes into dbd275b on slimphp:3.x.

@llvdl
Copy link
Contributor Author

llvdl commented May 23, 2016

@danielgsims, thanks for looking into this. The framework seems to consistently call Route with an array of methods, but the Route constructor is public, so this could be BC.

I have removed the typehint and added a check in the constructor to convert the parameter to an array if a string is passed. This way Route::getMethods always returns an array, as stated in the docblock. The docblock of the constructor is changed so it is clear that either a string or an array can be used.

@danielgsims
Copy link

Maybe go make a note on the Slim 4 road map discussion!

public function __construct($methods, $pattern, $callable, $groups = [], $identifier = 0)
{
$this->methods = $methods;
$this->methods = is_string($methods) ? [$methods] : $methods;
Copy link
Member

Choose a reason for hiding this comment

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

Why add support for string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because FastRoute supports both a string parameter and an array of string. Because there is no array typehint on $methods, string was already supported. I updated the docblock to reflect this.

Also, when setting Route::$methods in the constructor, the value is set to an array, so Route::getMethods() always returns an array as stated in the docblock.

@akrabat akrabat added this to the 3.4.3 milestone Jul 26, 2016
@akrabat akrabat merged commit 1ac6460 into slimphp:3.x Jul 26, 2016
akrabat added a commit that referenced this pull request Jul 26, 2016
@akrabat akrabat modified the milestones: 3.5.0, 3.4.3 Jul 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants