The rule ImproveDoctrineCollectionDocTypeInEntityRector does not keep Intersection types

[julienfastre]

When running the rule ImproveDoctrineCollectionDocTypeInEntityRector, the diff produced is this one:

     /**
-     * @var \Doctrine\Common\Collections\Collection<int, \Chill\MainBundle\Entity\User\UserScopeHistory>&Selectable
+     * @var \Doctrine\Common\Collections\Collection<int, \Chill\MainBundle\Entity\User\UserScopeHistory>
      */
     #[ORM\OneToMany(mappedBy: 'user', targetEntity: UserScopeHistory::class, cascade: ['persist', 'remove'], orphanRemoval: true)]
     private Collection&Selectable $scopeHistories;

The problem is that the docblock does not keep the Selectable type. It causes other tools like PHPStan to complaints that some methods are missing in Collection classes.

[bobvandevijver]

I experience the same, also with fully typed Selectable:

-  /** @var Collection<int, Equipment>&Selectable<int, Equipment> */
+  /** @var Collection<int, Equipment> */
   #[ORM\ManyToMany(targetEntity: Equipment::class, mappedBy: 'technologies', fetch: 'EXTRA_LAZY')]
   private Collection&Selectable $equipment; // Default in constructor

This is caused by #339, before that merge this rule was not run for properties with attributes.

[TomasVotruba]

This seem like a fix in wrong place.

See #343 (comment)

[bobvandevijver]

@TomasVotruba I believe you misinterpreted the issue. The issue is that Rector is changing the doc block while it shouldn't. The var type is either already fully correct (my example), or it removes an intersection that should be kept (the first example). Both matched with the actual PHP type (Collection&Selectable), which is no longer the case after this rule touched it.

Doctrine and/or PHPStan can't fix a broken Rector...

[samsonasik]

@TomasVotruba using Selectable means it also support Selectable "matching" method, the current rule remove already exists in docblock Collection&Selectable

-  /** @var Collection<int, Equipment>&Selectable<int, Equipment> */
+  /** @var Collection<int, Equipment> */

which should be kept as is.

[TomasVotruba]

This rule purpose is to turn all collections types into single style.
If you'd like to keep some of them in different way, those should be ignored.

Still, this seems like wrong use of annotation to make single tool happy.

[bobvandevijver]

The rule description disagrees with your point of view:

Improve @var, @param and @return types for Doctrine collections to make them useful both for PHPStan and PHPStorm

From the Doctrine documentation we can see that some collection implementations also implement the Selectable interface, ArrayCollection (doctrine/collections) and PersistentCollection (doctrine/orm) being the most used of those. If you do not need the matching method it is enough to just use Collection, otherwise you need to add the Selectable so you are sure you have something that you can call the matching() method on.

See https://phpstan.org/r/608d3b24-8f23-4861-90ad-787763b5e878 for an analysis example.

Still, this seems like wrong use of annotation to make single tool happy.

The only tool that is not happy here is Rector, which expresses itself by removing useful static analysis information. We want to make sure PHPStorm and PHPStan know that matching() is available, and we need that to be statically typed. This rule is broken in the sense that it removes information that is needed for that purpose: it just didn't pop up earlier because it wasn't doing anything when ORM attributes instead of annotations were used.

Anyways, I have disabled this rule now for my configurations, works for me 🤷🏻‍♂️

[TomasVotruba]

Description is correct, just view is different :)

IMO this should be fixed in Doctrine itself, as some methods are missing in the interface.

[julienfastre]

I do not agree on the way that this issue is closed.

If a collection is typed in PHP with some intersection types, I expect rector to keep all the types in the docblock.

For instance:

     /**
+     * @var \Doctrine\Common\Collections\Collection<int, \Chill\MainBundle\Entity\User\UserScopeHistory>&SomethingElse
      */
     #[ORM\OneToMany(mappedBy: 'user', targetEntity: UserScopeHistory::class, cascade: ['persist', 'remove'], orphanRemoval: true)]
     private Collection&SomethingElse $scopeHistories;

and not:

     /**
+     * @var \Doctrine\Common\Collections\Collection<int, \Chill\MainBundle\Entity\User\UserScopeHistory>
      */
     #[ORM\OneToMany(mappedBy: 'user', targetEntity: UserScopeHistory::class, cascade: ['persist', 'remove'], orphanRemoval: true)]
     private Collection&SomethingElse $scopeHistories;

(IMO, I don't think that there is an issue with Doctrine and the separation of the Collection and Selectable interface. Collection gather the minimal methods required for a readable and writable collection of objects, and Selectable add some features, which are not required everywhere. But I admit that gathering both would ease the usage. This should be discussed on the doctrine's issue tracker, but as I am not convinced that this should change, I don't find the motivation to open an issue there.)

[samsonasik]

@TomasVotruba I created PR #344 if you want to consider