DOCTRINE_CODE_QUALITY + DOCTRINE_COLLECTION_22 breaks ORM\OrderBy attribute usage

[j-dobr]

Setting default order for collection inside entity, ORM\OrderBy is used like this: #[OrderBy(["name" => "ASC"])] (docs)

When using DoctrineSetList::DOCTRINE_CODE_QUALITY, above is replaced with #[OrderBy(["name" => \Doctrine\Common\Collections\Criteria::ASC])] which, while not according to docs, still works.

But the \Doctrine\Common\Collections\Criteria::ASC is deprecated and thus the DoctrineSetList::DOCTRINE_COLLECTION_22 further replaces it with #[OrderBy(["name" => \Doctrine\Common\Collections\Order::Ascending])]. And that breaks as the string is expected there, not enum.

PHPStan: Parameter #1 $value of attribute class Doctrine\ORM\Mapping\OrderBy constructor expects array<string>, array<string,Doctrine\Common\Collections\Order::Ascending> given.

Manually adding value like this #[OrderBy(["name" => \Doctrine\Common\Collections\Order::Ascending->value])] makes it work again (although it still breaks for me on PHP 8.1 and works only with PHP 8.2 (not sure why, might be my setup)). There is still an ongoing discussion regarding the whole \Doctrine\Common\Collections\Order stuff. Main issue being probably this and there are few others touching the topic too, so the situation will probably evolve in future.

Seeing all this, I would suggest to stick to the docs here and avoid replacing the "ASC" / "DESC" in DoctrineSetList::DOCTRINE_CODE_QUALITY as the constants in \Doctrine\Common\Collections\Criteria were never meant to be used for that (source).

[samsonasik]

I didn't look the whole version by version class yet, but that maybe a class on specific version, and enum on next version, @julienfastre could you verify?

the possible way to do is verify if thats a Class, with custom rule, or check the Class on RenameClassConstFetchRector with probably add optional "only_class" flag...

[julienfastre]

I can reproduce this issue, but it's quite unclear to me on how to solve this.

I didn't look the whole version by version class yet, but that maybe a class on specific version, and enum on next version

I think that there are multiple doctrine projects which use the same concept, but differently: doctrine/collections introduced this enum Order, while other projects use string for ordering, and constants (\Doctrine\Common\Collections\Criteria::ASC and \Doctrine\Common\Collections\Criteria::DESC were introduced with the same string value.

Seeing all this, I would suggest to stick to the docs here and avoid replacing the "ASC" / "DESC" in DoctrineSetList::DOCTRINE_CODE_QUALITY as the constants in \Doctrine\Common\Collections\Criteria were never meant to be used for that (https://github.com/doctrine/orm/issues/11313#issuecomment-1969130334).

Reading the links provided by @j-dobr, keeping the ASCand DESC string is the most efficient; we should not replace them by constants. Maybe we should do it soon to avoid bad DX.

I am not sure of this, but I think that we should replace the string ASC and DESC, and the constant Criteria::ASC and Criteria::DESC, only if they are in use as argument in method Criteria::orderBy: see the related PR which says:

Criteria::orderBy() accepts array<string, string|Order>, but passing anything other than array<string, Order> is deprecated.

[johanadivare]

@julienfastre i see it might be a problem in php 8.1 to do the enum with ->value we could confirm this. In my case https://github.com/rectorphp/rector-doctrine/issues/325 i was using php 8.1 and in combination with query builder.

I just tested a project with entity ordering and querybuilder ordering using Order::Descending->value and i got a error for PHP 8.1 Fatal error: Constant expression contains invalid operations in x this was related to the entity

[julienfastre]

@j-dobr the rules regarding those constants were refactored in his MR: https://github.com/rectorphp/rector-doctrine/pull/336

Thanks for reporting this issue.

Can you test it ?

[j-dobr]

@julienfastre now it works as expected. Strings are left intact. Thanks a lot!