Problem/Motivation

This was split from #2302617: Define a standard mechaism for backend-aware service overrides but didn't happen. Now #2443679: PostgreSQL: Fix taxonomy\Tests\TermTest needs this. Note the change record for the parent issue claims mysql is set as the default backend in services.yml but that didn't actually happen.

Proposed resolution

Just default to the database driver do not bother with writing services.yml. We do not want to write services.yml because a) we would lose comments, we do not have a comments-keeping YAML dumper like we have drupal_rewrite_settings b) the upgrade path is very complicated. I allow for an easy opt out if it's not desired. Noone will need that.

Remaining tasks

User interface changes

API changes

You can add things like pgsql.entity.query.sql to core.services.yml. This is an addition in preparation of a bug fix.

Data model changes

Extremely unlikely any existing sites would break: you'd need a mysql.servicename and a servicename AND no set backend in service.yml. I suspect anyone doing drivername.servicename is already setting a default_backend in service.yml per the change record but there can't be many.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task, because it is the logic consequence of the backend functionality.
Issue priority Normal, because it is a small change, but could enable improvements to the runtime performance on the longrun
Disruption Really unlikely to have any disruption, see data model change in the issue summary.
CommentFileSizeAuthor
#7 interdiff.txt593 byteschx
#7 2531408_6.patch5.95 KBchx
#4 interdiff.txt4.63 KBchx
#4 2531408_4.patch5.91 KBchx
#3 2531408_3.patch5.89 KBchx
default_backend.patch1.8 KBchx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Tag1 Consulting commented

You can add things like pgsql.entity.query.sql to core.services.yml. This is an addition in preparation of a bug fix.

Well, the infrastructure was possible already, so yeah +1

Do you mind ensuring that with a test?

Status: Needs review » Needs work

The last submitted patch, default_backend.patch, failed testing.

chx’s picture

chx CreditAttribution: chx commented
Status: Needs work » Needs review
FileSize
2531408_3.patch5.89 KB

With a test. Oh and the Symfony Container is broken (edit: reported https://github.com/symfony/symfony/issues/15261 ), it misses __clone. Oh and BackendCompilerPassTest was broken too due to this: the first data set resulted in ServiceClassMysql but it was not caught because ServiceClassMysql is an instance of ServiceClass just fine. Refactored and fixed.

chx’s picture

chx CreditAttribution: chx commented
FileSize
2531408_4.patch5.91 KB
interdiff.txt4.63 KB

More test cleanup.

The last submitted patch, 3: 2531408_3.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 4: 2531408_4.patch, failed testing.

chx’s picture

chx CreditAttribution: chx commented
Status: Needs work » Needs review
FileSize
2531408_6.patch5.95 KB
interdiff.txt593 bytes

Cute test failure. This is the semi-official way to delete the service although it is not documented but then again, Drupal 8 is not alone in completely useless doxygen -- Symfony 2 is guilty too, in this case * @param object $service The service instance so all we can do is relying on the code:

        if (null === $service) {
            if (self::SCOPE_CONTAINER !== $scope) {
                unset($this->scopedServices[$scope][$id]);
            }

            unset($this->services[$id]);
        }
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Tag1 Consulting commented 
+++ b/core/tests/Drupal/Tests/Core/DependencyInjection/Compiler/BackendCompilerPassTest.php
@@ -96,14 +96,14 @@
+   * @internal param $prefix

Why does that need to be @internal? 

  1. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/BackendCompilerPass.php
@@ -42,11 +42,25 @@ class BackendCompilerPass implements CompilerPassInterface {
+    if ($container->hasParameter('default_backend')) {

    It would be great to at least explain in the IS why we don't try to write the services.yml file, as it would be more predictable as you have one file to look it up. Additional changing behaviour of existing websites out there might not be the best idea 

  2. +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/Compiler/BackendCompilerPassTest.php
@@ -48,7 +49,7 @@ protected function setUp() {
-    $this->assertInstanceOf($expected_class, $container->get('service'));
+    $this->assertEquals($expected_class, get_class($container->get('service')));

    why? This is certainly worse code, if you ask me and scope creap

chx’s picture

chx CreditAttribution: chx commented
Issue summary: View changes

> Why does that need to be @internal?

Already removed in #7 I think?

> Additional changing behaviour of existing websites out there might not be the best idea

AFAIK the only driver out there overriding services is MongoDB and even that is incomplete. We can post it in beta notes this behavior kicking in. We still can even if barely. Once 8.0 ships it's going to be much harder to do this.

> why? This is certainly worse code

The current code is not correct. The first dataset creates a ServiceClassMysql and not a ServiceClassDefault because the parameter on the second dataset contaminates the first container as the parameter bag is not cloned. The current test only passes because ServiceClassMysql extends the ServiceClassDefault and the instanceof check simply doesn't catch this problem. Try this: remove the "extends ServiceClassDefault" and you'll get a test fail.

chx’s picture

chx CreditAttribution: chx commented
Issue summary: View changes
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Tag1 Consulting commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Thank you for commenting on my feedback!

What about trying to figure out how to write to the file properly in the installer as a follow up?

Extremely unlikely any existing sites would break: you'd need a mysql.servicename and a servicename AND no set backend in service.yml. I suspect anyone doing drivername.servicename is already setting a default_backend in service.yml per the change record but there can't be many.

I think this is a fair assumption.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

I've reviewed this patch several times and I agree it completes the work on #2302617: Define a standard mechaism for backend-aware service overrides. Also it does allow us to fix postgres and sqlite issues in a better way. Committed 0cecb18 and pushed to 8.0.x. Thanks!

  • alexpott committed adde7a7 on 8.0.x 
    Issue #2531408 by chx, dawehner: Default backend database driver
neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented

quick follow up https://www.drupal.org/node/2539310

Status: Fixed » Closed (fixed)

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