Make use of proposed entity table relationship refactor

I'm sure you can do better ;)

I think this could use the decorate feature instead to avoid having to replace the class, it seems you don't need to change it completely, just have some code that runs before.

See big_pipe.services.yml for an example.

#2808333: Factor out the join from Tables.php is in. We need some tests for this.

Replacing the class is by far the easiest as we need the new classes to run instead of the old ones so that their namespace is picked up.

What if someone else wants to override the class?

Decorate by and large doesn't work with Drupal 8 because it doesn't move tags

I'm not following which tags aren't moved. Are service tags not working on decorating services (that seems like it'd be a symfony bug)?

I'm happy to write tests, but need further explanation of what this issue hopes to achieve in order to properly test :) Tagging for IS update.

If decorators are completely out, then replacing the class makes sense, otherwise, if we can work around the limitations of decorators @chx has outlined in #13, that seems preferable.

This is actually postpone on #2424791: Entity query hardcodes entity_reference and entity specifier.

Test only patch for this is also in #2424791-53: Entity query hardcodes entity_reference and entity specifier. We can totally add column type specific test for DER field in EFQ to test the fix after #2808333: Factor out the join from Tables.php.

#2424791: Entity query hardcodes entity_reference and entity specifier is in we can finish this now.

Re-uploading #2424791-53: Entity query hardcodes entity_reference and entity specifier

This combines #4 and #19. Since the test passes without the actual fix, we need to add a test that verifies the correct column was joined on. I'm not sure how to get at that information...

+++ b/tests/src/Kernel/EntityQueryRelationshipTest.php
@@ -133,8 +133,8 @@ class EntityQueryRelationshipTest extends EntityKernelTestBase {
-      ->execute();
+      ->condition("field_test.1.entity:entity_test_rev.name", 'Barfoo')->execute();

Oops, didn't mean to bump that up a line.

Those fails appear to be due to #2822387: Undefined offset 1 in BrowserTestBase::getMethodCaller(), but am not sure why they weren't present before...

+++ b/tests/src/Kernel/EntityQueryRelationshipTest.php
@@ -0,0 +1,141 @@
+    // @todo Verify correct column was used for join.

DER is not using #2827219: Add helper method to retrieve complete column name anywhere right now. I think we can use it here.

Since the $sql_column already comes through as foo_target_id, the new helper method getTargetIdColumnName won't work properly, but I did change this to use the entityHasIntegerId helper method that was also added.

For the tests, this is a bit ugly since it relies on reflection methods, but it does indeed verify that the proper column is joined on. I've attached a test only patch to illustrate this fix.

Anything left to be done here? Perhaps we can add a follow-up for the decorator approach, in the event that this approach here causes issues with other modules needing to also override the core class?

So as with @jibran, what worries me here is wholesale swapping core services.

I had a look into the API and at present the storage handler and entity query service are intertwined by way of the \Drupal\Core\Entity\EntityStorageBase::getQueryServiceName method.

At present each entity storage implementation returns a hard-coded service name.

I felt that there should be the possibility for this to be nominated by each entity type instead, so you could for example alter the Flag entity to use the DER query service, without hijacking every other content entity.

That way the storage handler implementations would instead looks like this:

/**
   * {@inheritdoc}
   */
  protected function getQueryServiceName() {
-     return 'entity.query.sql';
+    return $this->getEntityType()->getQueryServiceName();
  }

I discussed this with @tim.plunkett - asking if he saw any merit in letting the entity type keep track of its entity query service. This would require a new method on the entity-type. He was in general support of this.

If others here feel that is a better approach and makes our ability to nominate the DER query service where needed instead of with a sledgehammer approach, please comment as such here and I'll open a core issue.

Thanks

re #28

I think we can do both here (as larowlan and I discussed yesterday on IRC). Since switching from the sledgehammer approach the current patch uses, to a more nuanced approach (either via a core implementation, and/or using a decorator pattern), will not entail public API changes nor data model changes, that switch can happen in follow-up issue after alpha is tagged, and potentially after beta or even full release...

What about configurable fields though?

Those could exist on any entity type, not just one that happens to have a DER base field? If we'd have a more specific setting, it might need to depend on the field type even?

@Berdir I'm not sure which approach you're referring to--are you saying the swappable per-entity-type query service wouldn't work with configurable fields, or that none of these approaches work with them?

Yes, I'm saying per-entity-type doesn't work because the entity type doesn't know if he has configurable DER fields.

Right, so DER would need to implement the alter on behalf of all entity types and use field definitions to detect a DER field and swap the query service.

I'll create the core issue next week.

So back to reviewing the current patch - anyone able to give it another once-over?

Let's go with #25 for now and during alpha release we can change this in followup.

We need followups here.

I added #2835542: Utilize service decorator instead of taking over entire EFQ service.