Entity query hardcodes entity_reference and entity specifier

@dpi can you try this patch? Thanks for the help @chx.

The fix in #6 is correct but it doesn't fix the issue for DER. We have to do it in followup issue for DER to fix that.
Here is a sample code I tried.

use Drupal\field\Entity\FieldConfig;
use Drupal\field\Entity\FieldStorageConfig;

\Drupal::service('module_installer')->install(array('dynamic_entity_reference'));
FieldStorageConfig::create(array(
  'field_name' => 'field_der',
  'type' => 'dynamic_entity_reference',
  'entity_type' => 'node',
  'cardinality' => 1,
  'settings' => array(
    'exclude_entity_types' => FALSE,
    'entity_type_ids' => array(
      'user' => 'user',
    ),
  ),
))->save();

FieldConfig::create(array(
  'field_name' => 'field_der',
  'entity_type' => 'node',
  'bundle' => 'article',
  'label' => 'Foo Bar',
  'settings' => array(),
))->save();

print_r(\Drupal::entityQuery('node')
  ->condition('field_der.entity.name', 'admin', 'STARTS_WITH')
  ->count()
  ->execute());

exception 'Drupal\Component\Plugin\Exception\PluginNotFoundException' with message 'The "" entity type does not exist.' in            [error]
/vagrant/app/core/lib/Drupal/Core/Entity/EntityManager.php:257
Stack trace:
#0 /vagrant/app/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php(210): Drupal\Core\Entity\EntityManager->getDefinition(NULL)
#1 /vagrant/app/core/lib/Drupal/Core/Entity/Query/Sql/Condition.php(48):
Drupal\Core\Entity\Query\Sql\Tables->addField('field_der.entit...', 'INNER', NULL)
#2 /vagrant/app/core/lib/Drupal/Core/Entity/Query/Sql/Query.php(161):
Drupal\Core\Entity\Query\Sql\Condition->compile(Object(Drupal\Core\Database\Driver\mysql\Select))
#3 /vagrant/app/core/lib/Drupal/Core/Entity/Query/Sql/Query.php(87): Drupal\Core\Entity\Query\Sql\Query->compile()
#4 /vagrant/app/test.php(6): Drupal\Core\Entity\Query\Sql\Query->execute()
#5 /vagrant/vendor/drush/drush/commands/core/core.drush.inc(1074): include('/vagrant/app/te...')
#6 [internal function]: drush_core_php_script('test.php')
#7 /vagrant/vendor/drush/drush/includes/command.inc(359): call_user_func_array('drush_core_php_...', Array)
#8 /vagrant/vendor/drush/drush/includes/command.inc(210): _drush_invoke_hooks(Array, Array)
#9 [internal function]: drush_command('test.php')
#10 /vagrant/vendor/drush/drush/includes/command.inc(178): call_user_func_array('drush_command', Array)
#11 /vagrant/vendor/drush/drush/lib/Drush/Boot/DrupalBoot.php(46): drush_dispatch(Array)
#12 /vagrant/vendor/drush/drush/drush.php(74): Drush\Boot\DrupalBoot->bootstrap_and_dispatch()
#13 /vagrant/vendor/drush/drush/drush.php(11): drush_main()
#14 {main}

So $entity_type_id = $propertyDefinitions[$relationship_specifier]->getTargetDefinition()->getEntityTypeId(); in \Drupal\Core\Entity\Query\Sql\Tables:209 is returning NULL cuz DER can't add that constraint to the entity property.

Well #7 is kinda impossible to fix within EQ because we need the entity properties across the relationship, i mean ->condition('ref_field.entity.foo', 'bar') well what's foo and where the heck it is stored? I understand of course that this is a problem for DER... but I do not have any good ideas on solving it.

Well, condition is based on the "real world" entity API. $entity->author->entity->name turns into condition('author.entity.name'). I thought that neat.

I would vote on condition('uid.user.name'). Check the issue summary for code.

We already know we are looking for a relationship in this chunk of code; check a few lines above // If there are more specifiers to come, it's a relationship.

For the first part, if you were to change $entity_type_id = $propertyDefinitions['entity'] to $entity_type_id = $propertyDefinitions[$relationship_specifier] then you would have almost the same code as I suggested -- and for symmetry reason I still prefer mine.

