-
Notifications
You must be signed in to change notification settings - Fork 89
PHPStan fixes #296
base: master
Are you sure you want to change the base?
PHPStan fixes #296
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 defaultException::__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
); | ||
} | ||
|
||
$response = $e->getResponse(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Group the two assignments together, then have the two check blocks (easier to read)
* @return false|mixed | ||
*/ | ||
protected function getIdentifier($routeMatch, $request) | ||
protected function getIdentifier(RouteMatch $routeMatch, HttpRequest $request) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a BC break that needs to be reversed
|
||
if (! $viewModel instanceof ModelInterface) { | ||
throw new InvalidArgumentException( | ||
'The supplied View Model is not an instance of ' . ModelInterface::class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add $name
to the exception message
$response = $this->getResponse(); | ||
|
||
if (! $response instanceof HttpResponse) { | ||
throw new Exception\RuntimeException('Response is not an instance of ' . HttpResponse::class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the type of $response
to this exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly an UnexpectedValueException
btw - not a RuntimeException
.
@@ -75,6 +81,10 @@ public function injectTemplate(MvcEvent $e) | |||
|
|||
$template = $this->mapController($controller); | |||
|
|||
if (false === $template) { | |||
throw new RuntimeException('Invalid controller name'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the name of the controller
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to change the phpdoc because mapController()
cannot return a false
value as phpdoc says.
src/View/Http/ExceptionStrategy.php
Outdated
$statusCode = $response->getStatusCode(); | ||
if ($statusCode === 200) { | ||
$response->setStatusCode(500); | ||
} | ||
} else { | ||
throw new RuntimeException('Event response is not an instance of ' . HttpResponse::class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the type of $response
here
src/Service/ApplicationFactory.php
Outdated
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the type of $container
here
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the type of $response
here
Also: Stream
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the type of $response
here
@Ocramius I can add phpstan on CI, but just a low level. Too many errors depends on other libraries. |
@thomasvargiu that's fine - we can raise the level as we get better |
@thomasvargiu btw, I still suggest getting rid of all the |
@Ocramius I'm still working on this. I set this PR as WIP |
Done. Needs a complete review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good improvement, and almost there 👍
Testing the exception class, although it may sound silly, is also something I'd really want here.
phpstan.neon
Outdated
@@ -0,0 +1,15 @@ | |||
parameters: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the phpstan level and the file locations here, so it picks them up by default.
composer.json
Outdated
@@ -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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a helluvalot of added dependencies that may as well fail in production too :|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If they are in ::class
form, then using the string notation is preferable over adding dependencies here
@@ -462,6 +486,14 @@ public function onDispatch(MvcEvent $e) | |||
*/ | |||
public function processPostData(Request $request) | |||
{ | |||
if (! $request instanceof HttpRequest) { | |||
throw new Exception\InvalidArgumentException(sprintf( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same pattern about exception constructor to be applied here as well
/** @var $headerContentType \Zend\Http\Header\ContentType */ | ||
$headerContentType = $request->getHeaders()->get('content-type'); | ||
if (! $request instanceof HttpRequest) { | ||
throw new Exception\InvalidArgumentException(sprintf( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same pattern about exception constructor to be applied here as well
* @return false|mixed | ||
*/ | ||
protected function getIdentifier($routeMatch, $request) | ||
{ | ||
if (! $routeMatch instanceof RouteMatch) { | ||
throw new Exception\InvalidArgumentException(sprintf( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same pattern about exception constructor to be applied here as well
src/MiddlewareListener.php
Outdated
$response = $application->getResponse(); | ||
$serviceManager = $application->getServiceManager(); | ||
|
||
if (! $response instanceof Response) { | ||
throw new RuntimeException('Application response is not an instance of ' . Response::class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same pattern about exception constructor to be applied here as well
throw new UnexpectedValueException('No Request in event'); | ||
} | ||
|
||
if (! $application instanceof Application) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ApplicationInterface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/Service/ApplicationFactory.php
Outdated
@@ -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( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same pattern about exceptions
return []; | ||
} | ||
|
||
if ($config['view_manager'] instanceof ArrayAccess) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Careful: an (array)
cast of an object is potentially destructive. Prefer asserting that it is_array()
instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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());
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return value was used only by private methods, so I changed the signature to allow array|ArrayAccess
src/View/Http/ExceptionStrategy.php
Outdated
$statusCode = $response->getStatusCode(); | ||
if ($statusCode === 200) { | ||
$response->setStatusCode(500); | ||
} | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using an else
, put early break;
statements above
This repository has been closed and moved to laminas/laminas-mvc; a new issue has been opened at laminas/laminas-mvc#9. |
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:
|
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.