[4.0] Query builder - how to reset `From` query part?

[kingjia90]

Q	A
Version	4.0.0

Support Question

Related https://github.com/doctrine/dbal/issues/6186#issuecomment-1762278908

How should i do to reset the from query part from the Query Builder? Unlike the select, the only available method related to FROM part is pushing an array https://github.com/doctrine/dbal/blob/61d79c6e379a39dc1fea6b4e50a23dfc3cd2076a/src/Query/QueryBuilder.php#L676

Am i missing something or is this a regression/bug? TIA for the support

[derrabus]

You can't reset the from/join parts.

[greg0ire]

@kingjia90 can you elaborate on why you need to reset the from query part? I see you're linking this issue from a pimcore repository, so I suppose it must be fairly tricky.

Am i missing something or is this a regression/bug?

We think such a method is not needed. If you feel otherwise, please demonstrate why.

[mbabker]

Pimcore's got the same issue I've run into myself in client projects using Pagerfanta's DBAL adapter.

Here's the relevant code in Pimcore and here's the Pagerfanta adapter.

The issue both of those pieces of code have is that they're designed to edit an existing query builder, so in Pimcore's case specifically (and I had similar code in one of my client projects before refactoring some things), they're taking the SQL statement that the query builder generated, extracting it out, then resetting the query builder to turn it into a count query wrapping the original statement. Removing resetQueryParts() entirely breaks that workflow, and as pointed out, there just aren't APIs to clear or reset some parts in 4.0.

My change for my client projects was to refactor how the count query gets built and use a callback that returns a new query builder instance instead of relying on being able to manipulate the builder provided to the callback (and inherently rely on being able to do a full reset).

DBAL 3.x compatible callback:

public function createCountQueryBuilder(QueryBuilder $queryBuilder): void
{
    $query = (string) $queryBuilder;

    $queryBuilder->->resetQueryParts()
        ->select('COUNT(*)')
        ->from('(' . $query . ')', 'tmp')
    ;
}

DBAL 4.x compatible function:

public function createCountQueryBuilder(QueryBuilder $queryBuilder): QueryBuilder
{
    return $this->getEntityManager()
        ->getConnection()
        ->createQueryBuilder()
        ->select('COUNT(*)')
        ->from('(' . $queryBuilder->getSQL() . ')', 'tmp')
        ->setParameters($queryBuilder->getParameters(), $queryBuilder->getParameterTypes())
    ;
}

[greg0ire]

Thanks for clarifying. Conceptually, if I followed, it seems both of you have a user-supplied query builder that is going to give results, and you want to build another query from it: a count query, for pagination purposes. In that case, I think it would feel more natural to start afresh like you did in your DBAL 4.x compatible function, but I must admit it's not very convenient. I think maybe we could consider reintroducing resetQueryParts() (or another name), but this time:

• only allow it to reset all query parts (no arguments)
• returning a clone of the original query builder, with the parameters preserved.

Maybe we should even go as far as recognizing this as the sole legit use case for this and introducing wrapInCount() instead :thinking:

[kingjia90]

Thank you @derrabus for the quick confirmation Thank you @mbabker for helping clarifying the issue, it is spot-on

Thank you @greg0ire for taking a deeper look. I am totally fine with the proposed solutions. Maybe It depends on the stance of the query builder to support or discourage the usage of sub-queries (in the case of the count, the subquery was needed under the FROM part but could be cases where it's used in SELECT,JOIN for example), resetQueryParts looks like it would be more flexible.

[derrabus]

there just aren't APIs to clear or reset some parts in 4.0.

Well, except for resetWhere(), resetGroupBy(), resetHaving(), resetOrderBy(), …

I mean, we've been open to making parts of the QB resettable in the past (see #6186) and you @mbabker were very much involved back then. From that perspective, your comment irritates me a bit.

I'm looking at the use-case you've presented:

public function createCountQueryBuilder(QueryBuilder $queryBuilder): void
{
    $query = (string) $queryBuilder;

    $queryBuilder->resetQueryParts()
        ->select('COUNT(*)')
        ->from('(' . $query . ')', 'tmp')
    ;
}

No offense, but this is horrible: What's the point in resetting the entire builder when you could simply create a new one?

We could certainly think about adding more resetters where it makes sense. Talk to us, present a valid use-case and we will greenlight it. But resetting the whole thing won't happen anymore. Create a new builder (as you've done in your "4.0 compatible" version).

[mbabker]

No offense, but this is horrible: What's the point in resetting the entire builder when you could simply create a new one?

That API design dates back well before I inherited Pagerfanta, and I went with the "it ain't broke so don't touch it" approach since then. Except I guess it is kind of broken with DBAL 4.0, and I haven't really had the energy to revise the adapter to deprecate the existing callback strategy in favor of the 4.0 compat code I shipped for a client project (which I do think is a better API design overall, just need to make it happen at some point).

[derrabus]

That API design dates back well before I inherited Pagerfanta, and I went with the "it ain't broke so don't touch it" approach since then.

That's totally fair. 🙂

So, creating a new query builder is the way to go in your case. Does this also solve the problem for Pimcore, @kingjia90?

[kingjia90]

Yes, it would be also fine to follow that blueprint of creating a new query builder, even though in our case it may be trickier and inconvenient, but we will find a way to deal with it

Thank you again and i am closing as resolved

[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.