Fix more psalm findings #2338
Fix more psalm findings #2338
Conversation
13f84a8 to
9f9c8ee
Compare
| */ | ||
| public function getClassMetadata($className): ClassMetadata | ||
| { | ||
| $metadata = $this->metadataFactory->getMetadataFor($className); | ||
| assert($metadata instanceof ClassMetadata); |
There was a problem hiding this comment.
I added this assert in another PR to make SA tools happy, but since this is a performance sensitive method, I think it's better to just ignore the issue which is related to vimeo/psalm#5788
There was a problem hiding this comment.
People are meant to disable assert in production mode anyway :)
There was a problem hiding this comment.
But given we need @psalm-suppress for two other things I'm fine with this 👍
| @@ -124,7 +124,7 @@ private function createInitializer( | |||
| } | |||
|
|
|||
| /** | |||
| * @return string[] | |||
| * @return array<int, string> | |||
There was a problem hiding this comment.
Change to be in sync with $proxyOptions of https://github.com/Ocramius/ProxyManager/blob/21e2b4aa7d7661e7641cc6362fc8635ddcfa8464/src/ProxyManager/Factory/LazyLoadingGhostFactory.php#L85
| @@ -742,7 +742,7 @@ public function enableShardingForDbByDocumentName(string $documentName): void | |||
| } | |||
| } | |||
|
|
|||
| private function runShardCollectionCommand(string $documentName, ?WriteConcern $writeConcern = null): array | |||
| private function runShardCollectionCommand(string $documentName, ?WriteConcern $writeConcern = null): void | |||
There was a problem hiding this comment.
This value was used prior to #1848, but after that it is not used anymore.
| @@ -213,7 +213,7 @@ final class UnitOfWork implements PropertyChangedListener | |||
| * At the end of the UnitOfWork all these collections will make new snapshots | |||
| * of their data. | |||
| * | |||
| * @psalm-var array<string, PersistentCollectionInterface<array-key, object>> | |||
| * @psalm-var array<string, array<PersistentCollectionInterface<array-key, object>>> | |||
There was a problem hiding this comment.
my fault, it was an array containing another one:
| @@ -1626,6 +1626,8 @@ public function removeFromIdentityMap(object $document): bool | |||
| * @throws InvalidArgumentException If the class does not have an identifier. | |||
| * | |||
| * @template T of object | |||
| * | |||
| * @psalm-suppress InvalidReturnStatement, InvalidReturnType because of the inability of defining a generic property map | |||
There was a problem hiding this comment.
these two suppresses are because the same cause than in #2328 (comment)
| @@ -1046,7 +1046,7 @@ public function prepareQueryOrNewObj(array $query, bool $isNewObj = false): arra | |||
| continue; | |||
| } | |||
|
|
|||
| if (isset($key[0]) && $key[0] === '$' && is_array($value)) { | |||
| if (strpos((string) $key, '$') === 0 && is_array($value)) { | |||
There was a problem hiding this comment.
We were getting:
ERROR: InvalidArrayAccess - lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php:1049:23 - Cannot access array value on non-array variable $key of type array-key (see https://psalm.dev/005)
if (isset($key[0]) && $key[0] === '$' && is_array($value)) {
There was a problem hiding this comment.
Current code is less performant, strpos will go through entire string looking for $ while what we had previously were taking a first char and checking whether it's a $. Instead we should ensure that $key is a string and we'll be ok psalm-wise: https://psalm.dev/r/8e2a12f216
There was a problem hiding this comment.
Makes sense 👍, I had used strpos to avoid the isset check, but didn't think about not having $ and check the entire string.
Instead we should ensure that $key is a string and we'll be ok psalm-wise: https://psalm.dev/r/8e2a12f216
I've casted it to string since it was also done afterwards.
| foreach ($value as $key => $value) { | ||
| if (isset($key[0]) && $key[0] === '$') { | ||
| foreach ($value as $key => $notUsedValue) { | ||
| if (strpos((string) $key, '$') === 0) { |
|
Future me: please go and do #2329 once we merge this one |
|
Thanks @franmomu! |
Summary
Now there are 23 issues in psalm level 6, this PR fixes 15 of them and for the rest I'll update #2329 so I think it would be easier to review.