PHP 7.1 - typehints, return types, strict types #41

michalbundyra · 2017-12-05T18:46:14Z

No description provided.

michalbundyra · 2017-12-05T18:48:12Z

src/Route.php

     * @throws Exception\InvalidArgumentException for invalid path type.
     * @throws Exception\InvalidArgumentException for invalid middleware type.
     * @throws Exception\InvalidArgumentException for any invalid HTTP method names.
     */
-    public function __construct($path, $middleware, $methods = self::HTTP_METHOD_ANY, $name = null)
+    public function __construct(string $path, $middleware, $methods = self::HTTP_METHOD_ANY, string $name = null)


Would be nice if we could typehint on $middleware and $methods.
I've seen somewhere inconsistent with $methods, I think in some place I've seen =null which was translated later on to ANY.

What with $middleware. Is it not now just MiddlewareInterface?

I was just thinking about this myself when I reviewed your earlier patch, and just dove into how zend-expressive uses it.

At this point, within zend-expressive itself, we always pass a middleware instance, as Application::prepareMiddleware() creates an instance based on the argument passed to Application::route(). As such, I think we could likely update this to have a typehint.

We cannot typehint $methods currently, as it can be either an array of HTTP method names or the HTTP_METHOD_ANY constant, which is an integer. I did it this way as the HTTP specification allows arbitrary method names, so there's no way to create a "master list" representing all methods.

Updated to MiddlewareInterface typehint on $middleware.

About $methods, please see:
https://github.com/zendframework/zend-expressive/pull/530/files#diff-c9248b3167fc44af085b81db2e292837R283
So there we have array $methods = null and then we have:

$methods = null === $methods ? Router\Route::HTTP_METHOD_ANY : $methods;

Maybe we should do the same?
I'm not sure if I like this behaviour (null means any, array defines method explicitly, so emoty array means none).
Similar solution we have on mocking in PHPUnit - setMethods() - we can pass null|array and it has different behaviour with null and empty array.

I would prefer to have it consistent - the same in expressive and expressive-router.

Nullable would work here, I guess; we'd have to change the value of the constant to be null, however, to ensure there's no breakage for folks who use the constant to specify the value. That would allow ?array $methods = null in the signature.

michalbundyra · 2017-12-05T20:14:27Z

test/RouteResultTest.php

