PHPStan fixes #296

thomasvargiu · 2018-10-13T13:15:46Z

In order to bump to PHP 7 minimum version in the future, I used phpstan to static analyse the code, correcting some phpdoc errors and adding guards throwing exceptions when needed. Code quality will be improved when other components will be improved.

It's just a first step, but I think it will be useful.

I don't know if this could be considered a BC break, probably it isn't.

Ocramius

Overall good improvement, but:

there's a lot of constructor code landing in here, making things complex to understand/follow. Strongly endorse using ExceptionType::fromSomething($parameters) instead of the default Exception::__construct() API, which is quite fragile
can we add phpstan to a build step? Would need to be locked to a specific version of phpstan

src/Controller/AbstractRestfulController.php

Ocramius · 2018-10-15T10:57:44Z

src/Controller/AbstractRestfulController.php

+            );
+        }
+
+        $response = $e->getResponse();


Group the two assignments together, then have the two check blocks (easier to read)

Ocramius · 2018-10-15T10:59:02Z

src/Controller/AbstractRestfulController.php

     * @return false|mixed
     */
-    protected function getIdentifier($routeMatch, $request)
+    protected function getIdentifier(RouteMatch $routeMatch, HttpRequest $request)


This is a BC break that needs to be reversed

Ocramius · 2018-10-15T10:59:36Z

src/Controller/Plugin/AcceptableViewModelSelector.php

