[RFC] Follow-up on doctrine/collections Order enum

[greg0ire]

In doctrine/collections#389, I introduced a new enum: Order

What should we use it for inside the ORM?

• The OrderBy attribute: this is a clear use case where we should switch to that enum, since it has to do with collections
• QueryBuilder::orderBy() / addOrderBy(): this one is less clear to me… when you select several entities, you don't get a collection, but a list, right?
• DQL : I don't think we should allow it inside DQL, it's cumbersome
• Expr\OrderBy: I'm not sure we should do it here either.

@derrabus @SenseException any thoughts?

Cc @ThomasLandauer

[ThomasLandauer]

QueryBuilder::orderBy() / addOrderBy() was the place where I initially suggested to get rid of those strings (#11263) - so this is a must IMO :-)

[ThomasLandauer]

Anybody who was using ->orderBy('foo', Criteria::ASC) is kinda stuck now (after upgrading to doctrine/collections 2.2.0). PHPStan is going crazy with:

Fetching deprecated class constant ASC of class Doctrine\Common\Collections\Criteria: use Order::Ascending instead

But the enum isn't working yet:

Parameter #2 $order of method Doctrine\ORM\QueryBuilder::orderBy() expects string|null, Doctrine\Common\Collections\Order::Ascending given.

This can be overcome by using Order::Ascending->value (as you said in doctrine/collections#389 (comment)), but if the current situation is going to stay for longer than just a few days, then the deprecation message should be changed to:

... use Order::Ascending or Order::Ascending->value instead

[greg0ire]

@ThomasLandauer using Order::Ascending->value would solve the error, but there would still be a deprecation because ultimately, it's a string. We need to modify our public API to allow Order. Worse, it would mean users would have to first migrate to Order::Ascending->value, and then to Order::Ascending. 2 migrations instead of one.

@derrabus in doctrine/collections#389 (comment), I suggested we give the ORM a chance to upgrade, but I failed to make my point, so let me elaborate. What makes that upgrade so special that we should change the way we do things?

I think the main difference with other PRs where we add an API and immediately deprecate the other API is that we expect the users of doctrine/orm to use doctrine/collections's API when interacting with our own API, and don't support (yet) using the non-deprecated API. I suggest we remove the deprecation layer until we and probably the ODM folks (Cc @malarzm) figure out what changes to conduct in the ORM and the ODM. Or at least, the parts that could be referred to by our users, such as the Criteria::* constants.

[derrabus]

It's a bit unfortunate that the ORM uses constants from a different package in their public API. For ORM 2.19, we could add replacement constants to the ORM codebase. Supporting the Order enum directly is a feature I don't see for ORM 2.x anymore, tbh.

[derrabus]

Also, we have to be careful: Collections is not the only abstraction that we're dealing with here. When talking about the query builder, we're probably closer to the Persistence abstraction which also declares orderings as strings at the moment:

https://github.com/doctrine/persistence/blob/e7cf418cafe83a75f7a7096cc9fa890b201d28f1/src/Persistence/ObjectRepository.php#L42-L43

It would be a bit weird if the query builder and the repository used different abstractions for ordering stuff.

[NMe84]

The OrderBy attribute: this is a clear use case where we should switch to that enum, since it has to do with collections

I wouldn't switch, I'd support both for a while to give users time to adjust their code. Should be easy enough to accept string|Order as a parameter, right?

That said, the current situation leaves people with either a deprecation (which is very annoying in PhpUnit) or very ugly code in the form of something like this:

#[OrderBy(['id' => Order::Ascending->value])]

...where previously this much shorter and more readable version was usable:

#[OrderBy(['id' => Criteria::ASC])]

Supporting the Order enum directly is a feature I don't see for ORM 2.x anymore, tbh.

Then either the deprecation should go (because you can't fix the deprecation in an acceptable way in 2.x) or the functions requiring a string should also accept the enum, as I just suggested.

[curry684]

Supporting the Order enum directly is a feature I don't see for ORM 2.x anymore, tbh.

...thus forcing a double migration on all of us, migrating to Order::Ascending->value first in 2.x, and then breaking/deprecating all our code again when making the switch to 3?

[derrabus]

I mean, we could un-deprecate the constants, which basically means that Collections 3 would ship two constants it has no use for. I could live with that.

[curry684]

Preferably the constants would remain gone in 3 as you're otherwise going to be stuck with them sort of forever. I would just like a better migration path than we have now. For example (without really looking into the current code) we could consider introducing the Order enum as a static final class in 2.x, so Order::Ascending actually is a real string, and then in 3.x leave it as a real enum which is handled correctly as an enum everywhere. With something like that the final 2.x release would present a real upgrade path to 3 while being code-wise compatible.

This would interlock the major versions of Collections and ORM, but I doubt that's really a bad thing (ORM 3.x only works with major X or higher, ORM 2.x only works with major Y or lower).

Alternatively I don't really see why we wouldn't support the enums in 2.x, it can't be that much work to support both...?

[derrabus]

Preferably the constants would remain gone in 3 as you're otherwise going to be stuck with them sort of forever.

I think, I can live with the maintenance burden of two unused constants, really.

I would just like a better migration path than we have now.

Then don't use the constants for the query builder or metadata mapping. Use strings, as it is officially documented. The constants are part of the Collections package, meant for use with the Criteria class of that package. They just conveniently happen to match the values that the query builder and the mapping attributes accept.

The "migration path" that you're discussing is actually a workaround for people that have used those constants in an undocumented way, outside of their scope.

By keeping the constants around, we would give people who want to remain on ORM 2 (for what reason ever) the possibility keep everything as it is. Why's that a problem?

[curry684]

The "migration path" that you're discussing is actually a workaround for people that have used those constants in an undocumented way, outside of their scope.

Fair enough, I forgot about that perspective and indeed this will likely not affect that many people 😄

[malarzm]

until we and probably the ODM folks (Cc @malarzm) figure out what changes to conduct in the ORM and the ODM.

@greg0ire sorry for taking long to respond, I was on vacations :) You can take ODM out of equation for two reasons:

1. ODM's query builder is accepting 1 and -1 as well as asc and desc for sorting. Ints are used by MongoDB and are first-class citizens for us
2. ODM never got Selectable and therefore Criteria implemented (PersistentCollection doesn't have the matching method mongodb-odm#929 to have linked topics) 🙈

So feel free to do whatever is best for ORM here :)

[bobvandevijver]

The "migration path" that you're discussing is actually a workaround for people that have used those constants in an undocumented way, outside of their scope.

To chime in here: while it might be outside their scope, it is not undocumented. See https://www.doctrine-project.org/projects/doctrine-orm/en/3.0/reference/working-with-associations.html#filtering-collections, where the constant is used in the official documentation.

edit: after submitting this, I see it is actually a Criteria method, and not an ORM method... It is confusing, caused by all those untyped strings 😄 I would love to see either the enum to be supported, or an additional enum in the ORM namespace itself.

[VincentLanglet]

It's a bit unfortunate that the ORM uses constants from a different package in their public API. For ORM 2.19, we could add replacement constants to the ORM codebase.

Just my two cent, but I agree with this point.
doctrine/collections is not, at first sight, a Db-related library so it feel weird to me that doctrine/orm use constants/enums from doctrine/collections fro Db-related directive like "OrderBy".

So far I solved the "deprecated" issue by using again the string value 'ASC' or 'DESC',
but I would have expected a constant directly from doctrine/orm like Expr\Join::WITH,

Doctrine\ORM\Query\Expr\OrderBy::ASC;
Doctrine\ORM\Query\Expr\OrderBy::DESC;

[MartijnBUZ]

Hello, not a contributor, just someone who uses the package here (combined with Symfony) :)

I've just manually changed 200+ files from Criteria:: to Order::, because Criteria:: told me to do that, only to end up getting the errors as described above. This makes me a bit annoyed. After reading the replies I understand what the issue is (keeping things seperated).

I'd like to know what the long term solution is?

1. Should I just revert all my work and keep the deprecated code?
2. Should I update everything to the ->value notation (honestly, not really gonna do that, its ugly)
3. Should I replace everything with 'ASC' and 'DESC'? That feels a bit amateur-y.

I like using the const, as less strings is more better IMO, but Im not gonna do this job twice. Has any decision been made to just update the AddOrderBy method? Its one line extra ($order = $order instanceof Order ? $order->value : $order).

Could someone please shed some light on this?

[derrabus]

1. Should I just revert all my work and keep the deprecated code?

You can do that.

2. Should I update everything to the ->value notation (honestly, not really gonna do that, its ugly)

I would advise against that.

3. Should I replace everything with 'ASC' and 'DESC'?

That would be the usage as we've always documented it: https://www.doctrine-project.org/projects/doctrine-orm/en/3.2/reference/query-builder.html#high-level-api-methods

As I've written earlier, you've used constants from the collections package outside of their scope.

That feels a bit amateur-y.

I don't share that sentiment, tbh. But if you prefer constants, you can of course just create your own constants within your codebase and simply use those.

Has any decision been made to just update the AddOrderBy method?

Not likely to happen.

Its one line extra

… and a breaking change for anyone who overrides the query builder.

[VincentLanglet]

Doctrine\ORM\Query\Expr\OrderBy::ASC;
Doctrine\ORM\Query\Expr\OrderBy::DESC;

WDYT about adding these constants @greg0ire @derrabus ?

[derrabus]

Yes, we can do that, if that helps. Would it make sense to duplicate them on Doctrine\ORM\Mapping\OrderBy as well?

[MartijnBUZ]

The things with consts is that it makes programming a bit more robust. You type Order:: in your editor and you get two options, no room for error. With string i might've typed 'ASCENDING' and now I have weird errors. Yes, I know that this specific example is a bit silly, but its the philosophy behind it. It allowes for unittests to go over the ENUM_NAME::cases() to test behaviours etc.

At the very least I agree with the other replies that the deprecation notice could use a clearification, avoiding people to make the same mistake :) But in my case, I'll update it all to the strings (or my own consts), thanks for the reply :)

