Suggested commit message:

Issue #2302617 by dawehner | Crell: Define a standard mechaism for backend-aware service overrides.

Problem/Motivation

We've poured thousands of hours of work into piecing apart Drupal 8 so that anything that touches the database is swappable for an alternate backend. (MongoDB, Redis, Memcache, APC, Cassandra, you name it.) That's wonderful. Go Drupal!

However, we have no standard way of actually leveraging that capability. It's all ad-hoc container alters, which is problematic when you want to override only some core services with an alternate backend.

At the same time, as discussed in #2290771: [policy, no patch] Make default SQL drivers MySQL specific making "lowest common denominator" SQL for all databases is, at this point, a losing battle. Many places we're still using SQL could benefit greatly from backend-specific optimizations, and there's no reason that the exact same mechanism that can swap an SQL service for a MongoDB service can't also swap a MySQL service for a PostgreSQL service.

So let's define a standard way to override a "driver" service with a new driver backend that can handle MySQL/PostgreSQL/MongoDB/Cassandra/etc. all as equal citizens.

Note: This is JUST about the common "datastore-aware" services, which are the most likely to be swapped out. For everything else there's the existing ServiceModifierInterface that will continue to work exactly the same.

Proposed resolution

chx and I discussed this problem at length, and have come up with the following solution. I'll use the lock service as the example below for clarity.

1) Core defines a service, lock. It points to an SQL-generic implementation that aims for portability, NOT optimization.

2) That service is tagged "backend_overridable". (Tag name bikesheddable, but don't go overboard.)

3) The site-wide services.yml file for site-specific services has a container parameter defined in it, "default_backend". The installer will create this and set it to the DB driver name. (mysql, pgsql, sqlite, etc.)

4) A BackendCompilerPass object will do the following:

* The lock service is tagged backend_overridable. OK!
* Is lock an alias? If yes, stop.
* If not, look for a service named $default_backend.lock (taking $default_backend from the container parameter).
* Is there one? Change lock into an alias to $default_backend.lock.

(For all services with that tag.)

5) Now, the mongodb module can define a mongodb.lock service. It doesn't need to do anything else but define it.

6) If a site admin wants to replace just the lock service with the MongoDB version, he can do so by overriding the lock service in services.yml and aliasing it to mongodb.lock. kthxbye.

7) If a site admin wants to use MongoDB for everything possible, just change the default_backend parameter to "mongodb". BackendCompilerPass will then use the mongodb version for everything it can and leave the rest at the default SQL version.

8) Core can then enhance the MySQL, PostgreSQL, and SQLite drivers (assuming the latter two remain in core; if not contrib can do the same) to support driver-specific functionality. Then, we provide a mysql.lock service that uses those capabilities. Poof, BackendCompilerPass will now use the MySQL version of the lock service on a MySQL site (the 99% case). These alternates can be done piecemeal service by service without changing APIs.

Note that there's nothing core-specific about this approach. While very few contrib modules should have cause to write directly to a backend from a service, those that do can use the exact same mechanism above, opt-in, by just adding the tag to the service in question.

Remaining tasks

1) Agree on the above.
2) Implement the above.
3) ???
4) Commit!!!1!

User interface changes

None.

API changes

Probably none. Only additional ways to use the container.

That also means this is not beta-blocking or beta-blocked. And we can add new override services to core at any time in the 8.x cycle without breaking anything.

CommentFileSizeAuthor
#24 interdiff.txt3.21 KBdawehner
#24 backend_services-2302617-24.patch6.73 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,231 pass(es). View
#19 interdiff.txt1.73 KBdawehner
#19 backend_services-2302617-19.patch9.66 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#9 2302617_9.patch7.93 KBchx
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,323 pass(es). View
#7 2302617-7.patch8.15 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,093 pass(es). View
#7 interdiff.txt5.62 KBdawehner
#5 2302617.patch3.22 KBdawehner
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Crell’s picture

Issue summary: View changes
chx’s picture

I am in for 1) and 2).

chx’s picture

Issue tags: +happy chx
dawehner’s picture

