Problem/Motivation

Entity Query sucks! Really! erm. Where was I? Yes. Currently you need to implement all five classes (QueryFactory, Query, QueryAggregate, Tables, Condition) to add a single line to code to, say Tables. This takes unnecessary boilerplate to a new level.

Proposed resolution

Make only an empty QueryFactory mandatory. Then grab the namespace of it and the namespace of any ancestors and use the first class that exists within those namespaces. For eg. if Drupal\user\Entity\Query\Sql\QueryFactory extends Drupal\Core\Entity\Query\Sql\QueryFactory then we will look for the rest (Tables, Condition, Query, QueryAggregate) first in Drupal\user\Entity\Query\Sql then in Drupal\Core\Entity\Query\Sql. Much like DBTNG drivers just a bit more flexible.

Remaining tasks

Commit this.

User interface changes

None.

API changes

If you were extending Entity Query, you can drop a little boilerblate.

#1751274: Do not query user_roles directly
#2038707: Entity query sql backend limits storage controllers changes in contrib

CommentFileSizeAuthor
#9 2096271_4.patch8.93 KBchx
#4 2096271_4.patch8.94 KBchx
#4 interdiff.txt1.7 KBchx
better_eq.patch8.54 KBchx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

Status: Active » Needs review

Note that when this and #1751274: Do not query user_roles directly both is in, we can drop the three new classes from that issue or commit this first and don't even commit those. The test in that issue is the test for this issue too.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Nice!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Config/Entity/Query/Query.php
    @@ -44,8 +44,8 @@ class Query extends QueryBase implements QueryInterface {
    -  function __construct($entity_type, $conjunction, EntityManager $entity_manager, StorageInterface $config_storage) {
    -    parent::__construct($entity_type, $conjunction);
    +  function __construct($entity_type, $conjunction, EntityManager $entity_manager, StorageInterface $config_storage, array $namespaces) {
    +    parent::__construct($entity_type, $conjunction, $namespaces);
    

    We're missing the docblock for $namespace parameter.

  2. +++ b/core/lib/Drupal/Core/Entity/Query/QueryBase.php
    @@ -185,8 +193,8 @@ public function range($start = NULL, $length = NULL) {
    +    $class = self::getClass($this->namespaces, 'Condition');
    

    Why are we using self here? Let's add a comment.

chx’s picture

FileSize
1.7 KB
8.94 KB
chx’s picture

Status: Needs work » Reviewed & tested by the community
dawehner’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/lib/Drupal/Core/Entity/Query/QueryBase.php
@@ -192,8 +192,10 @@ public function range($start = NULL, $length = NULL) {
+  final protected function conditionGroupFactory($conjunction = 'AND') {

I can't set myself a RTBC under a code in drupal which uses final, sorry.

chx’s picture

Erm, QueryFactory hardwires QueryBase (because it kind of needs to). If a class extends QueryBase and changes this method then we have a hilarious situation where the query is finding different classes to the factory. Let's not allow for that. For that matter, most of our utility classes are final as well.

dawehner’s picture

Every final is wrong and just makes the life of people who actually cares about unit testing a HELL, let me repeat it a HELL. No matter whether there are good intentions behind the usage of it.

chx’s picture

FileSize
8.93 KB

Fine.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you!

alexpott’s picture

Title: Make it easier to extend entity query » Change notice: Make it easier to extend entity query
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed 6976d52 and pushed to 8.x. Thanks!

We need a change notification that is basically the content on the issue summary and can point to #1751274: Do not query user_roles directly as an example of the benefits of this change.

chx’s picture

Title: Change notice: Make it easier to extend entity query » Make it easier to extend entity query
Status: Active » Fixed
Issue tags: -Needs change record

We already had a change notice I just needed to update it https://drupal.org/node/2070303

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.