Problem/Motivation

In the Dynamic Entity Reference module, somewhere between Drupal 8.2 and 8.5.x, this issue has appeared: #2908109: Entity queries broken for non-DER entity reference base fields. It is due to #2424791: Entity query hardcodes entity_reference and entity specifier, and DER's overriding of the addNextBaseTable method. DER is unable to check if the field referencing the table and column to be added is a normal entity_reference field (in which case it should do nothing) or a dynamic_entity_reference field, in which case it should change the column to utilize the _int column.

Proposed resolution

If the protected method addNextBaseTable had the field storage object passed to it, DER could work around this issue by checking that the referencing field was a dynamic_entity_reference field, and not a normal entity reference.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#22 pass_field_storage_to-2908605-22.patch1.43 KBjibran
#22 interdiff-735430.txt1.73 KBjibran
#20 pass_field_storage_to-2908605-20.patch1.3 KBjibran
#20 interdiff-4705c0.txt1.2 KBjibran
#19 pass_field_storage_to-2908605-19.patch1.29 KBjibran
#19 interdiff-9effe8.txt1.16 KBjibran
#14 pass_field_storage_to-2908605-14.patch889 bytesjibran
#14 interdiff-b84664.txt1.44 KBjibran
#12 pass_field_storage_to-2908605-12.patch2.12 KBjibran
#12 interdiff-808095.txt1.25 KBjibran
#2 2908605-02.patch2.05 KBjhedstrom
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

jhedstrom created an issue. See original summary.

jhedstrom’s picture

jhedstrom’s picture

Issue summary: View changes
jibran’s picture

Status: Needs review » Reviewed & tested by the community

It is a contrib project blocker, is there a chance that this can be committed to 8.4.x as well?

xjm’s picture

I'd really like an Entity subsystem maintainer review on this one. Thanks!

xjm’s picture

Status: Reviewed & tested by the community » Needs review

NR for them; if they sign off it can go back to RTBC.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs subsystem maintainer review

This looks ok to me. The addNextBaseTable() method was added specifically for DER in #2808333: Factor out the join from Tables.php but it seems that it is not very useful for them without this additional parameter.

xjm’s picture

Thanks @amateescu!

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

Ah, this would need a small CR as a parameter addition to a protected method. It could cause BC breaks for a child implementation that overrode this method and added a different fourth parameter already. The BC break is allowable though since this method is not on TablesInterface. (I don't think we should backport it during RC, though, because there is a very small chance it could break something. Edit: Except that I forgot that this is also a contrib blocker per the IS, duh.)

Still, #2808333: Factor out the join from Tables.php should probably have had a CR, and it doesn't look like it did. Let's add a single CR for both that and this?

xjm’s picture

xjm’s picture

How bad is this issue for DER? Since it is a slightly disruptive change. I wonder if there is a way to fix it that doesn't require an internal BC break?

jibran’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record
FileSize
1.25 KB
2.12 KB

Thank you @amateescu for looking into it and pointing out to the original issue in which the method was added. Thank you @xjm for giving it a thorough review and pointing out the issues.
Added change notice https://www.drupal.org/node/2909405. Accommodate the BC as well.

jibran’s picture

How bad is this issue for DER?

In DER 8.x-2.x, when in EFQ you try to add a condition on the property of base ER field it errors out this means it is broken for every entity with the author field so almost all entities.
For example following query is broken when DER 8.x-2.x is installed:

$result = \Drupal::entityTypeManager()->getStorage('node')->getQuery()
      ->condition('uid.entity.status', 1)
      ->execute();

For more details please see IS of #2908109: Entity queries broken for non-DER entity reference base fields.

jibran’s picture

Ignore the patch in #12 this is patch is with BC.

jibran’s picture

Now DER can do this.

diff --git a/src/Query/Tables.php b/src/Query/Tables.php
index c95d842..5e24294 100644
--- a/src/Query/Tables.php
+++ b/src/Query/Tables.php
@@ -15,10 +15,11 @@ class Tables extends BaseTables {
    * {@inheritdoc}
    */
   protected function addNextBaseTable(EntityType $entity_type, $table, $sql_column) {
+    $field_storage = func_get_arg(3);
     // Parent method is overridden in order to choose the correct column to
     // join on (string or int).
     $entity_type_id_key = $entity_type->getKey('id');
-    if ($entity_type_id_key !== FALSE) {
+    if (($field_storage->getType() === 'dynamic_entity_reference') && ($entity_type_id_key !== FALSE)) {
       if (DynamicEntityReferenceItem::entityHasIntegerId($entity_type->id())) {
         $sql_column .= '_int';
       }
jibran’s picture

Update the change record as well with the new approach. We might need another subsystem maintainer review.

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Needs followup, +Needs subsystem maintainer review

Ah yeah that is more backportable! I can't think of any way that would break anything. The chances of anything else overriding the caller and already passing an undeclared extra argument of a different data type and doing its own func_get_arg() are well and truly miniscule.

I think we should add an inline comment explaining why we're passing an extra argument, with an @todo to a followup issue.

There's two things the followup issue could be. Since this is a protected method not on a corresponding interface, our BC policy allows us to change it in a minor release if we want. The full BC thing to do would be to deprecate the method and add a new one with the new signature. However, that seems unnecessary to me; just wanted to document that I'd thought it through.

Tagging again per #16.

xjm’s picture

Version: 8.5.x-dev » 8.4.x-dev

Moving to 8.4.x; I'm fairly confident we are fine with the new approach during RC. I'd even consider it during a patch release.

amateescu’s picture

+++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
@@ -273,7 +273,13 @@ public function addField($field, $type, $langcode) {
+          // An addition $field_storage parameter is passed to make this
+          // function more usable. To maintain the backwards compatibility, the
+          // additional parameter is not added to the function signature and it
+          // will be added to the function signature in 8.5.x.
+          // @todo Add the $field_storage parameter to addNextBaseTable
+          //   signature in 8.5.x. https://www.drupal.org/node/2909425

This whole paragraph sounds a bit.. non-english :) I would suggest something like this instead:

An additional $field_storage argument is being passed to addNextBaseTable() in order to improve its functionality, for example by allowing extra processing based on the field type of the storage. In order to maintain backwards compatibility in 8.4.x, the new argument has not been added to the signature of that method, and it will be added only in 8.5.x.
@todo Add the $field_storage argument to addNextBaseTable() in 8.5.x: https://www.drupal.org/node/2909425.

jibran’s picture

Sure here we go.

amateescu’s picture

Title: Pass field storage to Tables::addNextBaseField » Pass field storage to Tables::addNextBaseField()
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs subsystem maintainer review

Ok, let's do this :)

  • xjm committed fc4a1d6 on 8.4.x
    Issue #2908605 by jibran, jhedstrom, xjm, amateescu: Pass field storage...

  • xjm committed b625153 on 8.5.x
    Issue #2908605 by jibran, jhedstrom, xjm, amateescu: Pass field storage...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

Great, that works. The change record also looks like it explains this pretty well. Committed to 8.5.x and cherry-picked to 8.4.x. I published the change record and updated its "fixed in version" stuff.

Status: Fixed » Closed (fixed)

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