@@ -45,7 +46,7 @@ public function testRouteFailureRetrieveAllHttpMethods()
    public function testRouteFailureRetrieveHttpMethods()
    {
        $result = RouteResult::fromRouteFailure();
-        $this->assertSame([], $result->getAllowedMethods());
+        $this->assertSame(['*'], $result->getAllowedMethods());


Behaviour here has been changed, we can set default value to [] and typehint on ?array in fromRouteFailure method to keep the previous functionality.
@weierophinney What do you think?

I'd keep it consistent.

We need to make sure that the consumers (the individual routing libraries) are able to upgrade without issue. So long as the same argument types are accepted, how the default works is less important.

That said, [] seems VERY DIFFERENT than ['*']; the first looks like an empty set (no HTTP methods specified), while the latter looks like it accepts a "wildcard" (i.e., any method).

I think in this case, we need to look at the RouteMiddleware in Expressive to determine what it expects.

I can't see any usages of fromRouteFailure method in Expressive.

Look in the individual routers, then; just looked, and the FastRouter bridge definitely calls it.

It is changed to keep the previous functionality, defailt value (mow empty array) will set allowed methods to none

`null` value is no longer possible

weierophinney · 2017-12-05T22:10:39Z

src/RouterInterface.php

     * @throws Exception\RuntimeException when called after match() or
     *     generateUri() have been called.
     */
-    public function addRoute(Route $route);
+    public function addRoute(Route $route) : void;


Ooof. The signature changes in this interface will require new major versions of each router unless we update them to have return values now (return types are covariant, so an implementation can define a more specific return type).

I'll experiment with that momentarily on the zendrouter repo.

weierophinney · 2017-12-05T22:14:41Z

src/RouterInterface.php

     * @throws Exception\RuntimeException if unable to generate the given URI.
     */
-    public function generateUri($name, array $substitutions = [], array $options = []);
+    public function generateUri(string $name, array $substitutions = [], array $options = []) : string;


And this one requires a major version bump, as the typehint of an argument has changed.

I think we should limit the changes in this interface to return value typehints for now, as it will result in fewer moving parts with regards to coordinating releases.

We can revisit after we have all the various repos updated.

As we allow bind only MiddlewareInterface the method will return only MiddlewareInterface instances

weierophinney

Per a conversation on Slack, we agreed that adding the typehints to the interface makes sense, as it solidifies and makes explicit the contract by which implementations must abide. This will, of course, require major-version bumps in each implementation.

weierophinney · 2017-12-06T20:50:36Z

.docheader

@@ -3,3 +3,4 @@
 * @copyright Copyright (c) %regexp:(20\d{2}-)?20\d{2}% Zend Technologies USA Inc. (http://www.zend.com)
 * @license   https://github.com/zendframework/zend-expressive-router/blob/master/LICENSE.md New BSD License
 */
+declare(strict_types=1);


IIRC, we need one empty line between the file-level docblock and any directives, per PSR-12. I'll update those when I merge.

Close #41

weierophinney · 2017-12-06T21:21:12Z

Merged to release-3.0.0 branch; not sure why it didn't auto-close.

michalbundyra added 6 commits December 5, 2017 18:25

Added declare strict types

38ebd9a

Removed redundant default values, these are set in constructor anyway

425b50e

Added typehints and return types

b9a87cf

Updated see links

3b59d2f

Removed redundant bracket

0529d35

Simplify if statement

d1a9cd6

michalbundyra commented Dec 5, 2017

View reviewed changes

weierophinney added this to the 3.0.0alpha1 milestone Dec 5, 2017

michalbundyra added 4 commits December 5, 2017 19:42

Added MiddlewareInterface typehint on $middleware in Route class

3a64995

Added constants visibility to public

05b729a

CS fix

d69c3fd

Methods null means any - added array typehint on $methods

864c7dd

michalbundyra commented Dec 5, 2017

View reviewed changes

michalbundyra added 2 commits December 5, 2017 20:44

Updated fromRouteFailure signature: nullable array type hint

8f7462b

It is changed to keep the previous functionality, defailt value (mow empty array) will set allowed methods to none

Updated default value of $allowedMethods to empty array

a77e5e3

`null` value is no longer possible

weierophinney reviewed Dec 5, 2017

View reviewed changes

weierophinney suggested changes Dec 5, 2017

View reviewed changes

michalbundyra added 2 commits December 6, 2017 07:13

PHPDocs updates

276c3ad

Added missing return type on getMiddleware method in Route class

5b5c921

As we allow bind only MiddlewareInterface the method will return only MiddlewareInterface instances

weierophinney approved these changes Dec 6, 2017

View reviewed changes

Added blank line after file-level docblock

d615c31

weierophinney added a commit that referenced this pull request Dec 6, 2017

Adds CHANGELOG entries for #41

44f8728

weierophinney added a commit that referenced this pull request Dec 6, 2017

Merge branch 'feature/41-php71-typehints' into release-3.0.0

6b2cfc3

Close #41

weierophinney closed this Dec 6, 2017

michalbundyra deleted the feature/php-7.1 branch December 6, 2017 21:21

michalbundyra mentioned this pull request Jan 29, 2018

Add ConfigProvider zendframework/zend-expressive-aurarouter#30

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHP 7.1 - typehints, return types, strict types #41

PHP 7.1 - typehints, return types, strict types #41

michalbundyra commented Dec 5, 2017

michalbundyra Dec 5, 2017

weierophinney Dec 5, 2017

michalbundyra Dec 5, 2017

weierophinney Dec 5, 2017

michalbundyra Dec 5, 2017

weierophinney Dec 5, 2017

michalbundyra Dec 5, 2017

weierophinney Dec 5, 2017

weierophinney Dec 5, 2017

weierophinney Dec 5, 2017

weierophinney left a comment

weierophinney Dec 6, 2017

weierophinney commented Dec 6, 2017

PHP 7.1 - typehints, return types, strict types #41

PHP 7.1 - typehints, return types, strict types #41

Conversation

michalbundyra commented Dec 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney commented Dec 6, 2017