Utilize service decorator instead of taking over entire EFQ service

After #2808337: Make use of proposed entity table relationship refactor there is this fail in HEAD https://travis-ci.org/jibran/dynamic_entity_reference/jobs/186344732.

There was 1 failure:
1) Drupal\Tests\dynamic_entity_reference\Kernel\EntityQueryRelationshipTest::testEntityQuery
Query joined on target_id_int column.
Failed asserting that false is true.
/home/travis/build/jibran/drupal/core/tests/Drupal/KernelTests/AssertLegacyTrait.php:31
/home/travis/build/jibran/dynamic_entity_reference/tests/src/Kernel/EntityQueryRelationshipTest.php:215
/home/travis/build/jibran/dynamic_entity_reference/tests/src/Kernel/EntityQueryRelationshipTest.php:122

NVM #2. Changing the core version fixed the tests https://travis-ci.org/jibran/dynamic_entity_reference/builds/188110987

+++ b/dynamic_entity_reference.services.yml
@@ -21,3 +21,11 @@ services:
+  dynamic_entity_reference.entity.query.sql:
+    decorates: 'entity.query.sql'
+    class: Drupal\dynamic_entity_reference\Query\QueryFactory
+    arguments: ['@database']
+  dynamic_entity_reference.entity.query.sql.pgsql:
+    decorates: 'pgsql.entity.query.sql'
+    class: Drupal\dynamic_entity_reference\Query\PgsqlQueryFactory
+    arguments: ['@database']

Should these inject the "inner" service from the decoration? Then instead of extending the concrete version, you can call $this->decorated->function();

@jhedstrom what are the benefits of this patch? How can it help other contrib modules/drupal projects who are overriding the EFQ service as well?

I don't know what cores namespace detection does, so I might be missing some context. Jibran asked for a review of this, so this is my current grokking of the issue.

You would need the original service as a property of the new service and you would need to implement the interfaces directly, instead of extending a base class to achieve #7. This part of the issue summary is incorrect:

to achieve the same end-goal via service decorators instead of taking over the entire class

The before and after implementations both take over the whole class in pretty much the same way. This is the only functional difference between the two implementations:

if ($service_definition->getClass() == $data['old']) {

Previously if another module had taken over the service, replacing it would be skipped in DER. I don't believe using "decorates" has that same semantic, because it's for implementations that build on each other by design. So I think the summary of the current changes is:

• Nice code improvement.
• Will always replace the service, regardless of other implementations.

If using the Tables class is very important, then I suppose there isn't much choice than to ignore other versions of the class in the container. At that point it's either "they are broken or we are".

This new implementation does open the door to use an actual decorator however, which would be the best of both worlds, provided the other implementations of the factory were actual compatible with the kind of alterations DER is making. Additionally it assumes the other implementation isn't there only for the Tables class either.

So I think the old way is fine, the new way is fine too. Even better would be finding an alternative implementation that doesn't require the Tables class to change.

All of this might be a consequence of some interesting design choices with regards to magically loading the table class from the same namespace, but I have no alternative suggestions.

Previously if another module had taken over the service, replacing it would be skipped in DER.

The only side effect would be that you'll end up joining int column to sting column which is not a total disaster so I think the current implementation is fine. If we'll see some issue down the road then we can revisit this and reconsider our approach.

Also, by looking at the current patch it is not a BC break at all so I don't think this is a stable blocker anymore. The only issue left is #2915512: Entity query doesn't allow relationship specifier for base entity fields but DER can't fix it we have to fix it in core.

+++ b/dynamic_entity_reference.services.yml
@@ -21,3 +21,11 @@ services:
+  dynamic_entity_reference.entity.query.sql.pgsql:

Note that this won't work as expected, we still need to use pgsql as a prefix for the service name (instead of a suffix like in the current patch) and add the backend_overridable tag to the main service (dynamic_entity_reference.entity.query.sql).

See the patch from #2940446: Decorate core's entity query services instead of replacing them and fix PgSQL support for a working implementation :)

Now that workspace is in the core we should fix this. This how workspace is overriding the EFQ service with decorator.

  workspace.entity.query.sql:
    decorates: entity.query.sql
    class: Drupal\workspace\EntityQuery\QueryFactory
    arguments: ['@database', '@workspace.manager']
    public: false
    decoration_priority: 50
    tags:
      - { name: backend_overridable }
  pgsql.workspace.entity.query.sql:
    decorates: pgsql.entity.query.sql
    class: Drupal\workspace\EntityQuery\PgsqlQueryFactory
    arguments: ['@database', '@workspace.manager']
    public: false
    decoration_priority: 50

We can increase DER decoration_priority to make it work with workspace.
We also have to fix #10. Thank you @amateescu for pointing it out.

I added #3090365: Add tests to make sure DER can work together with workspaces.
I added following remianing step over there

In \Drupal\dynamic_entity_reference\DynamicEntityReferenceServiceProvider, check if workspaces module is installed or not and then swap workspaces EFQ service instead of one provided by core.

I think if we were to able to do this then we can mark is as a won't fix.