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

Files: 
CommentFileSizeAuthor
#9 2096271_4.patch8.93 KBchx
PASSED: [[SimpleTest]]: [MySQL] 58,445 pass(es).
[ View ]
#4 2096271_4.patch8.94 KBchx
PASSED: [[SimpleTest]]: [MySQL] 58,804 pass(es).
[ View ]
#4 interdiff.txt1.7 KBchx
better_eq.patch8.54 KBchx
PASSED: [[SimpleTest]]: [MySQL] 58,872 pass(es).
[ View ]

Comments

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.

Status:Needs review» Reviewed & tested by the community

Nice!

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.

StatusFileSize
new1.7 KB
new8.94 KB
PASSED: [[SimpleTest]]: [MySQL] 58,804 pass(es).
[ View ]

Status:Needs work» Reviewed & tested by the community

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.

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.

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.

StatusFileSize
new8.93 KB
PASSED: [[SimpleTest]]: [MySQL] 58,445 pass(es).
[ View ]

Fine.

Status:Needs review» Reviewed & tested by the community

Thank you!

Title:Make it easier to extend entity queryChange 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.

Title:Change notice: Make it easier to extend entity queryMake 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.

Issue summary:View changes

Updated issue summary.