PHP 7.1 improvements #126

michalbundyra · 2017-11-30T07:49:02Z

Removed catching Exception, we can now use only Throwable as we support PHP 7.1+

We can now use just Throwable, as we support PHP 7.1+

- added strict types declaration - added typehints - added return types - removed redundant tags in PHPDocs

On previous version spaces around = in declare statement were required

weierophinney · 2017-12-04T22:44:54Z

src/Exception/InvalidMiddlewareException.php

@@ -4,6 +4,7 @@
 * @copyright Copyright (c) 2015 Zend Technologies USA Inc. (http://www.zend.com)
 * @license   https://github.com/zendframework/zend-stratigility/blob/master/LICENSE.md New BSD License
 */
+declare(strict_types=1);


Remove these throughout; we haven't yet made a decision as a project whether or not we want to support strict types yet, so having them here is inconsistent.

Please see my comment here #126 (comment)

I've seen that @Xerkus added it also in his PR to zend-router and zend-mvc v4.

Just because somebody else added to a PR doesn't mean it's accepted practice; in point of fact, I've not had a chance to review any of his patches recently. ;-)

I need to think a bit on your other comment.

@weierophinney

Just because somebody else added to a PR doesn't mean it's accepted practice;

Yes, but actually I think it's good, and I just wanted to point it out, that not only I think like that ;-)

Okay, the argument about loose typing on the string argument convinced me, at least for this component. We'll move forward with that.

weierophinney · 2017-12-04T22:45:20Z

composer.lock

@@ -4,7 +4,7 @@
        "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#composer-lock-the-lock-file",


Why is the lockfile changing?

Because of 2997bc6 - Updated PHP_CodeSniffer.
Old version report errors on declare(strict_types=1); (requires spaces around =)

weierophinney · 2017-12-04T22:47:39Z

src/Middleware/ErrorHandler.php

         * @throws ErrorException if error is not within the error_reporting mask.
         */
-        return function ($errno, $errstr, $errfile, $errline) {
+        return function (int $errno, string $errstr, string $errfile, int $errline) : void {


Does PHP have a problem with this? Is it possible for PHP to send different types at all (such as nulls for the $errfile or $errline when generated from an anonymous class)?

I've checked and all seems to be fine with typehints, see below example:

<?php declare(strict_types=1); $c = new class { public function error() { trigger_error('hello'); } }; set_error_handler(function (int $errno, string $errstr, string $errfile, int $errline) { var_dump('Error', $errno, $errstr, $errfile, $errline); }); $c->error();

https://3v4l.org/LjmEF

void as return type also works fine. It is possible to return bool false, then we would need to change the return type. But as long as we are not doing it, it's fine.

Okay, that's all fine, then.

weierophinney · 2017-12-04T22:49:51Z

src/Route.php

     * @return mixed
     * @throws OutOfRangeException for invalid properties
     */
-    public function __get($name)
+    public function __get(string $name)


Could typehinting the argument cause issues, being a PHP "magic" method type? (Just curious.)

I can't see any issues with it. Documentation says the param type should be string:
http://php.net/manual/en/language.oop5.overloading.php#object.get

Only issue will be when we call this method manually ($class->__get(1)) with non-string value, then we get TypeError, what's right. I think we can keep it here.

weierophinney · 2017-12-04T22:50:51Z

src/Utils.php

    {
-        if (($error instanceof Throwable || $error instanceof Exception)
+        if (null !== $error


$error can never be null here, as it has a typehint that isn't nullable...

Please see my todo comment in line 28-29, I think before it was possible to pass null here.
You have right, my change is inconsistent, and we should decide what to do. Do we even need this class in Stratigility as it is no used?

The reason we broke this into the static class method was due to folks wanting to re-use that functionality in their own response generators; they were having to copy-paste previously as ErrorResponseGenerator is marked final.

ErrorResponseGenerator::__invoke() receives either a Throwable or Exception as the first argument, always, so $error here will always be a Throwable under PHP 7.

As such, we can get rid of the null !== $error condition and keep the method.

Ok, will change it then. Thanks for explanations. Changed in: 32b5928

weierophinney · 2017-12-04T22:51:40Z

test/RouteTest.php

@@ -49,7 +50,7 @@ public function nonStringPaths()
     */
    public function testDoesNotAllowNonStringPaths($path)


We can likely remove this method entirely, as it existed pre-typehints.

Hm... yes and no... Please try:

<?php // declare(strict_types=1); function test(string $x) { var_dump($x, gettype($x)); } test(1); test('str'); test(true);

and then uncomment line with declare(strict_types=1);.
This is why I've added this declaration and this is why I kept this test.

There is silent conversion between simple types, but for array, object, or null it will fail (TypeError exception). When we declare strict types we do not allow silent conversion, and this is want we want, IMHO.

michalbundyra · 2017-12-05T10:17:53Z

test/MiddlewarePipeTest.php

@@ -117,7 +118,7 @@ public function testReturnsResponseReturnedByQueue()
        $this->assertSame($return, $result, var_export([
            spl_object_hash($return) => get_class($return),
            spl_object_hash($result) => get_class($result),
-        ], 1));
+        ], true));


This change was needed because of strict_types declaration.

That, and using a truthy value versus the expected boolean was just sloppy on my part. :)

@weierophinney

- per @weierophinney

PHP 7.1 improvements

Close #126

michalbundyra added 5 commits November 30, 2017 07:47

Removed catch Exception

da504ea

We can now use just Throwable, as we support PHP 7.1+

PHP 7.1 strict types

ded9c81

- added strict types declaration - added typehints - added return types - removed redundant tags in PHPDocs

Updated PHP_CodeSniffer in lock file

2997bc6

On previous version spaces around = in declare statement were required

Fixed type in PHPDoc

74b255c

Removed not used imports

c096c5e

weierophinney suggested changes Dec 4, 2017

View reviewed changes

michalbundyra commented Dec 5, 2017

View reviewed changes

Removed redundant check - Throwable cannot be null

32b5928

- per @weierophinney

weierophinney merged commit 32b5928 into zendframework:release-3.0.0 Dec 5, 2017

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

Merge pull request #126 from webimpress/php-7.1

9bf5bbc

PHP 7.1 improvements

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

Merge branch 'feature/126-7.1-updates' into release-3.0.0

5372e87

Close #126

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

michalbundyra deleted the php-7.1 branch December 5, 2017 15:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHP 7.1 improvements #126

PHP 7.1 improvements #126

michalbundyra commented Nov 30, 2017

weierophinney Dec 4, 2017

michalbundyra Dec 5, 2017

weierophinney Dec 5, 2017

michalbundyra Dec 5, 2017

weierophinney Dec 5, 2017

weierophinney Dec 4, 2017

michalbundyra Dec 5, 2017

weierophinney Dec 4, 2017

michalbundyra Dec 5, 2017

weierophinney Dec 5, 2017

weierophinney Dec 4, 2017

michalbundyra Dec 5, 2017

weierophinney Dec 5, 2017

weierophinney Dec 4, 2017

michalbundyra Dec 5, 2017

weierophinney Dec 5, 2017

michalbundyra Dec 5, 2017

weierophinney Dec 4, 2017

michalbundyra Dec 5, 2017

michalbundyra Dec 5, 2017

weierophinney Dec 5, 2017

		@@ -4,7 +4,7 @@
		"Read more about it at https://getcomposer.org/doc/01-basic-usage.md#composer-lock-the-lock-file",

		@@ -49,7 +50,7 @@ public function nonStringPaths()
		*/
		public function testDoesNotAllowNonStringPaths($path)

PHP 7.1 improvements #126

PHP 7.1 improvements #126

Conversation

michalbundyra commented Nov 30, 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

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

Choose a reason for hiding this comment