Entity query sql backend limits storage controllers changes in contrib

This issue will potentially be solved by #1497374: Switch from Field-based storage to Entity-based storage

Thanks for the bug report, this is very interesting. I am fairly certain this is not the solution we will go with because it puts an SQL-only thing on the entity class. The simplest would be put this on the storage controller? Then you would only need to override the storage controller and that'd be it.

Alternatively, we could look at an extender-like solution for the entity query system -- would that help, I wonder? Could we put the aggregator under it?

Here's a patch that puts a getTables() method into the DatabaseStorageController class. This method allows us to do everything we need (because we can swap 'Tables' out for our own class that extends it very easily.

It feels like the getTables() method would fit better on Drupal\Core\Entity\Query\Sql\Query, however if you put the method there you have to override every class in Drupal\Core\Entity\Query\Sql namespace.

I think the only other option is to make it so you can pass the Table's class into the constructor of Drupal\Core\Entity\Query\Sql\Query or add a setTables() method to all of those classes. Then it would be possible to register a new service for a given entity type and have a new factory object that passes a different Tables class in, but that uses the same Drupal\Core\Entity\Query\Sql classes.

Forgot to add the Tables class to the list at the top.

Here's another go.

It would be great if the issue summary would describe why the chosen route was implemented (A not B).

As chx suggested in IRC, there might be better a TableInterface.

Thanks much! I think making Tables pluggable makes a tremendous amount of sense -- we made everything pluggable here, the Query, the Condition class so why not Tables. However, this doesn't belong to the storage controller either, it's an internal affair to the query class. I have made it so that just overriding QueryFactory, Query, QueryAggregate in a new namespace without any methods will behave appropriately: QueryFactory will construct Query and QueryAggregate in that namespace and Query will use the Tables again in that namespace.

Discussed the removal of conditionGroupFactory from the interface with alexpott and he greenlighted the API change.

PHP sucks. __NAMESPACE__ is the defining class, there's no way to get the current namespace aside from string parsing the current class name.

That regex can be simplified a little.

