Postpone deprecation of string representation of order

[greg0ire]

In downstream packages that representation is recommended in places where type hints do not allow using an enum yet, which means we would have to modify the public API of downstream packages to allow it.

Let us postpone the deprecation until we figure what exactly needs to be done.

More info on this at https://github.com/doctrine/orm/issues/11313

[derrabus]

I not convinced we need to take back the deprecation. Why can't it be fixed downstream?

[greg0ire]

After reading https://github.com/doctrine/orm/issues/11313#issuecomment-1966231735, I believe it would imply changing, among other things, the signature of QueryBuilder::orderBy() as follows:

-    public function addOrderBy(string|Expr\OrderBy $sort, string|null $order = null): static
+    public function addOrderBy(string|Expr\OrderBy $sort, Order|string|null $order = null): static

That's a breaking change for extending classes.

I tried addressing the deprecations yesterday evening, couldn't finish, and I think I wouldn't expect such a change in a minor release.

[derrabus]

Then we don't change this API yet. 🙂

[greg0ire]

Sure, but people will still get the deprecation about Criteria::ASC and when they try to address it, :boom:

[derrabus]

Sure, but people will still get the deprecation about Criteria::ASC and when they try to address it, 💥

They can use Order::Ascending->value instead. But we could also copy those constants over to the ORM repository.

[derrabus]

See doctrine/orm#11315 for a green build in ORM 2.18.x

[greg0ire]

They can use Order::Ascending->value instead.

And then we ask them to migrate to Order::Ascending? That would be two migrations in a short time… not a great UX.

But we could also copy those constants over to the ORM repository.

Maybe it's best if each package has its own constants or enum, yes :thinking: It would avoid this whole situation entirely. In general, a lot of the pain in Doctrine is caused by exposing types of other packages through inheritance or like this, by using them in our public API.

[bobvandevijver]

I got about 200 deprecation notices in a single project because of the deprecation of the Criteria::ASC / Criteria::DESC, and while migrating to Order::Ascending->value might be an option, I do not like having to migrate that twice. Next to that, the deprecation of a class constant is a lot easier to find for certain static analysis tooling compared to deprecating an argument type in the future (for example for the ORM\OrderBy attribute), so I would indeed opt to revert the deprecation for now until at least ORM accepts the new Order enum.

I do like the enum by the way, so kudos on having that option! 👍🏻