Psalm 5 fixes #10248
Psalm 5 fixes #10248
Conversation
lib/Doctrine/ORM/UnitOfWork.php
Outdated
| default: | ||
| throw new UnexpectedValueException(sprintf( | ||
| 'Unexpected entity state: %s. %s', | ||
| $entityState, | ||
| self::objToStr($entity) | ||
| )); |
There was a problem hiding this comment.
I wouldn't remove these defaults. They're technically dead code, but I'd consider them a defense against a misbehaving getEntityState() implementation. Let's baseline or suppress the error.
On 3.0.x, we could turn the switch into a match and rely on the runtime to error if none of the cases matched.
There was a problem hiding this comment.
Let's baseline or suppress the error.
That will only be doable when using Psalm 5, but OK 👍
lib/Doctrine/ORM/UnitOfWork.php
Outdated
| default: | ||
| throw new UnexpectedValueException(sprintf( | ||
| 'Unexpected entity state: %s. %s', | ||
| $entityState, | ||
| self::objToStr($entity) | ||
| )); |
greg0ire
force-pushed
the
psalm-5-fixes
branch
from
91ffaa7
to
0a839c0
Compare
| @@ -15,6 +15,8 @@ interface CollectionPersister | |||
| /** | |||
| * Deletes the persistent state represented by the given collection. | |||
| * | |||
| * @param PersistentCollection<array-key, object> $collection | |||
There was a problem hiding this comment.
I think template parameters should always be specified, shouldn't they?
There was a problem hiding this comment.
Yes, but you're narrowing the second type parameter from mixed to object.
There was a problem hiding this comment.
I don't remember, and my commit message doesn't help, sorry.
When I try running Psalm v5 with all the errors baselined and then changing object to mixed, I get this error:
ERROR: InvalidArgument - lib/Doctrine/ORM/UnitOfWork.php:425:94 - Argument 1 of Doctrine\ORM\Persisters\Collection\CollectionPersister::delete expects Doctrine\ORM\PersistentCollection<array-key, mixed>, but Doctrine\ORM\PersistentCollection<array-key, object> provided (see https://psalm.dev/004)
$this->getCollectionPersister($collectionToDelete->getMapping())->delete($collectionToDelete);
Maybe the right fix would be to use @template-covariant here… also, I wonder if we should use T of object for as a template for PersistentCollection… a PersistentCollection is always going to be a collection of entities, isn't it?
There was a problem hiding this comment.
Maybe the right fix would be to use
@template-covarianthere…
We can't for the same reasons we've introduced the ReadableCollection. 😕
also, I wonder if we should use
T of objectfor as a template forPersistentCollection… aPersistentCollectionis always going to be a collection of entities, isn't it?
Probably, but then again, do CollectionPersister implementations actually care about the type of the values?
Did you try to declare the methods as generic methods instead, like…
/**
* @template TKey of array-key
* @template TValue
*
* @param PersistentCollection<TKey, TValue> $collection
*
* @return void
*/
public function delete(PersistentCollection $collection);There was a problem hiding this comment.
We can't for the same reasons we've introduced the ReadableCollection.
Ah yeah right, I should know 😓
Probably, but then again, do CollectionPersister implementations actually care about the type of the values?
I suppose they don't, since specifying that we have an object here does not improve the baseline more than silencing the aforementioned error.
Did you try to declare the methods as generic methods instead, like…
That appears to work, thanks a lot! I don't understand why we have this issue only with delete() though. I didn't notice the methods below before noticing the plural to "methods" in your previous message, and now I have mixed feeling about this… probably just going to drop that commit as well, so that we find a better solution later.
greg0ire
force-pushed
the
psalm-5-fixes
branch
from
0a839c0
to
28cb24b
Compare
Found while trying out Psalm 5 the other day. It does not fixes all new issues, but it's a start. Maybe we'll go through the others, maybe we'll add them to the baseline.