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

e7cf418caf/src/Persistence/ObjectRepository.php (L42-L43)

@derrabus commented on GitHub (Feb 27, 2024):

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:

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

@derrabus commented on GitHub (Feb 27, 2024): 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.

admin commented

@NMe84 commented on GitHub (Feb 28, 2024):

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.

@NMe84 commented on GitHub (Feb 28, 2024): > 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.

admin commented

@curry684 commented on GitHub (Feb 28, 2024):

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?

@curry684 commented on GitHub (Feb 28, 2024): > 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?

admin commented

@derrabus commented on GitHub (Feb 28, 2024):

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.

@derrabus commented on GitHub (Feb 28, 2024): 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.

admin commented

2026-01-22 15:50:08 +01:00

@curry684 commented on GitHub (Feb 28, 2024):

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...?

@curry684 commented on GitHub (Feb 28, 2024): 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...?

admin commented

@derrabus commented on GitHub (Feb 28, 2024):

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?

@derrabus commented on GitHub (Feb 28, 2024): > 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?

admin commented

2026-01-22 15:50:08 +01:00

@curry684 commented on GitHub (Feb 28, 2024):

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 😄

@curry684 commented on GitHub (Feb 28, 2024): > 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 😄

admin commented

2026-01-22 15:50:08 +01:00

@malarzm commented on GitHub (Mar 3, 2024):

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:

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
ODM never got Selectable and therefore Criteria implemented (https://github.com/doctrine/mongodb-odm/issues/929 to have linked topics) 🙈

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

@malarzm commented on GitHub (Mar 3, 2024): > 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 (https://github.com/doctrine/mongodb-odm/issues/929 to have linked topics) 🙈 So feel free to do whatever is best for ORM here :)

admin commented

@bobvandevijver commented on GitHub (Mar 4, 2024):

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.

@bobvandevijver commented on GitHub (Mar 4, 2024): > 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.

admin commented

@VincentLanglet commented on GitHub (Apr 3, 2024):

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;

@VincentLanglet commented on GitHub (Apr 3, 2024): > 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; ```

admin commented

@ghost commented on GitHub (Jul 8, 2024):

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?

Should I just revert all my work and keep the deprecated code?
Should I update everything to the ->value notation (honestly, not really gonna do that, its ugly)
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?

@ghost commented on GitHub (Jul 8, 2024): 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?

admin commented

@derrabus commented on GitHub (Jul 8, 2024):

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

You can do that.

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

I would advise against that.

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.

@derrabus commented on GitHub (Jul 8, 2024): > 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.

admin commented

@VincentLanglet commented on GitHub (Jul 8, 2024):

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

WDYT about adding these constants @greg0ire @derrabus ?

@VincentLanglet commented on GitHub (Jul 8, 2024): > ``` > Doctrine\ORM\Query\Expr\OrderBy::ASC; > Doctrine\ORM\Query\Expr\OrderBy::DESC; > ``` WDYT about adding these constants @greg0ire @derrabus ?

admin commented

2026-01-22 15:50:10 +01:00

@derrabus commented on GitHub (Jul 8, 2024):

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

@derrabus commented on GitHub (Jul 8, 2024): Yes, we can do that, if that helps. Would it make sense to duplicate them on `Doctrine\ORM\Mapping\OrderBy` as well?

admin commented

@ghost commented on GitHub (Jul 9, 2024):

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 :)

@ghost commented on GitHub (Jul 9, 2024): 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 :)

admin commented

@curry684 commented on GitHub (Jan 6, 2025):

A year later and I fell into this trap again (https://github.com/doctrine/orm/pull/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)

@curry684 commented on GitHub (Jan 6, 2025): A year later and I fell into this trap again (https://github.com/doctrine/orm/pull/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.](https://github.com/doctrine). 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`.](https://github.com/doctrine/orm/issues/11313#issuecomment-1975353641) (and yes I'm aware that DBAL nor ODM currently require `doctrine/common`)

admin commented

@VincentLanglet commented on GitHub (Jan 6, 2025):

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, 133bb28255/src/Persistence/ObjectRepository.php (L43)

@VincentLanglet commented on GitHub (Jan 6, 2025): > 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

admin commented

@greg0ire commented on GitHub (Jan 7, 2025):

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 :(

@greg0ire commented on GitHub (Jan 7, 2025): 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 :(

admin commented

@curry684 commented on GitHub (Jan 7, 2025):

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).

@curry684 commented on GitHub (Jan 7, 2025): > 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).

admin commented

@VincentLanglet commented on GitHub (Jan 7, 2025):

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.

@VincentLanglet commented on GitHub (Jan 7, 2025): > > 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.

admin commented

@greg0ire commented on GitHub (Jan 7, 2025):

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 commented on GitHub (Jan 7, 2025): > 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.

admin commented

@greg0ire commented on GitHub (Jan 7, 2025):

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.

@greg0ire commented on GitHub (Jan 7, 2025): 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.

admin commented

@ThomasLandauer commented on GitHub (Jan 7, 2025):

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?

@ThomasLandauer commented on GitHub (Jan 7, 2025): 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](https://github.com/doctrine/orm/issues/11313#issuecomment-2576155999): Who cares?

admin commented

@curry684 commented on GitHub (Jan 7, 2025):

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.

@curry684 commented on GitHub (Jan 7, 2025): > 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.](https://github.com/php-fig/container/tree/master/src) [PSR Events](https://github.com/php-fig/event-dispatcher/tree/master/src). [PSR Cache.](https://github.com/php-fig/cache/tree/master/src) and so on. [Symfony HTTP Client Contracts defines 8 exceptions](https://github.com/symfony/http-client-contracts/tree/main/Exception) 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.

admin commented

@greg0ire commented on GitHub (Jan 8, 2025):

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?

@greg0ire commented on GitHub (Jan 8, 2025): > 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?

admin commented

@curry684 commented on GitHub (Jan 8, 2025):

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 commented on GitHub (Jan 8, 2025): > 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.

admin commented

@curry684 commented on GitHub (Jan 8, 2025):

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.

@curry684 commented on GitHub (Jan 8, 2025): Addendum about why I keep harking about exceptions, in a project just using ORM: ```sh /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.

admin commented

@greg0ire commented on GitHub (Jan 8, 2025):

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.

@greg0ire commented on GitHub (Jan 8, 2025): > 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](https://github.com/doctrine/orm/pull/6743), 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.

admin commented

@VincentLanglet commented on GitHub (Jan 8, 2025):

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

@VincentLanglet commented on GitHub (Jan 8, 2025): > > 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 use `Collection/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`

admin commented

@curry684 commented on GitHub (Jan 8, 2025):

or just Doctrine/Criteria

Doctrine\Contracts\Types\Order

😉

@curry684 commented on GitHub (Jan 8, 2025): > or just Doctrine/Criteria ``` Doctrine\Contracts\Types\Order ``` 😉

admin commented