As for the fallback that the relationship specifier equals the entity id -- well, that would mean that you have condition('fieldname.something.foo') where later $entity->something->foo wouldn't work. It would also make condition('uid.node.foo') work and wreak spectacular havoc -- the query would find nodes which IDs in place of users with those IDs. I do not think this is desirable.

So no, I do not like #13 sorry.

And to clarify: I am not against the whole idea, absolutely not, relation et al will rely on this, I am really quite glad we are working on this; I just would like to find a good solution. Thanks for perseverance!

looks like option 1 in #16 is exactly what @chx proposed in issue summary. +1 that!

Option #1

so @berdir said in IRC

but what if DER would expose a property definition for each enabled entity type? then entity query should not require any magic?

But that mean

 $node->derfield[0]->user
$node->derfield[0]->node
$node->derfield[0]->taxonomy_term
$node->derfield[1]->user
$node->derfield[1]->node
$node->derfield[1]->taxonomy_term
// ... and so on.

although I'm not 100% sure about the implications, of e.g. using that at runtime, how should $node->some_field->user behave, if a user is referenced, if a node is referenced?

Yeah that is a big problem.

I'm more worried about making it work at runtime, all that setValue() business and updates when a value is set is not trivial

That will be a mess.

I know node.entity doesn't make sense for DER in EFQ(I agree with @chx point that the mapping is there for a reason) but for all practical purposes(entity api and field api) it makes a lot of sense cuz i don't have to worry which entity_type is it $node->field_der[0]->enitity it just gives me all information.

