Use normal Collection<ClassName> when property has indexBy

[samsonasik]

Avoid using <int> index when property has indexBy.

Fixes rectorphp/rector#8791
Fixes #357

[samsonasik]

Fixed 🎉 /cc @AlexeyKosov @yguedidi

[samsonasik]

All checks have passed 🎉 @TomasVotruba I am merging it ;)

[yguedidi]

Thanks @samsonasik !!
Maybe better to detect the type of the indexed by referenced property, and put either int or string as collection key type?

[samsonasik]

@yguedidi that's will be too much lookup :), using Collection<ClassName> is just fine for it

[yguedidi]

Then maybe at least skip changing if there is already an index in the type?

[samsonasik]

that will be too much for that :)

[yguedidi]

@samsonasik the thing is that I don't want Rector to change my Collection<string, ClassName> to Collection<ClassName> 😅

[samsonasik]

Feel free to skip the rule.

[TomasVotruba]

Both values have to filled Collection<key, value>, as first is key, next is value, see:

https://github.com/doctrine/collections/blob/2790cb57e6510e4404873bdca6688e447550d720/src/Collection.php#L29-L30

[samsonasik]

per @AlexeyKosov at issue rectorphp/rector#8791 (comment)

the key is not necessarily need to be defined, we use \Doctrine\Common\Collections\Collection<ClassName> too in our project, and working well.

[TomasVotruba]

That doesn't respect the parent interface contract:

should have the Collection<string, Training> type rather than Collection<int, Training>.

Both key and value should be included.

[samsonasik]

I think for win win solution, skip php doc change is better if there is indexBy key, since lookup target class -> property type defined by indexBy may or may not work.

I will create new PR

[samsonasik]

see new PR:

• [CodeQuality] Skip change doctrine @var/@return annotation with indexBy key in annotation/attribute #361

[yguedidi]

Feel free to skip the rule.

I know it's possible, but I find it weird that Rector will break a more precise type (the one with key type define), and in order to avoid that, we have to make our config more complex to maintain by adding a skip configuration (either full rule, either specific to few files)
I think, as a general behavior, rectors shouldn't degrade existing type definitions if they are more precise than what the rectors changes are capable of

[samsonasik]

It only change when it has unioned type as far as I see: @var Collection|Training[] -> @var Collection<Training>

[yguedidi]

in my case I have something like

    /**
     * @var Collection<string, Training>
     */
    #[ORM\OneToMany(targetEntity: Training::class, indexBy: "id", mappedBy: "trainer")]
    private Collection $trainings;

that got transformed to

    /**
     * @var Collection<int, Training>
     */
    #[ORM\OneToMany(targetEntity: Training::class, indexBy: "id", mappedBy: "trainer")]
    private Collection $trainings;

[samsonasik]

@yguedidi that's correctly skipped, see PR fixture

• Skip correct Collection<string, Training> fixture #360