Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor the OrderByKeyToClassConstRector to use the new enum only in Criteria::orderBy method calls #336

Merged
merged 4 commits into from
Aug 26, 2024

Conversation

julienfastre
Copy link
Contributor

@julienfastre julienfastre commented Jul 31, 2024

Since version 2.2.0 of doctrine/collection, the constant Doctrine\Common\Collections\Criteria is deprecated in favor of new enum \Doctrine\Common\Collections\Order.

But this enum should only be used in invocations of Doctrine\Common\Collection\Criteria::orderBy method call. Other calls (in attributes, or for the same method name on object instantiating another class) should not be changed: the string 'ASC' and 'DESC' should be kept elsewhere.

This PR refactor the rule which, previously, replaced all the 'ASC' and 'DESC' by the Criteria::ASC|DESC.

After this MR:

  • all invocation of class constant Criteria::ASC and Criteria::DESC are replaced by, respectively, the string "ASC" ans "DESC";
  • in the method invocation Criteria::orderBy, the usage of the "ASC" (lowercase or uppercase), "DESC" (lowercase or uppercase), Criteria::ASC, Criteria::DESC is replaced by the new enum Order::Ascending, Order::Descending;
  • the rule is added to the new ruleset for Collection 2.2;

The previous rule, which changed the usage of the string "ASC" (lowercase or uppercase) or "DESC" (lowercase or uppercase) by the usage of Criteria::ASC and Criteria::DESC is, obviously, removed.

Comments and documentation supporting this change

Closes #325

EDIT list the changes in a more understandable (?) way.

@julienfastre
Copy link
Contributor Author

julienfastre commented Jul 31, 2024

Still some TODO in this PR:

  • this rule should not apply on PHP versions earlier that 8.1.0 and doctrine collection greater that 2.2.0
  • find a way to ensure that the 'orderBy' method does apply on a \Doctrine\Common\Collections\Criteria instance
  • find a way to identify the full FQDN for Criteria

@TomasVotruba TomasVotruba marked this pull request as ready for review August 9, 2024 21:46
@julienfastre julienfastre force-pushed the 325-use-ordering-enum branch 2 times, most recently from 24ef0ce to b41f96c Compare August 25, 2024 16:20
… Criteria::orderBy method call, and remove usage of Criteria::ASC and Criteria::DESC where not recommended

Relates to:

- doctrine/collections#389;
- doctrine/orm#11313 (comment)
* @return ArrayItem
*/
private function buildArrayItem(string $direction, Node\Expr|null $key): Node\Expr\ArrayItem
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$direction need to be transformed to strtoupper first to ensure it always uppercase to compare

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this is done on lines 169 and 181.

But this is maybe not the most readable way for doing this...

Simplify the type-checking condition by directly verifying if the criteria object type is a super type and ensuring it returns a positive result.
Copy link
Member

@samsonasik samsonasik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's give it a try 👍

@samsonasik
Copy link
Member

Thank you @julienfastre

@samsonasik samsonasik merged commit a302734 into rectorphp:main Aug 26, 2024
7 checks passed
[
new CodeSample(
<<<'CODE_SAMPLE'
<?php
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

php open tag seems cause use statement prefixed in code example, and become:


namespace RectorPrefix202408;
use RectorPrefix202408\Doctrine\Common\Collections\Criteria;

ref rectorphp/rector@b8c4f15#diff-c260cb91d355f253688b74213c5bb7c75414832d6c18ba756b6ba3accf08964f

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TomasVotruba
Copy link
Member

Thank you @julienfastre 👍

Comment on lines -35 to 33
OrderByKeyToClassConstRector::class,
]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder, why not add the rule here too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This specifically for doctrine 2.2+

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DOCTRINE_COLLECTION_22] constant to enum error for querybuilder
3 participants