Introduce Order enum #389

greg0ire · 2024-02-17T11:22:03Z

The plan is to ultimately use when dealing with Criteria::$orderings

ThomasLandauer · 2024-02-17T11:45:40Z

Yeah, looks good to me :-)
If you merge it, I will take care of the rest of the docs, as promised in doctrine/orm#11263 (comment)

src/Criteria.php

greg0ire · 2024-02-18T11:34:31Z

src/Criteria.php

@@ -155,16 +155,19 @@ public function getWhereExpression()
     */


In the future, we would have @return array<string, Order> here

Maybe I should use array<string, string|Order> right now so that people start adapting their code, even if in practice we don't break BC and stay with array<string, string> ? 🤔 either that or introducing a new method.

We should provide a way to opt into the future behavior of this method. We could do so by adding a boolean flag (meh) or by adding a new method and deprecating this one.

Ok, I'll try to find a new name.

Since there is no setter, I went with just orderings()

derrabus · 2024-02-18T15:54:37Z

src/Order.php

+    case ASC  = 'ASC';
+    case DESC = 'DESC';


Suggested change

case ASC = 'ASC';

case DESC = 'DESC';

case Ascending = 'ASC';

case Descending = 'DESC';

WDYT?

derrabus · 2024-02-18T15:59:39Z

src/Criteria.php

     *
-     * @param array<string, string> $orderings
+     * @param array<string, string|Order> $orderings


The types of the constructor signature have to be adjusted as well.

derrabus · 2024-02-18T16:00:07Z

src/Criteria.php

+                    return $ordering;
+                }
+
+                return strtoupper($ordering) === Order::ASC->value ? Order::ASC : Order::DESC;


Trigger a deprecation?

Should we give the ORM a chance to upgrade first?

I don't think we need to wait for that.

Ok, let me add that deprecation layer then.

derrabus · 2024-02-18T16:01:42Z

src/Criteria.php

-     * @see Criteria::ASC
-     * @see Criteria::DESC


Deprecate the constants?

derrabus · 2024-02-18T16:03:38Z

tests/BaseArrayCollectionTest.php

@@ -331,7 +332,7 @@ public function testMatchingWithSortingPreserveKeys(): void
                'object1' => $object1,
            ],
            $collection
-                ->matching(new Criteria(null, ['sortField' => Criteria::ASC]))
+                ->matching(new Criteria(null, ['sortField' => Order::ASC]))


By simply changing the tests, you lose coverage for the old way of using this API. Codecov rightfully complains about that.

ThomasLandauer · 2024-02-18T16:35:10Z

I guess this will then also be the way to order collections?:

#[ORM\OneToMany(...)
#[ORM\OrderBy(value: ['foo' => Order::Ascending])
private Collection $bar;

greg0ire · 2024-02-18T16:44:54Z

Yes 👍

The plan is to ultimately use it and only it dealing with Criteria::$orderings.

greg0ire · 2024-02-24T21:14:54Z

Ping @derrabus 🙏

ThomasLandauer · 2024-02-25T11:47:04Z

Which branch/version of the docs should I edit?

greg0ire · 2024-02-25T12:07:26Z

I think it depends, we have to look at the code, and see if the new names are passed as is or if some kind of translation/method signature changes are required.

BTW, why are you mentioning DBAL? It does not depend on collections, does it?

ThomasLandauer · 2024-02-25T12:33:02Z

I'm talking about the docs, not the code!
DBAL docs have some occurrences, e.g. https://www.doctrine-project.org/projects/doctrine-dbal/en/4.0/reference/query-builder.html#order-by-clause

So should I replace all 'ASC' with the new Order::Ascending everywhere in ORM 3.1 (assuming that by that time everybody will have upgraded their collections)? Or add a note "New in version xy" like at https://www.doctrine-project.org/projects/doctrine-orm/en/2.11/tutorials/extra-lazy-associations.html#extra-lazy-associations ? - This could become a mess, if there are many occurrences (which I guess there are...)

greg0ire · 2024-02-25T12:45:11Z

I'm talking about the docs, not the code!

I know, and please don't shout!

DBAL docs have some occurrences, e.g. https://www.doctrine-project.org/projects/doctrine-dbal/en/4.0/reference/query-builder.html#order-by-clause