+++ b/core/lib/Drupal/Core/Entity/Query/ConditionFundamentals.php
@@ -30,13 +30,21 @@
+  public function __construct($conjunction, QueryInterface $query) {

After adding the QueryInterface object to the __construct arguments, I think you need to specify:

use Drupal\Core\Entity\Query\QueryInterface;

Here is an attempt to fix the test fail...

No, I was wrong, the issue isn't a missing 'use ... ;', it is that in \Drupal\Core\Entity\Query\Sql\QueryAggregate::conditionAggregateGroupFactory a new ConditionAggregate object is being created:

  /**
   * Implements \Drupal\Core\Entity\Query\QueryAggregateInterface::conditionAggregateGroupFactory().
   */
  public function conditionAggregateGroupFactory($conjunction = 'AND') {
    return new ConditionAggregate($conjunction);
  }

ConditionAggregate extends ConditionAggregateBase which extends ConditionFundamentals.
Since the __construct() for ConditionFundamentals this now needs a QueryInterface object:

+++ b/core/lib/Drupal/Core/Entity/Query/ConditionFundamentals.php
@@ -30,13 +30,21 @@
+  public function __construct($conjunction, QueryInterface $query) {

we need to add $query to the arguments for conditionAggregateGroupFactory().

Ok, I ran tests this time before patching, and this seems to pass the test, although it seems almost too simple. I passed '$this' as a second argument to conditionAggregateGroupFactory() as $this is a QueryAggregate object and thus and extension of QueryInterface.

Let me know if this is wrong...

Thanks so much! Welcome to your first core patch :) and that is correct.

@@ -171,6 +171,24 @@ public function range($start = NULL, $length = NULL) {
+   * @param $conjunction
...
+   * return \Drupal\Core\Entity\Query\ConditionInterface

Let's nitpick ... this should be @param string $conjunction and @return instead.

@@ -171,6 +171,24 @@ public function range($start = NULL, $length = NULL) {
+    preg_match('/^.*\\\\/', get_class($this), $matches);
+    $class = $matches[0] .'Condition';

@@ -320,4 +314,15 @@ public function __clone() {
+    preg_match('/^.*\\\\/', get_class($this), $matches);
+    $class = $matches[0] .'Tables';

@@ -48,8 +48,10 @@ function __construct(Connection $connection) {
+    preg_match('/^.*\\\\/', get_class($this), $matches);
+    $class = $matches[0] .'Query';

@@ -64,8 +66,10 @@ function get($entity_type, $conjunction, EntityManager $entity_manager) {
+    preg_match('/^.*\\\\/', get_class($this), $matches);
+    $class = $matches[0] .'QueryAggregate';

It would seriously help to document what this regex does.

@@ -15,6 +15,11 @@
   /**
+   * @var \Drupal\Core\Entity\Query\Sql\Query
+   */
+  protected $query;

Let's also add some sentence about it if we already add some documentation

@@ -320,4 +314,15 @@ public function __clone() {
+   * Gets the Tables object for this query.
+   *

Missings docs for the parameter.

@@ -320,4 +314,15 @@ public function __clone() {
+  public function getTables(SelectInterface $sql_query) {

Is there a reason for this function to be public and not protected? I can't find a call outside of the class.

@@ -0,0 +1,29 @@
+
...
+

Remove these empty lines to use them later :p

@@ -0,0 +1,29 @@
+interface TablesInterface {
+  /**
...
+  public function addField($field, $type, $langcode);
+}

Let's put some empty lines in between.

one thing i missed: There should be a space after the ., so for example

$class = $matches[0] . 'QueryAggregate';

Let's also introduce a static helper method on the QueryBase so we don't end up with the same regex 4 times.

Actually, I have reviewed the PHP docs (I though I already did) and the following syntax is valid:

$a = new namespace\sub\cname(); // instantiates object of class MyProject\sub\cname

Please change / simplify the code to use this.

> Is there a reason for this function to be public and not protected? I can't find a call outside of the class.

Condition calls it.

Ah yes, that's not good enough. Posted a question to http://stackoverflow.com/questions/18091684/tthe-namespace-equivalent-of... here and here's the helper.

+++ b/core/lib/Drupal/Core/Entity/Query/QueryBase.phpundefined
@@ -402,4 +419,17 @@ protected function getAggregationAlias($field, $function) {
+   * @param $object

+++ b/core/lib/Drupal/Core/Entity/Query/Sql/Query.phpundefined
@@ -320,4 +314,17 @@ public function __clone() {
+   * @return \Drupal\Core\Entity\Query\Sql\TablesInterface

Needs extra docblock bits.

+++ b/core/lib/Drupal/Core/Entity/Query/QueryBase.phpundefined
@@ -402,4 +419,17 @@ protected function getAggregationAlias($field, $function) {
+  public static function getNamespace($object) {
+    return preg_replace('/.[^\\\\]+$/', '', get_class($object));

Looks kosher, I don't know of anything in PHP to do this...

+++ b/core/lib/Drupal/Core/Entity/Query/Sql/Query.phpundefined
@@ -289,7 +283,7 @@ protected function result() {
-      $this->tables = new Tables($this->sqlQuery);
+      $this->tables = $this->getTables($this->sqlQuery);

This is great.

Added extra doxygen and changed getNamespaces to not use a regex cos this is ever so slightly faster. And fixed getTables to actually do what we wanted it to do, the namespace stuff got stuck in there.

This patch tends to go pear shaped...

@@ -402,4 +419,18 @@ protected function getAggregationAlias($field, $function) {
+   * @param $object

Is @param object $object a valid syntax?

@@ -0,0 +1,29 @@
+  public function addField($field, $type, $langcode);

Missing docs for $langcode

@dawehner

Is @param object $object a valid syntax?

Looks like Drupal\Core\Entity\Query::compile uses the same syntax, and I can't find any use of '@param object $foo'.

   * @param $query
   *   The query object this conditional clause belongs to.
   */
  public function compile($query);

Maybe it's fine as is?

Missing docs for $langcode

Have added docs. Feel free to nitpick.

Thank you! Here are some nitpicks.

@@ -50,7 +50,20 @@ public function __construct(SelectInterface $sql_query) {
+   *   The language code the field values are to be shown in.

@@ -13,12 +13,16 @@
+   *   The language code the field values are to be shown in.
...
    * @return string

Let's put some empty lines between there.

@@ -50,7 +50,20 @@ public function __construct(SelectInterface $sql_query) {
+   * @throws \Drupal\Core\Entity\Query\QueryException

@@ -13,12 +13,16 @@
+   * @throws \Drupal\Core\Entity\Query\QueryException

I guess it would make sense to explain when this exception is thrown, because it does not seem to obvious.

No problem!

Thank you!

+++ b/core/lib/Drupal/Core/Entity/Query/Sql/Condition.phpundefined
@@ -15,6 +15,13 @@
   /**
+   * The SQL entity query object this condition belongs to.
+   *
+   * @var \Drupal\Core\Entity\Query\Sql\Query
+   */
+  protected $query;

This is already declared on ConditionFundementals (which this inherits from) and therefore does not need to be here.

+++ b/core/lib/Drupal/Core/Entity/Query/Sql/TablesInterface.phpundefined
@@ -0,0 +1,36 @@
+interface TablesInterface {

We're adding this interface but we're not using it? I guess it was supposed to be used by the Tables class

> This is already declared on ConditionFundementals (which this inherits from) and therefore does not need to be here.

Nope, the generic is merely QueryInterface, this specific $query is a Sql one so separate doxygen is justified.

@chx pointed out that the protected $query; is good because it declares that this is an instance of @var \Drupal\Core\Entity\Query\Sql\Query which defines the getTables() method.

xpost

Removed the addField doxygen in favor of {@inheritdoc} . This is ready . interdiff is against #40.

Catch and I have discussed this offline and have agreed this is a good change to make it easier to provide entity storage controllers in contrib.

Committed 265940c and pushed to 8.x. Thanks!

Created Extending the default entity query backend.