Entity Annotation & EntityRepository Template causes Psalm Covariance Issues

[hype09]

Bug Report

When updating our doctrine/orm dependency to 2.12.1 we experienced Psalm issues of the following kind in our CI pipeline:

ERROR: InvalidArgument - src/Common/User/Entity/UserConfig.php:14:27 - Argument 1 of Doctrine\ORM\Mapping\Entity::__construct expects class-string<Doctrine\ORM\EntityRepository<object>>|null, IServ\Auth\Common\User\Repository\DoctrineUserConfigRepository::class provided (see https://psalm.dev/004)
#[Entity(repositoryClass: DoctrineUserConfigRepository::class, readOnly: true)]

Q	A
BC Break	yes?
Version	2.12.1

Summary

This is reproducible for any entity that configures a repository class using the Entity Annotation / Class Attribute if the repository class uses Psalm generics:

#[Entity(repositoryClass: DoctrineUserConfigRepository::class, readOnly: true)]
#[Table(name: "config")]
class UserConfig
{
}

/**
 * @template-extends EntityRepository<UserConfig>
 */
final class DoctrineUserConfigRepository extends EntityRepository
{
}

The issue stems from the Entity Mapping annotation to require a class-string<EntityRepository> here:

orm/lib/Doctrine/ORM/Mapping/Entity.php

Lines 28 to 31 in a552df6

	/**
	* @psalm-param class-string<EntityRepository>|null $repositoryClass
	*/
	public function __construct(?string $repositoryClass = null, bool $readOnly = false)

This breaks due to what I believe to be covariance issues: https://psalm.dev/docs/annotating_code/templated_annotations/#template-covariance

How to reproduce

I have recreated a simplified reproduction case using the Psalm playground: https://psalm.dev/r/db2ba03745

Expected behavior

I would expect the Entity annotation to continue working as it did before the update.

Potential Solution 1

Adding @template-covariant T to the EntityRepository class DocBlock seems to resolve all Psalm issues for us. I am not 100% sure if that has any other implications, though.

Side Note: We use the ServiceEntityRepository class provided by doctrine-bundle, which transparently extends EntityRepository - any solution for this issue thus likely also applies to that class as well (e.g. adding the @template-covariant T there, too).

Potential Solution 2

Maybe the Entity Attribute / Mapping Annotation could also get a generic template parameter, something like this:

diff --git a/app/vendor/doctrine/orm/lib/Doctrine/ORM/Mapping/Entity.php b/app/vendor/doctrine/orm/lib/Doctrine/ORM/Mapping/Entity.php
--- a/app/vendor/doctrine/orm/lib/Doctrine/ORM/Mapping/Entity.php	
+++ b/app/vendor/doctrine/orm/lib/Doctrine/ORM/Mapping/Entity.php
@@ -12,13 +12,14 @@
  * @Annotation
  * @NamedArgumentConstructor()
  * @Target("CLASS")
+ * @template T of object
  */
 #[Attribute(Attribute::TARGET_CLASS)]
 final class Entity implements Annotation
 {
     /**
      * @var string|null
-     * @psalm-var class-string<EntityRepository>|null
+     * @psalm-var class-string<EntityRepository<T>>|null
      */
     public $repositoryClass;
 
@@ -26,7 +27,7 @@
     public $readOnly = false;
 
     /**
-     * @psalm-param class-string<EntityRepository>|null $repositoryClass
+     * @psalm-param class-string<EntityRepository<T>>|null $repositoryClass
      */
     public function __construct(?string $repositoryClass = null, bool $readOnly = false)
     {

This also solves the Psalm issues in our codebase and seems like a less intrusive, more "correct" fix.

[derrabus]

Solution 2 would basically disallow passing EntityRepository itself or any other generic repository implementation as $repositoryClass, wouldn't it?

[hype09]

Hm, yes – seems like you're right. That's obviously not a good way to solve this, then. Do you have any other ideas on how to approach this?

[AndrolGenhald]

I think marking it as covariant is not ideal either, if you have a ParentEntity and ChildEntity extends ParentEntity then you'd want to catch accidental use of ChildRepository on ParentEntity right? It might be a good enough short term fix for most people though.

[michnovka]

Solution 2 would basically disallow passing EntityRepository itself or any other generic repository implementation as $repositoryClass, wouldn't it?

@derrabus I dont think so. As long as the generic repository itself doesnt use PHPDoc

/**
* @extends ServiceEntityRepository<XXXX>
*/

I am in favor of Solution 2, I see no drawbacks

[AndrolGenhald]

@michnovka I think this is the problem, not 100% sure though because I've always used a repository class for each entity instead of using a generic repository.

I'm not sure how to get around that though, class-string<Foo> denotes a string that when constructed will create a Foo, so class-string<Foo<T>> means you need a string that will create a Foo<T> when you construct it, but when Foo is generic Foo::class could be used to construct any Foo<T1> or Foo<T2> that might not be the same as Foo<T>. https://psalm.dev/r/b56bd65dbc

[michnovka]

Is this a valid concern though? Does anybody use the default EntityRepository in this case? Doesnt everybody extend on top of it for each entity? Is there any use case, where a generic Repository like EntityRepository would be passed to Entity attribute?

[michnovka]

As far as I checked, using solution 2 vs current situation does not break anything. Passing EntityRepository::class directly causes the same psalm error in current code already: https://psalm.dev/r/278fd705c6

But adding the template to the Entity attribute does resolve the issues for derived repositories, which imo are 99.99% use-case, if not 100%. Seriously, why would you pass EntityRepository as the repository class for Entity attribute? This will always be some user-code repository in which the programmer can make sure to properly annotate it with @extends PHPDoc

[derrabus]

Seriously, why would you pass EntityRepository as the repository class for Entity attribute?

Because it's possible and it's valid. It could also be a project-specific generic repository implementation that extends EntityRepository. This issue feels like it should be fixed in Psalm and not in this repo.

[derrabus]

It's a bit unfortunate that #9708 got merged before we finished this discussion. I'm opening the issue again because I believe we're not done here.

I think this is the problem

Yes and it's a problem that #9708 broke that scenario. I don't feel comfortable releasing the 2.12.x branch in its current state, tbh.

Regarding the idea to declare covariance on EntityRepository. I've explored this path a bit and I think this is reasonable. The parent interface ObjectRepository uses its template type exclusively for return types. I've created a PR to declare covariance for it: doctrine/persistence#292

EntityRepository however implements Selectable, an interface meant for collections. This is a bit odd because a repository is not a collection. Of course, this is nothing we can change without breaking BC. However, if we declare covariance on EntityRepository, Psalm will complain about implementig Selectable because the template variable of Selectable has to be invariant. But I think, ignoring that error is the lesser evil here.

Is there anything else apart from the Selectable issue that would prevent us from making EntityRepository's template type covariant?

[AndrolGenhald]

@derrabus It looks like it may have been broken before #9708, so #9708 left that case broken but fixed the case of having a specific repository, which seems better than nothing.

Is there anything else apart from the Selectable issue that would prevent us from making EntityRepository's template type covariant?

I'm not sure it actually fixes the problem anyway. I think the only way to support this correctly would be to fix vimeo/psalm#7913 to allow class-strings to be templated correctly.

[derrabus]

@derrabus It looks like it may have been broken before #9708

You're right. The root cause if probably #9274.

I think the only way to support this correctly would be to fix vimeo/psalm#7913 to allow class-strings to be templated correctly.

Yeah, it looks like the current solution is indeed the lesser evil. 😕