[curry684]

A year later and I fell into this trap again (#11779). Let's look at finally fixing it once and for all because PHPStan and other tools are causing more issues as they gain popularity.

Doctrine\ORM\Query\Expr\OrderBy::ASC;
Doctrine\ORM\Query\Expr\OrderBy::DESC;

This is a bad idea, because you'd also need to add them to DBAL and ODM. Collections already has its own. You'd have 4 constants for the same thing, and you'd be using them intermittently as ORM works with Collections half the time. We should strive to reduce confusion, not add to it.

Instead I suggest we look at the fact that The Doctrine Project is the home to several PHP libraries primarily focused on database storage and object mapping.. Filtering and ordering data is a common thing amongst those focus points.

Shouldn't we just add such common things to doctrine/common then, and patch ORM, DBAL, ODM and Collections to abstract them to whatever they want internally? ODM can internally remap to -1 and 1 with a simple match.

(and yes I'm aware that DBAL nor ODM currently require doctrine/common)

[VincentLanglet]

Shouldn't we just add such common things to doctrine/common then, [...]
(and yes I'm aware that DBAL nor ODM currently require doctrine/common)

I saw lot of works to remove/deprecate as many as possible code from doctrine/common. I thought this lib was deprecated.

and patch ORM, DBAL, ODM

and doctrine/persistence I think, https://github.com/doctrine/persistence/blob/133bb2825572ee59e506c0bb7d6c9658506619a4/src/Persistence/ObjectRepository.php#L43

[greg0ire]

Indeed, I'm trying hard to retire doctrine/common, putting it here is a no-go.
@VincentLanglet nice catch with persistence. Since persistence has no dependency on collections, we cannot put a common enum in doctrine/collections. The opposite is true as well so… persistence should have its own enum?

So I would say the choice is between introducing a new package just for this, or duplicating enums on each package. I agree that would add more confusion.

This is not very satisfying :(

[curry684]

So I would say the choice is between introducing a new package just for this

What's wrong with putting it in common and still deprecating unneeded/legacy functionality from it? What is exactly the problem if, in the most extreme case, it ends up being a package with a single enum? I've seen smaller packages on NPM, and Composer honestly doesn't care - the average Symfony app contains 5 to 8 "contracts" packages containing a single interface.

I see it more as a starting point for standardizing more Doctrine common elements, like exceptions (ORM contains 31 exception classes, 10 of which in the Exception namespace, the rest all over the place, and many of them non-ORM-specific).

[VincentLanglet]

So I would say the choice is between introducing a new package just for this

What's wrong with putting it in common and still deprecating unneeded/legacy functionality from it?

You end up installing multiple class you don't need, just to use an enum.

What is exactly the problem if, in the most extreme case, it ends up being a package with a single enum? I've seen smaller packages on NPM, and Composer honestly doesn't care - the average Symfony app contains 5 to 8 "contracts" packages containing a single interface.

There is already a lot of doctrine package, and maintenance works, with a limited maintainers (and time).

I see it more as a starting point for standardizing more Doctrine common elements, like exceptions (ORM contains 31 exception classes, 10 of which in the Exception namespace, the rest all over the place, and many of them non-ORM-specific).

DbalException are meant to be in the doctrine/dbal,
OrmException are meant to be in the doctrine/orm, and so on.

I don't expect a package with a lot of exception/context I don't use.

So I would say the choice is between introducing a new package just for this, or duplicating enums on each package.

Or just keep working with 'ASC' and 'DESC' string.
And if we really want a way to autocomplete those name and don't want to "duplicate enum":

• we can still expose those string as constant in each package. I see less issue having duplicated string.
• or we can just typehint 'ASC'|'DESC' in the phpdoc and let the static analysis do the job.

[greg0ire]

What's wrong with putting it in common and still deprecating unneeded/legacy functionality from it?

Not much. It's only "wrong" from a naming perspective. Just like when you have a Common , Tools or Utils directory in a project, it inevitably ends up being a place where to put unrelated pieces of code by default. In the past, this package hold the annotations feature, for instance, that's why you still have "common" in the namespace of annotations. Once something ends up there, it can be notoriously hard to move it, especially if you want a new namespace.
See https://alcaeus.medium.com/how-to-break-an-entire-ecosystem-by-publishing-a-release-b6aaab2b8aaa for a concrete example.

@VincentLanglet yeah, maybe we should do a full 180 on this, right now it seems like a dead-end. Let us see what other maintainers think.

[greg0ire]

Another solution could be to have a dependency from persistence to collections. Given the install statistics, I'd say it's quite probable that people installing persistence already install collections as well. ObjectRepository::find() returns an array, which is arguably some kind of collection, so it might make sense.

[ThomasLandauer]

Reverting this and going back to strings feels like a step backward to me.
If there's no place where it fits nicely, then I'd suggest to create a dedicated package for it: doctrine/order. Sure, having a package for 2 strings is kinda ridiculous, but I agree with @curry684 above: Who cares?

[curry684]

You end up installing multiple class you don't need, just to use an enum.

I would honestly be surprised if the vendor folder in any of my projects has less than 75% unused files. It's kinda the point of libraries that they install files to cover all bases, including the ones your specific project doesn't.

There is already a lot of doctrine package, and maintenance works, with a limited maintainers (and time).

I think the maintenance burden of a single static 2-value enum is overseeable if it fixes recurring discussions like the one we're having right here, and not having to maintain them in 4 different projects anymore.

I don't expect a package with a lot of exception/context I don't use.

Yet you probably install them all the time. PSR Container. PSR Events. PSR Cache. and so on. Symfony HTTP Client Contracts defines 8 exceptions so implementations don't have to.

All these packages contain just a few common interfaces, types and/or exceptions. Because it Makes Sense to abstract and reuse.

OrmException are meant to be in the doctrine/orm, and so on.

But is NonUniqueResultException really an ORM exception, or a "database and persistence" related exception?

Not much. It's only "wrong" from a naming perspective. Just like when you have a Common , Tools or Utils directory in a project, it inevitably ends up being a place where to put unrelated pieces of code by default.

Fair enough, but the fact that such namespaces/libraries can end up becoming the garbage can doesn't mean it's a bad idea to have them. Just that it's a good idea to maintain them properly. And if 4 libraries share common elements then it makes sense to bring these common elements up to a library for which common is a logical name.

[greg0ire]

And if 4 libraries share common elements then it makes sense to bring these common elements up to a library for which common is a logical name.

It might be logical, but it's a bit inconsistent, isn't it? On the one hand, we have small libraries, with a hopefully clear focus, and then, there would be "bag-of-stuff-that-we-could-not-be-bothered-to-properly-store". Plus, some things would be common to some libraries, but some other things could be common to other libraries. inflector will never need the Order enum, for instance.

Another solution could be to have a dependency from persistence to collections. Given the install statistics, I'd say it's quite probable that people installing persistence already install collections as well. ObjectRepository::find() returns an array, which is arguably some kind of collection, so it might make sense.

Any comment on this solution?

[curry684]

Any comment on this solution?

From an architectural perspective it sounds to me like "So we have this code here that we should and want to re-use in 5 packages, but we don't want to create a real dependency, so we're going to introduce a false dependency"?

I'm not really seeing the problem here, the only argument against given has been "we're trying to get rid of common". Yet Symfony introduced tons of contracts packages over the past years to standardize interfaces, values and exceptions. If we really don't want to call it common, shouldn't we call it doctrine/contracts and treat it like that? So no real classes, but stuff like the Order enum, an OrderableQueryBuilderInterface standardizing things like addOrderBy with correct typing, a base PersistenceException, NoResultException, CacheException and such. It could even include traits and abstract base classes similar to psr/log.

I've presented my case, I think at this point it's more up to the maintainer's committee.

[curry684]

Addendum about why I keep harking about exceptions, in a project just using ORM:

/vendor/doctrine$ find -name "*Exception.php"
./dbal/src/Schema/SchemaException.php
./dbal/src/ArrayParameters/Exception.php
./dbal/src/Platforms/Exception/PlatformException.php
./dbal/src/Driver/PDO/Exception.php
./dbal/src/Driver/SQLite3/Exception.php
./dbal/src/Driver/PgSQL/Exception.php
./dbal/src/Driver/AbstractException.php
./dbal/src/Driver/Exception.php
./dbal/src/ConnectionException.php
./dbal/src/Exception/LockWaitTimeoutException.php
./dbal/src/Exception/NonUniqueFieldNameException.php
./dbal/src/Exception/InvalidArgumentException.php
./dbal/src/Exception/ReadOnlyException.php
./dbal/src/Exception/ConstraintViolationException.php
./dbal/src/Exception/ForeignKeyConstraintViolationException.php
./dbal/src/Exception/ConnectionException.php
./dbal/src/Exception/TableNotFoundException.php
./dbal/src/Exception/DatabaseObjectNotFoundException.php
./dbal/src/Exception/TableExistsException.php
./dbal/src/Exception/DeadlockException.php
./dbal/src/Exception/DriverException.php
./dbal/src/Exception/MalformedDsnException.php
./dbal/src/Exception/SyntaxErrorException.php
./dbal/src/Exception/UniqueConstraintViolationException.php
./dbal/src/Exception/DatabaseObjectExistsException.php
./dbal/src/Exception/InvalidFieldNameException.php
./dbal/src/Exception/NotNullConstraintViolationException.php
./dbal/src/Exception/ServerException.php
./dbal/src/Exception/RetryableException.php
./dbal/src/Types/Exception/TypesException.php
./dbal/src/Types/ConversionException.php
./dbal/src/Cache/CacheException.php
./dbal/src/Exception.php
./dbal/src/SQL/Parser/Exception.php
./dbal/src/Query/QueryException.php
./instantiator/src/Doctrine/Instantiator/Exception/InvalidArgumentException.php
./instantiator/src/Doctrine/Instantiator/Exception/UnexpectedValueException.php
./orm/src/NoResultException.php
./orm/src/Tools/Exception/MissingColumnException.php
./orm/src/Tools/ToolsException.php
./orm/src/Tools/Console/EntityManagerProvider/UnknownManagerException.php
./orm/src/Exception/PersisterException.php
./orm/src/Exception/EntityIdentityCollisionException.php
./orm/src/Exception/RepositoryException.php
./orm/src/Exception/DuplicateFieldException.php
./orm/src/Exception/ConfigurationException.php
./orm/src/Exception/MultipleSelectorsFoundException.php
./orm/src/Exception/ORMException.php
./orm/src/Exception/SchemaToolException.php
./orm/src/Exception/ManagerException.php
./orm/src/Exception/NoMatchingPropertyException.php
./orm/src/NonUniqueResultException.php
./orm/src/Internal/Hydration/HydrationException.php
./orm/src/Internal/TopologicalSort/CycleDetectedException.php
./orm/src/EntityNotFoundException.php
./orm/src/PessimisticLockException.php
./orm/src/Mapping/MappingException.php
./orm/src/ORMInvalidArgumentException.php
./orm/src/Persisters/PersisterException.php
./orm/src/Cache/LockException.php
./orm/src/Cache/Exception/CacheException.php
./orm/src/Cache/CacheException.php
./orm/src/OptimisticLockException.php
./orm/src/TransactionRequiredException.php
./orm/src/UnexpectedResultException.php
./orm/src/Query/QueryException.php
./orm/src/Query/Filter/FilterException.php
./orm/src/Query/AST/ASTException.php
./data-fixtures/src/Exception/CircularReferenceException.php
./persistence/src/Persistence/Mapping/MappingException.php

I've always taught people that an Exception class only makes sense if anyone's ever going to want to catch it, and details should go in the error message. By that perspective many of the classes above can be eliminated. Did you know PessimisticLockException and OptimisticLockException share no other base class than \Exception, and only one of them correctly eventually derives from \RuntimeException? I just discovered, because stuff like this has just become Too Big over time. That's also what contracts do in software architecture - introduce clarity, rules and oversight.

[greg0ire]

I've always taught people that an Exception class only makes sense if anyone's ever going to want to catch it

One could argue that you can move the complexity of building a meaningful message in such a class. Sometimes, a single exception can have several constructors. So while it's indeed not necessary at all, it can make sense IMO.

I did make a lot of changes to exceptions in the past, but I don't claim I did a perfect job, so if you feel more changes are needed, you are welcome to send a separate RFC, or a PR.

[VincentLanglet]

Another solution could be to have a dependency from persistence to collections. Given the install statistics, I'd say it's quite probable that people installing persistence already install collections as well.

Any comment on this solution?

I don't like this solution, but for the same reason I already don't like the fact of using Doctrine/Collections/Order enum in other doctrine package. I don't see why the domain "Collection" would appear here.
I thought Collection/Criteria are meant for in-memory filtering/ordering while queryBuilder is constructing SQL queries.

If PHP had a native method signature sort($array, 'ASC'|'DESC')we wouldn't try to useCollection/Criteria/Order::Ascending` "just because it's the same constant.

ObjectRepository::find() returns an array, which is arguably some kind of collection, so it might make sense.

IMHO it would have more sens if the method returned a real Collection.
But still, method like findOne would have return single element.

In doctrine/dbal, I dunno if ASC and DESC are used but if so these are more Query/DB-related keyword and doctrine/collections is never used. Same idea for doctrine/persistence.

In doctrine/orm, I see usage in

• Doctrine\ORM\Mapping\OrderBy (and ToManyAssociationMapping,
• Doctrine\ORM\Query\SqlWalker\OrderByItem
• Doctrine\ORM\Query\Expr\OrderBy

Non of this code

• Works with collections or criteria
• Returns collections

So I feel like we try to fit square in circle...
A Doctrine/Common/Criteria would have been better than Doctrine/Collections/Criteria.
or just Doctrine/Criteria

[curry684]

or just Doctrine/Criteria

Doctrine\Contracts\Types\Order

😉

[greg0ire]

If you suggest a doctrine/contracts package, I think that would only be slightly better than doctrine/common, because we'd still be grouping possibly unrelated stuff together, it's just that we'd have the extra constraint of having only interfaces and enums in there.