That's an SQL query builder. This has nothing to do with collections whatsoever. doctrine/dbal should work without doctrine/collections installed. If you modify those docs to refer to Order, you will generate support tickets.

So should I replace all 'ASC' with the new Order::Ascending everywhere in ORM 3.1 (assuming that by that time everybody will have upgraded their collections)? Or add a note "New in version xy" like at https://www.doctrine-project.org/projects/doctrine-orm/en/2.11/tutorials/extra-lazy-associations.html#extra-lazy-associations ? - This could become a mess, if there are many occurrences (which I guess there are...)

Maybe, wait a bit, we have to evaluate which ones are going to be OK, and which ones require a breaking change. For instance consider this:

class ClassFromTheORM
{
    public function methodFromTheOrm(string $ordering)
    {
        Criteria::orderBy(['field' => $ordering);
    }
}

Can't recommend Order::Ascending for that one.

ThomasLandauer · 2024-02-25T13:02:05Z

and please don't shout!

Sorry :-)

That's an SQL query builder. This has nothing to do with collections whatsoever.

I thought this is what this PR addresses?! - Moving from ->orderBy('username', 'ASC') to ->orderBy('username', Order::Ascending)?
So what is deprecated then?

greg0ire · 2024-02-25T13:15:49Z

So what is deprecated then?

What you just said + referring to Criteria::ASC. We can see that it happens in the ORM, here for instance: https://github.com/doctrine/orm/blob/2a8802af122921ad9c40a266a74ce8c210c18e44/src/PersistentCollection.php#L588 or here: https://github.com/doctrine/orm/blob/2a8802af122921ad9c40a266a74ce8c210c18e44/src/Mapping/Driver/XmlDriver.php#L408

I don't think you will be able to find similar stuff in the DBAL.

ThomasLandauer · 2024-02-25T13:41:34Z

OK, gotcha.

https://www.doctrine-project.org/projects/doctrine-dbal/en/4.0/reference/query-builder.html#order-by-clause is about the SQL Query Builder (not DQL Query Builder as I thought)
Indeed there are no other occurrences of ASC or DESC in the DBAL docs.
In the ORM docs there are fewer occurrences than I thought, most of them are on https://www.doctrine-project.org/projects/doctrine-orm/en/3.0/reference/query-builder.html So should I start with that page now or wait?

greg0ire · 2024-02-25T14:34:37Z

You should IMO wait until we at least:

release doctrine/collections 2.2.0
contribute something that allows using Order::Ascending (right now, QueryBuilder::orderBy() accepts a string, and Order::Ascending is no string, Order::Ascending->value is).

jifer · 2024-03-13T17:11:44Z

We just upgraded doctrine packages. Changing Criteria constants to enum was straightforward. We do love enums.

Problem is that we used this everywhere where QueryBuilder is involved (a lot), and it was quite disappointing seeing that this is not going to work.

We go from this:

$this->getEntityManager()
            ->getRepository(SomeEntity::class)
            ->createQueryBuilder('h')
            ->orderBy('h.id', Criteria::DESC)
            ->getQuery()
            ->getResult();

to this:

$this->getEntityManager()
            ->getRepository(SomeEntity::class)
            ->createQueryBuilder('h')
            ->orderBy('h.id', Order::Descending->value)
            ->getQuery()
            ->getResult();

Yes, I do understand that Collections & ORM are separate, but ORM relays on Collections as dependency.
So can we count on:
a) Allowing Order as orderBy second param
b) or dedicated enum for QB that will replace string.

derrabus · 2024-03-13T17:26:02Z

Neither a) nor b) is planned at the moment. That being said, this is the wrong repository for this discussion.

greg0ire mentioned this pull request Feb 17, 2024

Deprecate not passing Criteria constants to orderBy() #386

Closed

greg0ire force-pushed the order-enum branch from 456b4ce to 1de0d8f Compare February 18, 2024 11:33

greg0ire commented Feb 18, 2024

View reviewed changes

src/Criteria.php Outdated Show resolved Hide resolved

greg0ire commented Feb 18, 2024

View reviewed changes

greg0ire force-pushed the order-enum branch from 1de0d8f to 563c41e Compare February 18, 2024 11:35

greg0ire marked this pull request as ready for review February 18, 2024 15:10