+
+        if (! $viewModel instanceof ModelInterface) {
+            throw new InvalidArgumentException(
+                'The supplied View Model is not an instance of ' . ModelInterface::class


Please add $name to the exception message

Ocramius · 2018-10-15T11:00:02Z

src/Controller/AbstractRestfulController.php

+        $response = $this->getResponse();
+
+        if (! $response instanceof HttpResponse) {
+            throw new Exception\RuntimeException('Response is not an instance of ' . HttpResponse::class);


Please add the type of $response to this exception

Possibly an UnexpectedValueException btw - not a RuntimeException.

Ocramius · 2018-10-15T11:02:45Z

src/View/Http/InjectTemplateListener.php

@@ -75,6 +81,10 @@ public function injectTemplate(MvcEvent $e)

        $template = $this->mapController($controller);

+        if (false === $template) {
+            throw new RuntimeException('Invalid controller name');


Please add the name of the controller

I'm going to change the phpdoc because mapController() cannot return a false value as phpdoc says.

Ocramius · 2018-10-15T11:02:57Z

src/View/Http/ExceptionStrategy.php

                    $statusCode = $response->getStatusCode();
                    if ($statusCode === 200) {
                        $response->setStatusCode(500);
                    }
+                } else {
+                    throw new RuntimeException('Event response is not an instance of ' . HttpResponse::class);


Please add the type of $response here

Ocramius · 2018-10-15T11:03:17Z

src/Service/ApplicationFactory.php

@@ -26,6 +28,10 @@ class ApplicationFactory implements FactoryInterface
     */
    public function __invoke(ContainerInterface $container, $name, array $options = null)
    {
+        if (! $container instanceof ServiceManager) {
+            throw new InvalidArgumentException('Container should be an instance of ' . ServiceManager::class);


Please add the type of $container here

Ocramius · 2018-10-15T11:03:36Z

src/ResponseSender/SimpleStreamResponseSender.php

@@ -23,9 +24,16 @@ public function sendStream(SendResponseEvent $event)
            return $this;
        }
        $response = $event->getResponse();
+
+        if (! $response instanceof Stream) {
+            throw new RuntimeException('Event is not an instance of ' . Stream::class);


Please add the type of $response here

Also: Stream?

Yes, it's the only response type with getStream() method.

There is a chain to send the response, the first one is PhpEnvironmentResponseSender which has a method to check if the event response is an instance of Zend\Http\PhpEnvironment\Response.

I added the check also for the SimpleStreamResponseSender.

Ocramius · 2018-10-15T11:03:43Z

src/ResponseSender/AbstractResponseSender.php

@@ -25,11 +28,16 @@ public function sendHeaders(SendResponseEvent $event)

        $response = $event->getResponse();

+        if (! $response instanceof Response) {
+            throw new RuntimeException('Event response should be an instance of ' . Response::class);


Please add the type of $response here

thomasvargiu · 2018-10-15T13:06:25Z

@Ocramius I can add phpstan on CI, but just a low level. Too many errors depends on other libraries.

Ocramius · 2018-10-15T13:16:29Z

@thomasvargiu that's fine - we can raise the level as we get better

Ocramius · 2018-10-15T14:43:32Z

@thomasvargiu btw, I still suggest getting rid of all the throw new SomeException($someString), and having throw SomeException::unexpectedType($expected, $actual) instead

thomasvargiu · 2018-10-15T14:44:50Z

@Ocramius I'm still working on this. I set this PR as WIP

thomasvargiu · 2018-10-15T18:16:33Z

Done. Needs a complete review

Ocramius

Good improvement, and almost there 👍

Testing the exception class, although it may sound silly, is also something I'd really want here.

Ocramius · 2018-10-15T19:17:24Z

phpstan.neon

@@ -0,0 +1,15 @@
+parameters:


Please add the phpstan level and the file locations here, so it picks them up by default.

Ocramius · 2018-10-15T19:18:02Z

composer.json

@@ -30,8 +31,15 @@
        "http-interop/http-middleware": "^0.4.1",
        "phpunit/phpunit": "^6.4.4 || ^5.7.14",
        "zendframework/zend-coding-standard": "~1.0.0",
+        "zendframework/zend-filter": "^2.6",


That's a helluvalot of added dependencies that may as well fail in production too :|

There are classes used in LazyControllerAbstractFactory. The static analyser throws an exception if they are not found. We can convert the ::class notation in strings, probably is the best thing to do.

Btw, they are included only in require-dev directive so shouldn't be a problem in production.

If they are in ::class form, then using the string notation is preferable over adding dependencies here

Ocramius · 2018-10-15T19:19:07Z

src/Controller/AbstractRestfulController.php

@@ -462,6 +486,14 @@ public function onDispatch(MvcEvent $e)
     */
    public function processPostData(Request $request)
    {
+        if (! $request instanceof HttpRequest) {
+            throw new Exception\InvalidArgumentException(sprintf(


Same pattern about exception constructor to be applied here as well

Ocramius · 2018-10-15T19:19:12Z

src/Controller/AbstractRestfulController.php

-        /** @var $headerContentType \Zend\Http\Header\ContentType */
-        $headerContentType = $request->getHeaders()->get('content-type');
+        if (! $request instanceof HttpRequest) {
+            throw new Exception\InvalidArgumentException(sprintf(


Same pattern about exception constructor to be applied here as well

Ocramius · 2018-10-15T19:19:17Z

src/Controller/AbstractRestfulController.php

     * @return false|mixed
     */
    protected function getIdentifier($routeMatch, $request)
    {
+        if (! $routeMatch instanceof RouteMatch) {
+            throw new Exception\InvalidArgumentException(sprintf(


Same pattern about exception constructor to be applied here as well

Ocramius · 2018-10-15T19:21:09Z

src/MiddlewareListener.php

        $response       = $application->getResponse();
        $serviceManager = $application->getServiceManager();

+        if (! $response instanceof Response) {
+            throw new RuntimeException('Application response is not an instance of ' . Response::class);


Same pattern about exception constructor to be applied here as well

Ocramius · 2018-10-15T19:21:37Z

src/MiddlewareListener.php

+            throw new UnexpectedValueException('No Request in event');
+        }
+
+        if (! $application instanceof Application) {


ApplicationInterface

There is a method (marshalInvalidMiddleware) called some lines lower, where the signature require an Application instance. The interface would be enough but to avoid a BC break I can't change the signature.

Ocramius · 2018-10-15T19:22:15Z

src/Service/ApplicationFactory.php

@@ -26,6 +28,14 @@ class ApplicationFactory implements FactoryInterface
     */
    public function __invoke(ContainerInterface $container, $name, array $options = null)
    {
+        if (! $container instanceof ServiceManager) {
+            throw new InvalidArgumentException(sprintf(


Same pattern about exceptions

Ocramius · 2018-10-15T19:23:33Z

src/Service/HttpViewManagerConfigTrait.php

+            return [];
+        }
+
+        if ($config['view_manager'] instanceof ArrayAccess) {


Careful: an (array) cast of an object is potentially destructive. Prefer asserting that it is_array() instead

Thre is a problem with the config service in zend-mvc. Somewhere is declared as array|ArrayAccess, but then is passed on methods that accept just an array. This will be a problem with strict types.

I would prefer to return just an array. You're right, it's dangerous. What about to convert it iterating the array?

That's perfectly valid, but an (array) cast produces something quite broken: https://3v4l.org/OTFXt

<?php class A implements ArrayAccess { private $potato = 'haha'; public function offsetExists($offset){} public function offsetGet($offset){} public function offsetSet ($offset, $value){} public function offsetUnset ($offset){} } var_dump((array) new A());

I know. Do you agree that should be better to return just an array? In order to use strict types in the future I think should be better IMHO.

I think it's not possible to convert safely an object implementing ArrayAccess in array, so I should revert it, but it's a problem to handle it in other classes

The return value was used only by private methods, so I changed the signature to allow array|ArrayAccess

Ocramius · 2018-10-15T19:24:48Z

src/View/Http/ExceptionStrategy.php

                    $statusCode = $response->getStatusCode();
                    if ($statusCode === 200) {
                        $response->setStatusCode(500);
                    }
+                } else {


Instead of using an else, put early break; statements above

weierophinney · 2019-12-31T21:40:51Z

This repository has been closed and moved to laminas/laminas-mvc; a new issue has been opened at laminas/laminas-mvc#9.

weierophinney · 2019-12-31T21:42:12Z

This repository has been moved to laminas/laminas-mvc. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

Squash all commits in your branch (git rebase -i origin/{branch})
Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
Run the laminas/laminas-migration tool on the code.
Clone laminas/laminas-mvc to another directory.
Copy the files from the second bullet point to the clone of laminas/laminas-mvc.
In your clone of laminas/laminas-mvc, commit the files, push to your fork, and open the new PR.
We will be providing tooling via laminas/laminas-migration soon to help automate the process.

PHPStan fixes

3a1a397

Ocramius suggested changes Oct 15, 2018

View reviewed changes

Ocramius assigned thomasvargiu Oct 15, 2018

Ocramius added the enhancement label Oct 15, 2018

Better exception messages

1808073

thomasvargiu changed the title ~~PHPStan fixes~~ WIP: PHPStan fixes Oct 15, 2018

thomasvargiu added 11 commits October 15, 2018 17:03

PHPStan 0.10 fixes

00dd89c

Added phpstan in CI

4c52699

PHPCS

940576a

Use of UnexpectedValueException::unexpectedType()

d234371

Fixed travis config

ce87b2e

Fixed exception

f2ef81b

Updated locked dependencies on PHP 7.1

6022ee2

Locked dependencies on PHP 5.6

6d5733d

Added HttpDefaultRenderingStrategyFactoryTest

498163d

Fixed return type

4ab30bf

Removed debug code

bbefcb8

thomasvargiu changed the title ~~WIP: PHPStan fixes~~ PHPStan fixes Oct 15, 2018

Ocramius suggested changes Oct 15, 2018

View reviewed changes

thomasvargiu added 6 commits October 15, 2018 22:28

phpstan parameters in phpstan.neon config

ba0fb92

Exceptions test

511962e

Improved exceptions an readability

7f20264

Changed signature to allow ArrayAccess|array argument

09a0ebe

Replaced ::class notation with strings in order to reduce dependencies

404bad3

Removed useless dependencies

3845b73

Replaced PHPUnit TestCase class

5f40f54

weierophinney mentioned this pull request Dec 31, 2019

PHPStan fixes laminas/laminas-mvc#9

Closed

PHPStan fixes #296

Are you sure you want to change the base?

PHPStan fixes #296

Conversation

thomasvargiu commented Oct 13, 2018

Ocramius left a 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

thomasvargiu commented Oct 15, 2018

Ocramius commented Oct 15, 2018

Ocramius commented Oct 15, 2018

thomasvargiu commented Oct 15, 2018

thomasvargiu commented Oct 15, 2018

Ocramius left a 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

weierophinney commented Dec 31, 2019

weierophinney commented Dec 31, 2019