I really like this proposal. It makes the life of contrib authors easy. It makes the life of site admins easy and it makes the life of core developers easy. Noone has actively to think about the other groups, all just works automatically

dawehner’s picture

FileSize
3.22 KB

One thing we should discuss is whether we want to prefix by storage or the other way round. The proposed way makes more sense for me, because you most often have multiple service per backend (sqlite...) but less backends per service.

chx’s picture

Thanks for starting the patch, a couple notes:

$default_backend = $container->hasParameter('default_backend') ? $container->getParameter('default_backend') : 'mysql';

Crell was uncomfortable with hardwiring 'mysql' that's why we arrived to writing a services.yml during install (see 3) in proposal).

if ($container->hasDefinition("$default_backend.$id")) {

I think we want to add || $container->hasAlias("$default_backend.$id") because I think the getter is recursive.

Finally: yes, definitely prefix by backend, I fully agree there.

dawehner’s picture

Status: Active » Needs review
FileSize
5.62 KB
8.15 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,093 pass(es). View

Sadly I ran into #2302799: InstallerTestBase tests can not be run locally so in case you want to run the test on your local install.

Beside from this it is working fine for me.

dawehner’s picture

Assigned: chx » Unassigned

Well, we want that somehow reviews it

chx’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
7.93 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,323 pass(es). View

This is all good, removed a whitespace, please do not credit me!

Crell’s picture

Status: Reviewed & tested by the community » Needs review

Well, that escalated quickly. :-)

We need to test that defining an alias for a service in the services.yml file will override the non-alias definition provided by the module. That's a big part of the DX here so we need to confirm that things happen in an order that will allow that. (Eg, if I want to override JUST lock with a redis backend, I shouldn't have to do more than define lock as an alias to redis.lock in my site's services.yml. I don't think the test here covers that yet.)

Are there any core services that we can make an override for right now to demonstrate the change? (I'm OK with not, but I know committers will ask so I am asking before they get here.)

+++ b/core/tests/Drupal/Tests/Core/DependencyInjection/Compiler/BackendCompilerPassTest.php
@@ -0,0 +1,93 @@
+class ServiceClassMariaDb extends ServiceClassMysql {
+}

Damned tootin' this becomes possible. :-)

chx’s picture

> if I want to override JUST lock with a redis backend, I shouldn't have to do more than define lock as an alias to redis.lock in my site's services.yml. I don't think the test here covers that yet.

I think it does, check the last one:

+ // Configure a manual alias for the service, so ensure that it is not
+ // overridden by the default backend.

$container->setAlias('service', new Alias('mariadb.service'));

that's the exact case you are talking of isn't it

Crell’s picture

That confirms that the compiler pass does what it's supposed to do, right? I'm saying even if the compiler pass does nothing, if core.services.yml defines lock, and sites/default/services.yml defines lock, who wins? It should be the latter. That probably needs an integration test (Simpletest) to verify.

Brandonian’s picture

This sounds great. Over in contrib world, we built something similar for Geofield's backend storage system awhile ago so that we could store geospatial data natively depending on the database being used (i.e., use PostGIS field types if you have that enabled).

Example of our service in D8: http://cgit.drupalcode.org/geofield/tree/src/GeofieldBackend/GeofieldBac...

sun’s picture

if core.services.yml defines lock, and sites/default/services.yml defines lock, who wins? It should be the latter. That probably needs an integration test

That's the case, and already covered by DrupalKernelSiteTest.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

So it is! Excellent. Problem solved then. :-)

Unless we're going to add a MySQL override for something in this issue, I think we're back to RTBC.

dawehner’s picture

You could use change notices not to update stuff, but to learn about new APIs etc. so I really think it would be just fair to
add one for this change in order to document it.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
/**
 * Provides a form to configure and rewrite settings.php.
 */
class SiteSettingsForm extends FormBase {

This form now also writes the services.yml