While maintaining DER, my primary focus is to keep it in line with ER. So maybe we can replace core ER in D9 with DER. *wishful thinking* and this change will divert us so above change in DER is a no go. :(
Also @larowlan said in IRC it is an overkill and I agree.

Option #2

As @chx said
$node->uid->entity->name translate to

Drupal::entityQuery('node')
  ->condition('uid.entity.name', 'foobar', '=')
  ->execute();

Which makes a lot of sense.
For DER we have two choices.
Either map $node->field_der->entity->name to

Drupal::entityQuery('node')
  ->condition('field_der.entity:user.name', 'foobar', '=')
  ->execute();

Or

Drupal::entityQuery('node')
  ->condition('field_der.user.name', 'foobar', '=')
  ->execute();

'field_der.entity:user.name' is not neat but we are already doing it in a lot of places is core plugin system and views are the major examples.
'field_der.user.name' doesn't map to anything and it'll change the default behavior of EFQ and I wouldn't want that.

@chx With all due respect, I would like you to reconsider 'field_der.entity:user.name' option because as far as I can see this is the only viable option. If not please suggest something else.

Thank you @dpi for all the possible solution.

If we are not happy with condition('derfield.node.uid') what about a small variant of #11: condition('derfield.entity(node).uid')? This makes it crystal clear that this is not part of the entity after as $entity->derfield->entity(node)->uid will throw a fatal quite immediately. Also, the ordinary meaning of parentheses is

Use parentheses to include material that you want to de-emphasize or that wouldn't normally fit into the flow of your text but you want to include nonetheless.

Sounds the exact situation here, it doesn't fit the normal flow but we want to include it.

I have no problem with that.

But this is something that DER can decide. See the issue summary.

Sounds fair I'll create a followup for DER after this got fixed.

Last shot for tonight: if you pass the $relationship_specifier to getTargetDefinition and add a little preg to getTargetDefinition then entity(node) entity*node entity#node and whatever else can be made to work in one go: preg_match('/^entity[^a-z0-9_]+([a-z0-9_]+)/', $relationship_specifier, $matches) will give you an entity id and then you just need to fish out the definition for that, for example return \Drupal::entityManager()->getDefinition($matches[1]) .

So now how we'll make $propertyDefinitions[$relationship_specifier] instanceof DataReferenceDefinitionInterface true for DER.

So http://cgit.drupalcode.org/dynamic_entity_reference/tree/src/DataDynamic... @ 837d2df33e093491da325348cc6f9464d49d7704 has class DataDynamicReferenceDefinition extends DataReferenceDefinition and Drupal has class DataReferenceDefinition extends DataDefinition implements DataReferenceDefinitionInterface so your definition already implements that interface and I see DataDynamicReferenceDefinition being used in the field definition http://cgit.drupalcode.org/dynamic_entity_reference/tree/src/Plugin/Fiel... . What am I missing here?

Sorry for not being clear.
$propertyDefinitions['entity'] instanceof DataReferenceDefinitionInterface is true for DER but
$propertyDefinitions['entity(node)'] instanceof DataReferenceDefinitionInterface is not true.

Iterated proposal then: f you specify entity#node then we will look at the property called entity and pass entity#node to the getTargetDefinition. Still whatever separator after entity is accepted.

I still do not want Tables to just blindly accept foo.node.bar without the foo field having any say whether it has a "node" so this is the best I could come up with. This adds back entity as a special string but it's only a default -- if the foo field does define a node field of a type implementing DataReferenceDefinitionInterface then foo.node.bar will work just fine. Notably even foo.entity_type.bar would work if entity_type is a reference item.

That works for me. Let's see how much this fails.

1. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -203,9 +204,10 @@ public function addField($field, $type, $langcode) {
            }
            // Check for a valid relationship.
   -        if (isset($propertyDefinitions[$relationship_specifier]) && $field_storage->getPropertyDefinition('entity')->getDataType() == 'entity_reference' ) {
   +        $property = preg_match('/^entity([^a-zA-Z_0-9]|$)/', $relationship_specifier) ? 'entity' : $relationship_specifier;
   +        if (isset($propertyDefinitions[$property]) && $propertyDefinitions[$property] instanceof DataReferenceDefinitionInterface) {
              // If it is, use the entity type.
   -          $entity_type_id = $propertyDefinitions[$relationship_specifier]->getTargetDefinition()->getEntityTypeId();
   +          $entity_type_id = $propertyDefinitions[$property]->getTargetDefinition($relationship_specifier)->getEntityTypeId();
              $entity_type = $this->entityManager->getDefinition($entity_type_id);
              $field_storage_definitions = $this->entityManager->getFieldStorageDefinitions($entity_type_id);
   

   I don't really get how this is supposed to work?

   Shouldn't $property and $relationship_specifier be the other way round in the last changed line? Otherwise it would expect a property definition named 'node' to exist and then calls the method with entity(node)? Isn't this exactly what we want to avoid, otherwise we wouldn't have to do anything here?

   I am also not sure that the original change here makes sense. The check would now for example also return TRUE for the language item reference, but that makes no sense, because that is not something you can query on. Like this:

   condition('node.langcode.language.id'). I have no idea what that is even supposed to mean?

   We *are* looking for an entity reference here, not any kind of reference, because we only support entity references.

2. +++ b/core/lib/Drupal/Core/TypedData/DataReferenceDefinitionInterface.php
   @@ -20,9 +20,14 @@
   +   * @param string $relationship_specifier
   +   *   (optional) The relationship specifier for entity query.
   +   *
   +   * @see \Drupal\Core\Entity\Query\Sql\Tables::addField().
   

   The parameter and documentation isn't really explaining anything to me, or how I'm supposed to implement it.

I think that ^ shows that we somehow need tests for this, although I'm not quite sure how...

What is name(argument) ... ? I don't think I saw that in earlier patches.

^([a-zA-Z_0-9]*){1} why this is not ^([a-zA-Z_0-9]+) {1} is redundant to the best of my regex knowledge and you want + not * because foo.(bar).baz is illegal.

The getTargetDefinition doxygen is a bit weird, and $argument is also quite weird :/ Naming is hard. Maybe type_hint ? I would say something like "if multiple data types can be referenced by the object implementing this interface, pass this hint to get a relevant target definition" and the example ... well the example needs to move to the query interface because noone will find it here but it needs to carry a warning that core only supports single entity type references so (entity_type) is superflous.

I'm working on the realnames module.
The \Drupal\user\Plugin\EntityReferenceSelection\UserSelection handler builds an entity query which I would like to join to the realname db table.
I came here to file a request to un-protect property Drupal\Core\Entity\Query\Sql\Query::sqlQuery because otherwise I can see no way to add the join.
But I see I would also have to rewrite the basic conditions of the query too, because I need to filter on user.name OR realname.realname
I didn't understand anything at the top of this thread, and I don't know if this fix will help me.
Any thoughts?

@matslats, \Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection::buildEntityQuery() adds the 'entity_reference' tag to its entity query, along with the entire selection handler class as metadata, so you can just implement hook_query_TAG_alter() to alter the query. Note that in that hook, the query you receive as a parameter is a db query object, not the entity query from buildEntityQuery().

See system_query_entity_reference_alter() for an example.

Thanks @amateescu hadn't thought of that.
I'm not seeking support in the wrong place but I want it to be noted here that that solution won't help in the realnames module because the entityQuery was initiatied with AND and by the time it reaches the alter phase, its conditions look like:

Array
(
    [#conjunction] => AND
    [0] => Array
        (
            [field] => users_field_data.name
            [value] => %ad%
            [operator] => LIKE
        )

)

and I need to OR that with realnames.realname LIKE %ad%
i.e. gimme uids where user.name like %fragment% OR realnames.realname like %fragment%
To work around this I would have to build my own 'EntityReferenceSelection' plugin which didn't extend
\Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection::buildEntityQuery
and which therefore built a query without
$query = $this->entityManager->getStorage($target_type)->getQuery();
Looks like a lot of code.
So will this patch offer a way to join an entityQuery to another table and also rework the conditions?

We only need to address #45 and then we are done.

Here is the new and better version of the patch. Someone suggested to me to use entity:node format just like context system and then we don't have to fear about BC break and horrific regex. The patch only need -ve test but other then that every thing is working correctly.
Thanks someone for all the help.
It is BC safe solution to moving it back to 8.1.x
DER can now use ->condition("field_der.target_id.entity:entity_test.name", 'Foobar') to mention the target_type.

Here is how DER can leverage this.

Updated issue summary.

The patch only need -ve test but other then that every thing is working correctly.

We don't need -ve tests here because if the wrong entity type is used then the result set would be invalid and if entity type is not defined then entity manager would throw a PluginNotFoundException The "foobar" entity type does not exist..

Here some improvement after the discussion with @Berdir.

1. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -253,10 +255,20 @@ public function addField($field, $type, $langcode) {
   +          if (!$entity_type_id &&  $target_definition instanceof EntityDataDefinitionInterface) {
   

   nitpick: double space

2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityQueryRelationshipTest.php
   @@ -154,6 +154,45 @@ public function testQuery() {
   +    // This returns the 0th entity as that's only one pointing to the 0th
   +    // account.
   

   .. that is *the* only one..

   I also don't think that 0th account isn't really a thing. We start counting with 0, but that's still the 1st account?

This only adds tests for one part of this issue, the new "feature--task-bug" of being able to specificy the entity type, which is useful for DER but pretty meaningless for core. The actual bugfix here with the hardcoded entity property isn't tested but I'm not sure how to test it as we have no such entity type in core. Nor does DER use that, you would need an field type with a reference property not named entity.

Thanks for the review and a nice find.
How about something like testInvalidSpecifier? It will test this change

+++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
@@ -253,10 +255,20 @@ public function addField($field, $type, $langcode) {
-        if (isset($propertyDefinitions[$relationship_specifier]) && $field_storage->getPropertyDefinition('entity')->getDataType() == 'entity_reference' ) {
...
+        if (isset($propertyDefinitions[$relationship_specifier]) && $propertyDefinitions[$relationship_specifier] instanceof DataReferenceDefinitionInterface) {

.
And it will throw an exception now instead of the fatal error.

+++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityQueryRelationshipTest.php
@@ -154,6 +155,56 @@ public function testQuery() {
+
+  /**
+   *
+   */
+  public function testInvalidSpecifier() {
+    $this->setExpectedException(PluginNotFoundException::class);

missing docs :)

Thanks for the review @Berdir.

Works for me. Thanks.

Added #2818261: Add target_id as the main property just like ERItem for DER.

Ok, lets do this, to unblock entity query support for DER.

I don't think this has any effect on already working queries and for non-working queries, it might change them from a fatal error to an exception or a different exception, which is OK for 8.2.x.

The test failures look like #2806697: Random fail for AlreadyInstalledException and #2157927: Intermittent test fails in LocaleUpdateTest::testUpdateImportSourceRemote(), so requeuing tests on all environments.

I don't think this has any effect on already working queries and for non-working queries, it might change them from a fatal error to an exception or a different exception, which is OK for 8.2.x.

Well, technically, changing from one exception to a different one can disrupt code that handles the original exception. In this case it'd probably be okay. I'm confused though; isn't this adding support for new functionality? Which would mean 8.3.x and a CR needed.

+++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
@@ -253,10 +255,20 @@ public function addField($field, $type, $langcode) {
+        // Relationship specifier can also contain the entity type id i.e.
+        // entity:node, entity:user or entity:taxonomy.

Does EFQ document support for this already elsewhere?

(nit: ID should be uppercase, and there should be a comma before "i.e.").

Thank you @xjm for the review.

I'm confused though; isn't this adding support for new functionality?

Yeah, it resolves the bug, changes the fatal into the exception, and adds new functionality.

Which would mean 8.3.x and a CR needed.

Maybe we should commit the bug fix version of this patch to 8.2.x. I'm moving the issue to 8.3.x and I uploaded do not test patch for 8.2.x.

Does EFQ document support for this already elsewhere?

No, not yet I'll add the docs about this functionality once this is committed.

Thanks for pointing out a nit. New patch fixes it.

Changing from one exception to another could be problematic, but we are changing from a fatal error to an exception. For queries that are *definitely* invalid and do not and will not ever work. You can only "handle" fatal errors in PHP7 where they are throwable/catchable error objects.

The new test shows an example of such a query, it is doing a condition with a typed data reference property linke the language on the LanguageItem field. That's a computed reference to the language object, not something that is persisted and queryable (there are configurable languages but this data type is not an entity and we don't support querying config entity properties that are referenced either).

IMHO, that change is OK, also as a patch release. But if you prefer to only have it in 8.3, fine with, it's @jibran/Dynamic Entity Reference that will then be unable to support entity queries until 8.3.0 is out.

To summarize and explain the "change". To be able to support the use case from DER, we need to refactor the code to support a way of specifying the target entity type (some_field.entity:node.type instead of just some_field.entity.type).

We are adding better validation for the detected/specified target entity type, and this is now also throwing an exception on entity queries that before didn't.. but then died with a fatal error.

And yes, it can be argued if this is a bugfix or a task or even new feature to support that kind of syntax. And it's probably a bit of everything. DER right now does not support entity queries and that is a bug. To be able to fix that, we need to ad this new feature.

Sent this for a retest because all but one environments failed like this: https://www.drupal.org/pift-ci-job/523337

Doesn't seem related to the patch at all, but I haven't seen that elsewhere.

Switching from a fatal to an exception seems fine for a patch release to me as well, didn't fully review the patch though otherwise yet.

Thanks all for clarifying. That makes sense to me. If it's definitely a fatal in HEAD and not a different exception, I'm also fine with that change for a patch release as well and the only borderline bit for backporting is whether this is an "addition" or otherwise risky.

Does EFQ document support for this already elsewhere?
No, not yet I'll add the docs about this functionality once this is committed.

We should add the docs in the commit; it's the documentation gate. :) Also still need that CR pretty please since it's a bugish taskish featurish thing. A CR will also make me more comfortable with considering this for 8.2.x.

Oh regarding https://www.drupal.org/pift-ci-job/523337. That test ran during the hour where the timezone that hosts d.o was in the middle of the DST time change and the fail was related to datetime functionality. Hmmm. Could be a bug in HEAD for that special case.

Posted #2825845: DST-related test failures in FilterDateTimeTest for the test fail.

Thanks! @Berdir for convincing the core committers. Thank you @catch and @xjm for the reviews.

Created Entity query allows to specify entity type ID for reference fields and added some docs to the patch. Moving back to 8.2.x as per #76 and #77.

We should add the docs in the commit; it's the documentation gate. :)

I thought you meant docs on d.o i.e. https://www.drupal.org/docs/7/creating-custom-modules/howtos/how-to-use-.... :) Of course I don't want to jump the gate ;-)

PS: Please, add @Berdir to commit credit for all the discussions and help at DrupalCon Dublin. Can we also add the guy with 17 comments in this issue to commit credit? He came up with this idea and helped me a lot with this patch.

+++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
@@ -36,12 +36,16 @@ public function getEntityTypeId();
    *   followed by a column name. The column can be "entity" for reference
-   *   fields and that can be followed similarly by a field name and so on. Some
-   *   examples:
+   *   fields and that can be followed similarly by a field name and so on also
+   *   optionally entity type ID separated by ':' can be mentioned with the
+   *   "entity". Some examples:

I'd keep the . to end the sentence and start a new one, something like this:

Additionally, the target entity type can be specified by appending ":target_entity_type_id" to "entity".

Also, the second thing that we are fixing is that it does not have to be called entity. So the docs are not 100% correct on that.

We could start with something like "The column can be the reference property (usually "entity") for reference fields..

Thanks for the suggestions @Berdir. Fixed the #81.

Looks good to me. lets see if that works for @xjm too :)

Testbot failure, resetting status due to https://www.drupal.org/node/2828045

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!