Problem/Motivation
In QueryBase
there are several functions: aggregate()
, conditionAggregate()
, sortAggregate()
, and groupBy()
, which contain {@inheritdoc}comments indicating that these functions are overrides of functions provided by the QueryAggregateInterface
, but QueryAggregateInterface
is not implemented by QueryBase
.
Proposed resolution
Do one of the following:
- Fully implement the QueryAggregateInterface.
- Do not implement QueryAggregateInterface and rework the code in QueryBase.php.
All the patches so far (up to #15) use option 1, implement QueryAggregateInterface on QueryBase.
Remaining tasks
A decision is need on which of the two proposed resolutions should be used.
Additional analysis:
- Check the class hierarchy: are these methods declared in any parent class or interface?
- Check the classes that extend
QueryBase
. Do they implementQueryAggregateInterface
? - (hard) See whether these methods are ever called.
- Try removing the methods and see whether any automated tests fail.
None found
There are four classes the extend QueryBase, only Null implement QueryAggregateInterface
Comment | File | Size | Author |
---|---|---|---|
#31 | 2489538-31.patch | 3.96 KB | quietone |
| |||
#31 | interdiff-29-31.txt | 409 bytes | quietone |
Comments
Comment #1
jibla CreditAttribution: jibla commentedI am at DrupalCon LA. I will take care of this issue.
Comment #2
jibla CreditAttribution: jibla commenteduploading the patch.
Comment #4
jibla CreditAttribution: jibla commentednew patch that passes through tests.
Comment #5
jibla CreditAttribution: jibla commentedComment #6
mradcliffeI think these should have an @todo written for them following: https://www.drupal.org/coding-standards/docs#todo
Is there a follow-up issue for implementing the code already, @drupaldeep?
There is a trailing white space here that should be removed.
This will need testing on all database systems.
Comment #7
xpsusa CreditAttribution: xpsusa at CivicActions commentedComment #8
benjifisherI expanded the issue summary and added the "Needs beta evaluation" tag. My guess is that, unless we find symptoms of this problem, it will not pass the beta evaluation.
Comment #10
cilefen CreditAttribution: cilefen commentedComment #11
cristian100Rerolled patch against Drupal 8.2.x-dev and also updated code to drupal coding standard.
Comment #12
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedComment #13
andypostLooking through classes inherited from this I see
\Drupal\Core\Entity\Query\Null\Query
is the only one that implements this interface, so at least this one should be updated.The other 2 ones
\Drupal\Core\Config\Entity\Query\Query
&\Drupal\Core\Entity\KeyValueStore\Query\Query
does not care about aggreagation, so looks could inherit the same code from base classis out of scope, we deal with such issues in child issues of #2571965: [meta] Fix PHP coding standards in core
That's really debatable where implementation should live
Comment #14
radubutco CreditAttribution: radubutco commentedComment #15
radubutco CreditAttribution: radubutco commentedAdded the missing implementations in QueryBase for the methods added in the previous patch.
Removed the already implemented methods from one class which extends the QueryBase class.
Removed the implementing of QueryInterface and QueryAggregateInterface from the classes which extends the QueryBase class.
Comment #16
vladsocaciu CreditAttribution: vladsocaciu commentedComment #25
quietone CreditAttribution: quietone as a volunteer commentedReading the issue there I don't see that there is agreement on which of the two proposed resolutions is to be implemented. All the patches seem to implement option #1 without any discussion. Because of that I am removing the novice tag.
A question was asked in #13.2 if three methods should be moved to QueryBase , that needs to be answered.
Despite all that, I choose to reroll that patch so that coding standard fixes to unrelated lines could be removed and make it easier to read.
And I did some research for two of the remaining tasks.
1) The methods are not declared in any parent class or interface.
2) There are four classes the extend QueryBase, only Null implement QueryAggregateInterface
Comment #28
quietone CreditAttribution: quietone as a volunteer commentedReroll.
Comment #29
BS Pavan CreditAttribution: BS Pavan at Srijan | A Material+ Company commentedTried to fix the failed test case.
Please review
Comment #30
daffie CreditAttribution: daffie commentedThere is a missing use statement for the ConditionAggregate class.
Comment #31
quietone CreditAttribution: quietone as a volunteer commented@daffie, thanks.
Comment #32
daffie CreditAttribution: daffie commentedI do agree about moving the aggregate methods to the QueryBase class. To me it is the wrong place. When you a basic entity query the aggregate method should not be there. The best solution to me would be to create a new class called QueryAggregateBase which extends QueryBase and is the base class for all aggregate entity queries. Just my opinion, feal free to disagree.
When you call the first or the second moved methods from a basic entity query you will get an error, because the variable
$this->conditionAggregate
is null.