Problem/Motivation

Drupal\Core\Entity\Query\Sql\Query::__clone() re-parents and resets the regular $condition object, but leaves $conditionAggregate pointing at the same ConditionAggregate instance as the original query. As a result, cloning an aggregate query produces a clone that shares its aggregate condition tree with the source: calling conditionAggregate() on either query changes both, and the still-bound query reference means executing the clone can build SQL against the wrong query object.

Proposed resolution

Clone $conditionAggregate and set the query on the copy (mirroring the existing handling of $condition), so a cloned aggregate query gets its own isolated aggregate condition tree.

Remaining tasks

Review.

User interface changes

Nope.

Introduced terminology

N/A

API changes

Nope.

Data model changes

Nope.

Release notes snippet

N/A

Issue fork drupal-3593233

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

amateescu created an issue. See original summary.

amateescu opened merge request !15960

amateescu’s picture

amateescu commented
Status: Active » Needs review

Found this while reviewing #2875033: Optimize joins and table selection in SQL entity query implementation.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Visually looks great. There was one random functional test failure but on re-running it passed. Can't think of anything else.

amateescu’s picture

amateescu commented

Ran the test-only job as well to prove the failure :)

godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Needs review

Questions on the MR.

amateescu’s picture

amateescu commented

Thanks for reviewing!

I fixed those points by adding both setQuery() and getQuery() to ConditionAggregateInterface, which should be fine because they're both implemented in ConditionFundamentals.

If we ever decide in a followup that ConditionAggregateInterface should extend ConditionInterface (not sure whether we actually want that tbh) we can remove them, but until then I think this is the easiest way forward here.

And while doing the change in QueryBase, I discovered that this is actually the sister issue for something I found 14 years ago :)) #1829942: Cloning an entity query doesn't clone its conditions

godotislate’s picture

godotislate
he/him
commented

I fixed those points by adding both setQuery() and getQuery() to ConditionAggregateInterface, which should be fine because they're both implemented in ConditionFundamentals.

I think this is fine, and we should have a CR for the interface change.

amitgoyal’s picture

amitgoyal commented
Status: Needs review » Reviewed & tested by the community

Created a Change Record for the ConditionAggregateInterface API addition in this MR: https://www.drupal.org/node/3603991

With all MR threads resolved, CI passing, and the CR in place, this should be ready for RTBC.

godotislate’s picture

godotislate
he/him
commented

@amitgoyal can you disclose whether you used AI to help you compose the CR?

amitgoyal’s picture

amitgoyal commented

@godotislate - yes, I have used AI to compose the CR but reviewed it myself before submitting it.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

I've fixed up the CR by formatting it properly and removing extraneous info.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Discussed with @catch we agreed this belongs in 11.5.x too.

Committed and pushed 3e4104d1b00 to main and fb8ea960b77 to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • alexpott committed fb8ea960 on 11.x 
    fix: #3593233 Cloning an aggregate entity query shares its aggregate...

  • alexpott committed 3e4104d1 on main 
    fix: #3593233 Cloning an aggregate entity query shares its aggregate...
amateescu’s picture

amateescu commented
Version: 11.x-dev » 11.4.x-dev
Status: Fixed » Reviewed & tested by the community

As a bug fix, this should go into 11.4.x as well. It's actually a followup for #2875033: Optimize joins and table selection in SQL entity query implementation, because it fixes something that was missed there :)

catch’s picture

catch
he/him
Primary language English
commented

It's a bugfix but it adds new methods to an interface which we'd normally only do in a minor release.

amateescu’s picture

amateescu commented

Right, it adds the same methods that were added by #2875033: Optimize joins and table selection in SQL entity query implementation on 11.4.x

godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Needs work

I think we could backport it if we left out the interface change and checked that the method exists?

    if (isset($this->conditionAggregate) && method_exists($this->conditionAggregate, 'setQuery')) {
      $this->conditionAggregate->setQuery($this);
    }

godotislate closed merge request !15960

godotislate opened merge request !16287

godotislate’s picture

godotislate
he/him
commented
Status: Needs work » Needs review

MR 16287 for 11.4.x per #20.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Seems like a good backport.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.4.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • catch committed 1db960a1 on 11.4.x 
    fix: #3593233 Cloning an aggregate entity query shares its aggregate...

catch closed merge request !16287

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.