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

Deprecate not passing Criteria constants to orderBy() #386

Closed
wants to merge 2 commits into from

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented Feb 16, 2024

I had the idea of doing this while trying to review doctrine/orm#11263

The signature of orderBy() is needlessly hard to understand for static
analysis. Also, if somebody makes a typo and writes "dsc", they will end
up with Criteria::ASC.

Let's also take this as an occasion to improve the documentation of Criteria::getOrderings()

The signature of orderBy() is needlessly hard to understand for static
analysis. Also, if somebody makes a typo and writes "dsc", they will end
up with Criteria::ASC.
@ThomasLandauer
Copy link
Contributor

Just one quick question ("now or never"): Is Doctrine\Common\Collections\Criteria really the way to go or would it be better to create a dedicated class for those consts, higher in the namespace hierarchy, e.g. Doctrine\Order?

@greg0ire
Copy link
Member Author

greg0ire commented Feb 17, 2024

I think it might be better to have a class an enum indeed, but it should IMO stay in the collections namespace, I don't think we should create a dedicated namespace for that.

@ThomasLandauer
Copy link
Contributor

So the future will be that strings will be forbidden in ->orderBy()? Then I'd suggest to go for a pure enum, and call it Order.

@greg0ire
Copy link
Member Author

greg0ire commented Feb 17, 2024

I think the future should be a string-backed enum. That way, we can create it in 2.2.x accept it in orderBy() and use it internally, deprecate not using it in 2.3.x and in 3.0.x use it in getOrderings as well.

<?php

enum Order: string
{
    case ASC = "ASC";
    case DESC = "DESC";
}

@greg0ire greg0ire changed the base branch from 2.1.x to 2.2.x February 17, 2024 11:11
@greg0ire
Copy link
Member Author

@ThomasLandauer here is how it could look like: #389

@greg0ire greg0ire closed this Feb 25, 2024
@greg0ire greg0ire deleted the sa-order branch February 25, 2024 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants