[4.0] Query builder has no way to reset query parts once set

[mbabker]

Seeing #6179 land, which deprecates getQueryPart() and getQueryParts() in the upcoming 3.8 release, alerted me to the PR it is based on (#3836) which removed those methods and their reset counterparts (resetQueryPart() and resetQueryParts()) in 4.0. As a side effect of the changes from #3836, it is now no longer possible to remove something from the query builder once it's added in.

Example Use Case

In Pagerfanta, there is an adapter for a DBAL query builder which performs a SELECT query against a single table. In this adapter, the Closure which modifies the query builder to change the query to a SELECT COUNT()... style query uses the query builder's resetQueryPart() method to clear the ORDER BY part from the query. This behavior was introduced as a bug fix to resolve an issue with an incorrect query being created on certain platforms.

[greg0ire]

Cc @BenMorel @morozov

[derrabus]

Would introducing a method resetOrderBy() (possibly more fir other query parts) solve this issue?

[morozov]

It should and we can do that. But at the same time, it feels like resetting a part of the query is a workaround for poor design on the application side. If the application doesn't need the query results to be ordered, why does it specify the order to begin with?

Instead of the current logic as I understand it:

$builder->orderBy($order);

if (weNeedCount()) {
    $builder->resetOrderBy();
}

It should behave like this:

if (! weNeedCount()) {
    $builder->orderBy($order);
}

[derrabus]

If the application doesn't need the query results to be ordered, why does it specify the order to begin with?

My understanding of the issue is that we're talking about a component that applies pagination to an arbitrary query represented as a QueryBuilder object. That paginator needs to know, how many rows the unlimited query would yield, so it clones the QB, resets the "order by" part and replaces the selected columns with COUNT(*).

It is totally reasonable that the original QB contains the "order by" part and I find it okay to make that part resetable.

[mbabker]

If the application doesn't need the query results to be ordered, why does it specify the order to begin with?

On the application side, I agree wholeheartedly.

My understanding of the issue is that we're talking about a component that applies pagination to an arbitrary query represented as a QueryBuilder object. That paginator needs to know, how many rows the unlimited query would yield, so it clones the QB, resets the "order by" part and replaces the selected columns with COUNT(*).

Exactly this. In the Symfony ecosystem, there are Pagerfanta and KnpPaginatorBundle (the KNP package also provides a DBAL integration that uses a similar workflow and is hit by this same deprecation) which provide pagination components for applications to use, both of which have some sort of abstraction layer which lets them integrate with a number of libraries. In that abstraction layer, both have a mechanism for taking a SELECT query for a paginated data set and converting that to a SELECT COUNT(*) style query to get the total item count, most commonly used to render a pagination component on a site's frontend.

[morozov]

I don't mind the missing API being implemented, please feel free to file a pull request.

[derrabus]

Up for a PR, @mbabker? If yes, please target 3.8.x.

[BenMorel]

I don't mind either. I can work on a PR, unless @mbabker wants to do it!

[mbabker]

@BenMorel If you beat me to it, then go for it. Otherwise I can get to it at some point over the weekend (got other query issues to sort out for work today 😅 ).

[BenMorel]

@derrabus I'm not sure if targeting 3.8.x is a good idea:

• we can indeed add resetFrom() resetJoin() etc. methods in 3.8.x (but porting them to 4.0.x will not be straightforward)
• we could add getFrom(), getJoin() etc. in 3.8.x, but their return type would be very different from the return type they'll have in 4.0.x, as 3.8.x lacks value objects such as From and Join

Regarding the second point, I don't think implementing these methods will help consumers of the QueryBuilder, as they will have to change their code anyway if they use these methods in 3.8.x then upgrade to 4.0.x. This could possibly make the upgrade even more painful than not having these methods in the first place.

I would suggest to just implement them in 4.0.x, and document the upgrade path from (get|reset)QueryPart() to (get|reset)(From|Join|OrderBy|...)() in UPGRADE.md.

[BenMorel]

Another solution would be to backport the From / Join / ... value objects from 4.0.x to 3.8.x, and provide a forward-compatible API in getFrom() / getJoin() / etc.

Thoughts, @morozov?

[derrabus]

we can indeed add resetFrom() resetJoin() etc. methods in 3.8.x

So let's do that.

(but porting them to 4.0.x will not be straightforward)

Can you elaborate?

we could add getFrom(), getJoin()

I don't think that we need those.

they will have to change their code anyway if they use these methods in 3.8.x then upgrade to 4.0.x.

This does not apply to the proposed reset*() methods, does it?

[BenMorel]

(but porting them to 4.0.x will not be straightforward)

Can you elaborate?

I mean, merging into 4.0.x will require changing the implementation. Not that big of a deal.

we could add getFrom(), getJoin()

I don't think that we need those.

Don't paginators need to introspect the QueryBuilder in order to perform their job?

they will have to change their code anyway if they use these methods in 3.8.x then upgrade to 4.0.x.

This does not apply to the proposed reset*() methods, does it?

No, only to getters.

[derrabus]

Okay, we have a resetOrderBy() method now. Should we add more resetters for the sake of consistency or shall we leave it as it is?

[BenMorel]

Should we add more resetters for the sake of consistency

Yes, I think they should have been added in the same PR!

[derrabus]

Following up on @mbabker's work, I've created #6193. I don't think that we need replacements for every part, tbh.

[github-actions[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.