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

Postpone deprecation of string representation of order #403

Closed
wants to merge 1 commit into from

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented Feb 27, 2024

In downstream packages that representation is recommended in places where type hints do not allow using an enum yet, which means we would have to modify the public API of downstream packages to allow it.

Let us postpone the deprecation until we figure what exactly needs to be done.

More info on this at doctrine/orm#11313

Comment on lines 137 to 161
public function testPassingNonOrderEnumToOrderByIsDeprecated(): void
{
$this->expectDeprecationWithIdentifier('https://github.com/doctrine/collections/pull/389');
$criteria = Criteria::create()->orderBy(['foo' => 'ASC']);
}

public function testConstructingCriteriaWithNonOrderEnumIsDeprecated(): void
{
$this->expectDeprecationWithIdentifier('https://github.com/doctrine/collections/pull/389');
$criteria = new Criteria(null, ['foo' => 'ASC']);
}

public function testUsingOrderEnumIsTheRightWay(): void
{
$this->expectNoDeprecationWithIdentifier('https://github.com/doctrine/collections/pull/389');
Criteria::create()->orderBy(['foo' => Order::Ascending]);
new Criteria(null, ['foo' => Order::Ascending]);
}

public function testCallingGetOrderingsIsDeprecated(): void
{
$criteria = Criteria::create()->orderBy(['foo' => Order::Ascending]);
$this->expectDeprecationWithIdentifier('https://github.com/doctrine/collections/pull/389');
$criteria->getOrderings();
}
Copy link
Member

Choose a reason for hiding this comment

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

Even if you take back the deprecation, those execution paths still have to be tested.

@derrabus
Copy link
Member

I not convinced we need to take back the deprecation. Why can't it be fixed downstream?

@greg0ire
Copy link
Member Author

greg0ire commented Feb 27, 2024

After reading doctrine/orm#11313 (comment), I believe it would imply changing, among other things, the signature of QueryBuilder::orderBy() as follows:

-    public function addOrderBy(string|Expr\OrderBy $sort, string|null $order = null): static
+    public function addOrderBy(string|Expr\OrderBy $sort, Order|string|null $order = null): static

That's a breaking change for extending classes.

I tried addressing the deprecations yesterday evening, couldn't finish, and I think I wouldn't expect such a change in a minor release.

@derrabus
Copy link
Member

Then we don't change this API yet. 🙂

@greg0ire
Copy link
Member Author

greg0ire commented Feb 27, 2024

Sure, but people will still get the deprecation about Criteria::ASC and when they try to address it, 💥

@derrabus
Copy link
Member

Sure, but people will still get the deprecation about Criteria::ASC and when they try to address it, 💥

They can use Order::Ascending->value instead. But we could also copy those constants over to the ORM repository.

@derrabus
Copy link
Member

See doctrine/orm#11315 for a green build in ORM 2.18.x

@greg0ire
Copy link
Member Author

greg0ire commented Feb 27, 2024

They can use Order::Ascending->value instead.

And then we ask them to migrate to Order::Ascending? That would be two migrations in a short time… not a great UX.

But we could also copy those constants over to the ORM repository.

Maybe it's best if each package has its own constants or enum, yes 🤔 It would avoid this whole situation entirely. In general, a lot of the pain in Doctrine is caused by exposing types of other packages through inheritance or like this, by using them in our public API.

@bobvandevijver
Copy link

I got about 200 deprecation notices in a single project because of the deprecation of the Criteria::ASC / Criteria::DESC, and while migrating to Order::Ascending->value might be an option, I do not like having to migrate that twice. Next to that, the deprecation of a class constant is a lot easier to find for certain static analysis tooling compared to deprecating an argument type in the future (for example for the ORM\OrderBy attribute), so I would indeed opt to revert the deprecation for now until at least ORM accepts the new Order enum.

I do like the enum by the way, so kudos on having that option! 👍🏻

In downstream packages that representation is recommended in places
where type hints do not allow using an enum yet, which means we would
have to modify the public API of downstream packages to allow it.

Let us postpone the deprecation until we figure what exactly needs to be
done.
@greg0ire
Copy link
Member Author

greg0ire commented May 3, 2024

Just rebasing to know whether Codecov works, but ultimately I will close this, IIRC it's no longer relevant.

@greg0ire
Copy link
Member Author

greg0ire commented May 3, 2024

Works great.

@greg0ire greg0ire closed this May 3, 2024
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.

3 participants