Pass field storage to Tables::addNextBaseField()

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

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

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

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.

Thanks @amateescu!

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?

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?

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.

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.

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

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';
       }

Update the change record as well with the new approach. We might need another 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.

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.

Thank you once again @xjm for giving it a thorough review. I added the docs, created the follow-up #2909425: Add the $field_storage parameter to Tables::addNextBaseTable and update the change notice. Have you reviewed the change notice as well?

Improved the wording a little.

+++ 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.

Sure here we go.

Ok, let's do this :)

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.