greg0ire requested review from derrabus, SenseException and malarzm February 18, 2024 15:10

derrabus reviewed Feb 18, 2024

View reviewed changes

derrabus requested changes Feb 18, 2024

View reviewed changes

greg0ire force-pushed the order-enum branch 7 times, most recently from 2c70d32 to 6b56e71 Compare February 18, 2024 16:12

greg0ire requested a review from derrabus February 18, 2024 16:22

ThomasLandauer mentioned this pull request Feb 18, 2024

[make:entity] Use const for ASC in ORDER BY symfony/maker-bundle#1451

Closed

Introduce Order enum

a288ea7

The plan is to ultimately use it and only it dealing with Criteria::$orderings.

greg0ire force-pushed the order-enum branch from 6b56e71 to a288ea7 Compare February 18, 2024 22:14

ThomasLandauer mentioned this pull request Feb 20, 2024

[Documentation] Using const for ASC doctrine/orm#11263

Closed

greg0ire added the Deprecation label Feb 20, 2024

derrabus approved these changes Feb 24, 2024

View reviewed changes

greg0ire added this to the 2.2.0 milestone Feb 24, 2024

greg0ire merged commit 957e63b into doctrine:2.2.x Feb 24, 2024
9 checks passed

greg0ire deleted the order-enum branch February 24, 2024 23:08

greg0ire mentioned this pull request Feb 26, 2024

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

Open

doctrine locked and limited conversation to collaborators Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Order enum #389

Introduce Order enum #389

greg0ire commented Feb 17, 2024 •

edited

Loading

ThomasLandauer commented Feb 17, 2024

greg0ire Feb 18, 2024

greg0ire Feb 18, 2024 •

edited

Loading

derrabus Feb 18, 2024

greg0ire Feb 18, 2024

greg0ire Feb 18, 2024

derrabus Feb 18, 2024 •

edited

Loading

derrabus Feb 18, 2024

greg0ire Feb 20, 2024

derrabus Feb 18, 2024

greg0ire Feb 18, 2024

derrabus Feb 18, 2024

greg0ire Feb 18, 2024

derrabus Feb 18, 2024

greg0ire Feb 20, 2024

derrabus Feb 18, 2024

ThomasLandauer commented Feb 18, 2024

greg0ire commented Feb 18, 2024

greg0ire commented Feb 24, 2024

ThomasLandauer commented Feb 25, 2024

greg0ire commented Feb 25, 2024 •

edited

Loading

ThomasLandauer commented Feb 25, 2024

greg0ire commented Feb 25, 2024 •

edited

Loading

ThomasLandauer commented Feb 25, 2024

greg0ire commented Feb 25, 2024

ThomasLandauer commented Feb 25, 2024 •

edited

Loading

greg0ire commented Feb 25, 2024

jifer commented Mar 13, 2024

derrabus commented Mar 13, 2024

		@@ -155,16 +155,19 @@ public function getWhereExpression()
		*/

Introduce Order enum #389

Introduce Order enum #389

Conversation

greg0ire commented Feb 17, 2024 • edited Loading

ThomasLandauer commented Feb 17, 2024

Choose a reason for hiding this comment

greg0ire Feb 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derrabus Feb 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomasLandauer commented Feb 18, 2024

greg0ire commented Feb 18, 2024

greg0ire commented Feb 24, 2024

ThomasLandauer commented Feb 25, 2024

greg0ire commented Feb 25, 2024 • edited Loading

ThomasLandauer commented Feb 25, 2024

greg0ire commented Feb 25, 2024 • edited Loading

ThomasLandauer commented Feb 25, 2024

greg0ire commented Feb 25, 2024

ThomasLandauer commented Feb 25, 2024 • edited Loading

greg0ire commented Feb 25, 2024

jifer commented Mar 13, 2024

derrabus commented Mar 13, 2024

greg0ire commented Feb 17, 2024 •

edited

Loading

greg0ire Feb 18, 2024 •

edited

Loading

derrabus Feb 18, 2024 •

edited

Loading

greg0ire commented Feb 25, 2024 •

edited

Loading

greg0ire commented Feb 25, 2024 •

edited

Loading

ThomasLandauer commented Feb 25, 2024 •

edited

Loading