    $conf_path = './' . conf_path(FALSE);
    $services_file = $conf_path . '/services.yml';
    // @TODO Should we put some logic in here to ensure that the file is read
    //   and writable?
    file_put_contents($services_file, Yaml::encode(array('parameters' => array('default_backend' => $database['driver']))));

This does indeed seem fragile. Shouldn't be checking if we can write this file in hook_requirements install phase? Also should this file be web server writeable? Being that this can change the container I think not. Also this will result in completely over writing the file - is that what we desire? Do we even need to do this? The BackendCompilerPass has code to handle a situation where the parameter is not set.

chx’s picture

If we don't do this then the mysql specific drivers (which will happen after this) won't be picked up.

dawehner’s picture

Status: Needs work » Needs review
FileSize
9.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
1.73 KB

After talking with alex, here is a check for read/writability of this file.

claudiu.cristea’s picture

+++ b/core/includes/install.core.inc
@@ -2010,6 +2010,28 @@ function install_check_requirements($install_state) {
+    if (!$readable) {
+      $requirements['services file readable'] = array(
+        'title' => t('Services file'),
+        'value' => t('The services.yml file is not readable.'),
+        'severity' => REQUIREMENT_ERROR,
+        'description' => t('@drupal requires read permissions to %file at all times. If you are unsure how to grant file permissions, consult the <a href="@handbook_url">online handbook</a>.', array('@drupal' => drupal_install_profile_distribution_name(), '%file' => $yml_file, '@handbook_url' => 'http://drupal.org/server-permissions')),
+      );
+    }
+    if (!$writable) {
+      $requirements['service file writable'] = array(
+        'title' => t('Services file'),
+        'value' => t('The service.yml file is not writable.'),
+        'severity' => REQUIREMENT_ERROR,
+        'description' => t('The @drupal installer requires write permissions to %file during the installation process. If you are unsure how to grant file permissions, consult the <a href="@handbook_url">online handbook</a>.', array('@drupal' => drupal_install_profile_distribution_name(), '%file' => $yml_file, '@handbook_url' => 'http://drupal.org/server-permissions')),
+      );
+    }

It would be nice and more compact to have a single row with title "Services file" (and a single key services key) in case both conditions are met (!$readable && !$writable).

Status: Needs review » Needs work

The last submitted patch, 19: backend_services-2302617-19.patch, failed testing.

dawehner’s picture

It would be nice and more compact to have a single row with title "Services file" (and a single key services key) in case both conditions are met (!$readable && !$writable).

Well, my goal was to have a similar implementation than database itself.

Sadly testbot seems to have some configuration/permission issues so we maybe have to patch it first.

claudiu.cristea’s picture

@dawehner, It's not the testbot. Simply services.yml doesn't exist at that stage and the read/write test fail both. I installed manually and got the same errors. The problem is that the check occur before the file generation.

dawehner’s picture

Status: Needs work » Needs review
FileSize
6.73 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,231 pass(es). View
3.21 KB

After a chat with catch and chx we agreed that we should skip it for now
and discuss in another issue when and what to write by default.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Sure.

Crell’s picture

To clarify then, for now even having the mysql overrides enabled is opt-in, and we may default to that later if we can make the code for it work? (I'm totally fine if that's the case; I just want to confirm.)

chx’s picture

Yes. Exactly. There was even talk of perhaps using settings for this instead of services. Basically, let's have this in; let's write something that uses it (on it!) then we can switch it on.

Dries’s picture

Seems like a standard mechanism for service overrides could (or should?) be part of Symfony instead? Standardization across Symfony users seems like a good thing. Thoughts on that?

Crell’s picture

The only code for that is the compiler pass itself, which is relatively trivial. We could certainly offer a PR to Symfony's Framework Bundle (I think that's where it would make sense to live). Whether or not they'd want it is another matter, given that Fullstack has different usage patterns from Drupal.

I would recommend we proceed here, then submit a PR upstream. (The code in question is probably too small to have a licensing issue; if it does, I think there's only 2-3 meaningful authors -- chx, dawehner, and maybe me -- who would have to +1 just this piece.) I doubt there's be any dramatic changes except maybe the tag name, and that's totally easy for us to switch to or even support both if we want to be anal about it.

In short: Good idea, but not a commit blocker.

chx’s picture

I am not an author of this code.

effulgentsia’s picture

In the next day or two, I'll compile a list of services that could benefit from this (i.e., a subset of ones that receive an @database injection). I'm curious how many we really have, and which of them should move to some higher-level storage backend (i.e., should core's default "lock" service be based on an @keyvalue.expire injection rather than @database)? I have a slight concern that this issue will encourage services to be based on @database, whereas we should be moving things to our higher-level storage concepts, like keyvalue, entity, config, etc. I also wonder if we should add a keyvalue-like API for a non-relational table with an arbitrary number of columns, and if that would help us further reduce the number of services that directly depend on @database.

But I have no objection to this being committed in the meantime if there's a committer who wants to do so. Only that in the long run, my hope is that the automation that this affords us turns out to not be that valuable, since it will only automate a small number of services.

Crell’s picture

I believe that list is already being built: #2306071: Tag backend services.. Although yes, in some cases they probably could move to K/V instead. That can be a case by case decision.

dawehner’s picture

Opened an issue on https://github.com/symfony/symfony/issues/11460 but as @crell said, we would probably not be possible to directly use that code
if it is upstream, just share the idea.

I have a slight concern that this issue will encourage services to be based on @database, whereas we should be moving things to our higher-level storage concepts, like keyvalue, entity, config, etc.

If developers are not able to use the much nicer to use abstractions like a KV-store its their own fault. Not adding something useful to make it easier to babysit contributors, which I consider as damn smart people, is a bad thing. Glad you are okay with the issue!

catch’s picture

@keyvalue.expire isn't up to the task of doing what lock does, since it doesn't have atomic operations that match Lock::acquire().

We talked about adding a generic key/value storage API which could be used as the dependency for lock/cache/state etc. in #1202336: Add a key/value store API (possibly other issues too) and ended up going for specific implementations for specific services instead. Once you have a generic backend that tries to support the various operations, it becomes harder to have operations that are both optimized for the use case, and correct. I'm sure there are other places that have database as a direct dependency that shouldn't, but in general I'd keep the very low level APIs like key/value and lock separate for now rather than trying to introduce an additional layer between them and the database.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

We seem to have moved on to debating other (connected) issues. This patch adds a nice way to override backend services. Let's do it.

Committed 36faf4e and pushed to 8.x. Thanks!

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/BackendCompilerPass.php
@@ -0,0 +1,63 @@
diff --git a/core/lib/Drupal/Core/Installer/Form/SiteSettingsForm.php b/core/lib/Drupal/Core/Installer/Form/SiteSettingsForm.php

diff --git a/core/lib/Drupal/Core/Installer/Form/SiteSettingsForm.php b/core/lib/Drupal/Core/Installer/Form/SiteSettingsForm.php
index 23f0d03..241daec 100644

index 23f0d03..241daec 100644
--- a/core/lib/Drupal/Core/Installer/Form/SiteSettingsForm.php

--- a/core/lib/Drupal/Core/Installer/Form/SiteSettingsForm.php
+++ b/core/lib/Drupal/Core/Installer/Form/SiteSettingsForm.php

+++ b/core/lib/Drupal/Core/Installer/Form/SiteSettingsForm.php
+++ b/core/lib/Drupal/Core/Installer/Form/SiteSettingsForm.php
@@ -7,6 +7,7 @@

@@ -7,6 +7,7 @@
 
 namespace Drupal\Core\Installer\Form;
 
+use Drupal\Component\Serialization\Yaml;
 use Drupal\Component\Utility\Crypt;
 use Drupal\Core\Database\Database;
 use Drupal\Core\Form\FormBase;

I did not commit this change :)

  • alexpott committed 36faf4e on 8.x
    Issue #2302617 by dawehner, chx | Crell: Define a standard mechaism for...

Status: Fixed » Closed (fixed)

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

fgm’s picture

Any reason why we don't switch to auto_alias, since we now include Symfony 2.7, which added this mechanism from @dawehner's patch for us ? It would be more familiar to SF2 devs coming to Drupal.