8000 Initial psalm integration and fixes #108
Conversation
|
Thanks for starting this @SergiuBota1 I don't have time to review right now, but can I suggest you enable Roave BC Checker in CI like this: https://github.com/laminas/laminas-filter/blob/2.41.x/.laminas-ci.json#L5 There are a few BC breaks here that I've noticed which will have to be reverted and BC checker will help identify those. Also, yes, initialising nullable class properties with |
Signed-off-by: bota <Bota@dotkernel.com>
Signed-off-by: bota <Bota@dotkernel.com>
Signed-off-by: bota <Bota@dotkernel.com>
Signed-off-by: bota <Bota@dotkernel.com>
4105334 to
7c64013
Compare
There was a problem hiding this comment.
Thanks @SergiuBota1 - This is just a partial review, but there's a fair bit to go through already and I've run out of time 👍
| xmlns="https://getpsalm.org/schema/config" | ||
| xsi:schemaLocation="https://getpsalm.org/schema/config vendor/vimeo/psalm/config.xsd" | ||
| findUnusedBaselineEntry="true" | ||
| findUnusedCode="true" |
There was a problem hiding this comment.
findUnusedPsalmSuppress is helpful too
| /** @var null|string */ | ||
| protected $version = self::VERSION_11; |
There was a problem hiding this comment.
This is never null as far as I can see. If inheritors make it null, that's their problem
| protected $version = self::VERSION_11; | ||
|
|
||
| /** @var Headers|null */ | ||
| /** @var Headers|null|string */ | ||
| protected $headers; |
There was a problem hiding this comment.
| protected $headers; | |
| protected $headers = null; |
| @@ -51,7 +53,7 @@ public function setVersion($version) | |||
| /** | |||
| * Return the HTTP version for this request | |||
| * | |||
| * @return string | |||
| * @return string|null | |||
There was a problem hiding this comment.
Again, this should never be null
| @@ -75,7 +77,7 @@ public function setHeaders(Headers $headers) | |||
| /** | |||
| * Return the header container responsible for headers | |||
| * | |||
| * @return Headers | |||
| * @return Headers|bool|HeaderInterface|ArrayIterator|mixed | |||
| @@ -359,7 +362,7 @@ public function getFiles($name = null, $default = null) | |||
| * | |||
| * @param string|null $name Header name to retrieve, or null to get the whole container. | |||
| * @param mixed|null $default Default value to use when the requested header is missing. | |||
| 8000 | * @return Headers|bool|HeaderInterface|ArrayIterator | ||
| * @return Headers|bool|HeaderInterface|ArrayIterator|mixed | |||
There was a problem hiding this comment.
/**
* @template T
* @param string|null $name
* @param T $default
* @return ($name is null ? Headers : HeaderInterface|ArrayIterator|T)
*/
All of the getHeaders() implementations can use this doc block - it should solve a fair amount of unnecessary @var annotations and psalm issues
| @@ -386,7 +389,7 @@ public function getHeaders($name = null, $default = false) | |||
| * | |||
| * @param string|null $name Header name to retrieve, or null to get the whole container. | |||
| * @param mixed|null $default Default value to use when the requested header is missing. | |||
| * @return Headers|bool|HeaderInterface|ArrayIterator | |||
| * @return Headers|bool|HeaderInterface|ArrayIterator|mixed | |||
There was a problem hiding this comment.
The docs here can be copied from getHeaders
| return false !== $header && $header->getFieldValue() === 'XMLHttpRequest'; | ||
| $headers = $this->getHeaders(); | ||
|
|
||
| if (! $headers instanceof Headers) { |
There was a problem hiding this comment.
This conditional can be dropped when getHeaders is documented correctly
| return false !== $header && stristr($header->getFieldValue(), ' flash'); | ||
| $headers = $this->getHeaders(); | ||
|
|
||
| if (! $headers instanceof Headers) { |
There was a problem hiding this comment.
This conditional can be dropped when getHeaders is documented correctly
| return false; | ||
| } | ||
|
|
||
| return stripos((string) $header->getFieldValue(), ' flash') !== false; |
There was a problem hiding this comment.
Maybe we should also add a deprecation here. I mean Flash has been dead for a really long time now 😂 - perhaps leave it for a separate patch - this one is already huge
Description
Here is the initial commit for psalm integrations.
I know that there are several unresolved errors, including
PropertyNotSetInConstructor, considering that constructors are not used but setters, should the properties be initialized with null?