Follow-up to #1764474: Make Cache interface and backends use the DIC.

Quote from #1764474-63: Make Cache interface and backends use the DIC

- While testing the above configuration, I found an interesting bug. The database is now solely configured through the DIC. If the first request does not go through DatabaseBackend but e.g. db_query() then the Database never receives the necessary connection information and the site explodes. Discussed with Crell and we agreed this can be solved in a separate issue as the Database class should be refactored a bit anyway to make it easier to embed it into the DIC.

Files: 
CommentFileSizeAuthor
#24 database-dic-1811730-24-do-not-test.patch94.07 KBBerdir
#14 database-dic-1811730-14.patch104.62 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 30,586 pass(es), 1,447 fail(s), and 537 exception(s).
[ View ]
#14 database-dic-1811730-14-interdiff.txt29.63 KBBerdir
#9 database-dic-1811730-8.patch84.21 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 32,059 pass(es), 995 fail(s), and 301 exception(s).
[ View ]
#9 database-dic-1811730-8-interdiff.txt2.41 KBBerdir
#5 database-dic-1811730-5.patch83.62 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 29,988 pass(es), 1,132 fail(s), and 365 exception(s).
[ View ]

Comments

Crell’s picture

The fix to that, IMO, is for db_query() et al to not wrap Database::getConnection() but drupal_container()->get('database'); Then the Container is always responsible for initializing the database connection. We can then refactor the Database class to not be quite as ugly as it is now. (A lot of the purpose of it probably goes away once we can rely on the Container.)

Berdir’s picture

Right. I tried to get started with this, but I'm running into some problems:

- I understand we want the connection in the container, and not the database class?
- Given that, I'm not sure how to deal the fact that we support configuring multiple database backends? How would you access those?
- Similar for master/slave, the slave connection is currently registered as a second service. So if you wanted to use the master and the slave connection, you would have to get both injected to you?
- There are a number of methods in Database that I'm not sure about. Some should become unecessary, e.g. renameConnection() but what about the logging stuff? I'm also not sure if we want to continue supporting the setActive stuff to switch to non-default databases, instead of just getting them and running queries on them.

One approach that I'm currently trying is to make Database a class that can be instantiated and putting that in the DIC. Downside is that you need to call something like this: drupal_container()->get('database')->getConnection()->query(). On the other side, it's easy to solve those problems outlined above. And it's also quite easy for a class that gets the database injected to not store the Database instance but do something like $this->connection = $database->getConnection() in the constructor.

Opinions?

Crell’s picture

I don't see a problem with having both a database_manager service (which is an instantiated object version of the Database class) and direct access to the default database via a database and database.slave service. At least as a first pass, that's probably fine. It also has the benefit of not changing anything for existing code that uses an injected DB (its DIC configuration does not change). The call chain for 3rd party connections is not, I think, any worse than it is now, just different funky punctuation. Since that's an edge case and can be easily handled in constructors, I don't have a problem with that.

If we can improve that later in other follow ups, fine, but for now let's just get it working.

I also will not shed a tear if setActive() goes away.

I've toyed with the idea of meta-connection objects that encapsulate both the master and slave query, so that the master/slave switch can happen later. That's not going to happen in D8, though. So yes, if you really want to do both master and slave queries from the same service you request 2 connections; it's then up to the DatabaseManager to (or the Container configuration) to give you the same connection or different connections; if you have 2 references to the same object, one of which you think is "read" and the other "write", no harm done.

Logging is an interesting case, but I think a straight conversion of Database:: to $db_manager = new DatabaseManager(); will let us just move all the statics to properties and call it a day. If not, let's figure that out when we try to do so.

RobLoach’s picture

Berdir’s picture

Status:Active» Needs review
StatusFileSize
new83.62 KB
FAILED: [[SimpleTest]]: [MySQL] 29,988 pass(es), 1,132 fail(s), and 365 exception(s).
[ View ]

Ok, here's a start.

- Database now receives the database_info as a constructor and is a service
- master and slave connections are a service using setFactoryService() (is there anything that doesn't exist in Definition? ;))
- Installer seems to be working
- Running tests seems to be working. Some tricks were necessary but no more Database:: messing, everything happens through the container.
- setActive() isn't going to work anymore, we either need to replace the service or remove the feature.
- Haven't updated the database tests yet, so there will be some failures.

Status:Needs review» Needs work

The last submitted patch, database-dic-1811730-5.patch, failed testing.

Berdir’s picture

Looks like there's a problem with concurrency in there. Most of these tests work fine, but there are random fails when using e.g. concurrency 4.

Will not have time to look into it the next 48h probably, so if someone wants to have a look, be my guest ;)

Crell’s picture

This looks pretty solid overall. Nice work, Berdir. More detailed comments below.

+++ b/core/includes/bootstrap.inc
@@ -2447,6 +2451,21 @@ function drupal_container(Container $new_container = NULL, $rebuild = FALSE) {
+    // This will be overridden by settings.php in drupal_settings_initialize().
+    $container->setParameter('database.info', array());
+    $container->register('database_manager', 'Drupal\Core\Database\Database')
+      ->addArgument('%database.info%');
+
+    $container->register('database', 'Drupal\Core\Database\Connection')
+      ->setFactoryService('database_manager')
+      ->setFactoryMethod('getConnection')
+      ->addArgument('default');
+    $container->register('database.slave', 'Drupal\Core\Database\Connection')
+      ->setFactoryService('database_manager')
+      ->setFactoryMethod('getConnection')
+      ->addArgument('slave');
+

The way the spacing is setup here, it suggests that database_manager will be overridden in settings.php rather than just database.info. Nitpicky note, it's probably clearer to put the blank line between the setParameter call and the database_manager definition.

Also, it's not settings.php that overrides database.info but the bootstrap process, based on information from settings.php. Yes, nitpicking but the current comment is misleading. :-)

+++ b/core/includes/database.inc
@@ -208,11 +207,8 @@
-  if (empty($options['target'])) {
-    $options['target'] = 'default';
-  }
-
-  return Database::getConnection($options['target'])->query($query, $args, $options);
+  $service = empty($options['target']) || $options['target'] == 'default' ? 'database' : 'database.slave';
+  return drupal_container()->get($service)->query($query, $args, $options);

There's a subtle change here wherein the old code supported any target string, while the new code only supports "slave" or "default". That's *probably* OK, since we never actually got around to developing other targets and with the DIC I doubt we will, but I mention it for completeness unless someone wants to object.

+++ b/core/includes/database.inc
@@ -447,7 +423,7 @@ function db_escape_table($table) {
function db_escape_field($field) {
-  return Database::getConnection()->escapeField($field);
+  return drupal_container()->get('database_manager')->escapeField($field);
}

That's not the correct mapping. database_manager doesn't have an escapeField() method; that's on the connection object. (That's likely causing some failures.)

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
@@ -28,6 +27,8 @@ class DatabaseBackend implements CacheBackendInterface {
   /**
    * Implements Drupal\Core\Cache\CacheBackendInterface::__construct().
    */
@@ -38,6 +39,7 @@ public function __construct($bin) {

@@ -38,6 +39,7 @@ public function __construct($bin) {
       $bin = 'cache_' . $bin;
     }
     $this->bin = $bin;
+    $this->connection = drupal_container()->get('database');
   }

Um, eep? I guess this is only temporary until the rest of http://drupal.org/node/1764474 happens? If so, please @todo.

+++ b/core/lib/Drupal/Core/Cache/InstallBackend.php
@@ -34,6 +34,21 @@
   /**
+   * Overrides Drupal\Core\Cache\DatabaseBackend::__construct().
+   */
+  public function __construct($bin) {
+    // All cache tables should be prefixed with 'cache_', except for the
+    // default 'cache' bin.
+    if ($bin != 'cache') {
+      $bin = 'cache_' . $bin;
+    }
+    $this->bin = $bin;
+    if (drupal_container()->has('database')) {
+      $this->connection = drupal_container()->get('database');
+    }
+  }

Ibid.

+++ b/core/lib/Drupal/Core/Database/Database.php
@@ -9,12 +9,8 @@
-abstract class Database {
+class Database {

We can probably rename this to DatabaseManager at this point. It's not something anyone will be typing directly.

+++ b/core/lib/Drupal/Core/Database/Database.php
@@ -97,21 +97,21 @@
-  final public static function startLog($logging_key, $key = 'default') {
-    if (empty(self::$logs[$key])) {
-      self::$logs[$key] = new Log($key);
+  final public function startLog($logging_key, $key = 'default') {
+    if (empty($this->logs[$key])) {
+      $this->logs[$key] = new Log($key);

I think we can get rid of the final declarations here, too. They were needed on the static class to keep people from doing fugly things that didn't work properly in 5.2 anyway. With an instantiated object, I don't think that restriction is necessary anymore.

+++ b/core/lib/Drupal/Core/Database/Database.php
@@ -359,30 +345,30 @@ public static function addConnectionInfo($key, $target, $info) {
+  final protected function openConnection($key, $target, $database_info = array()) {
+    if (empty($this->databaseInfo)) {
+      self::parseConnectionInfo($database_info);
     }

Why do we need to pass in $database_info here, when it's already passed in via the constructor? I'd rather require people to futz with the connection info via the formal APs rather than an inline bypass.

Also, $this->parseConnectionInfo(). That's probably also causing some test fails.

+++ b/core/lib/Drupal/Core/Database/Database.php
index d88633f..8788531 100644
--- a/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php

--- a/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
@@ -48,7 +48,7 @@ protected function getPrefixInfo($table = 'default', $add_prefix = TRUE) {

@@ -48,7 +48,7 @@ protected function getPrefixInfo($table = 'default', $add_prefix = TRUE) {
       $info['table'] = substr($table, ++$pos);
     }
     else {
-      $db_info = Database::getConnectionInfo();
+      $db_info = drupal_container()->get('database_manager')->getConnectionInfo();
       $info['database'] = $db_info['default']['database'];
       $info['table'] = $table;
     }

Oh dear, this is a problem. Calling out to functions is verbotten inside the DB objects. So we can't use drupal_container() here. It sounds like we need to pass the connection array into the schema constructor, too, which means it probably needs to get passed into the connection since the connection is responsible for creating the schema object. Blargh. At least it's a straightforward fix.

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.php
@@ -73,7 +73,7 @@ protected function checkEncoding() {
-    $database_connection = Database::getConnection();
+    $database_connection = drupal_container()->get('database');

Ibid. Although perhaps not as straightforward for the install tasks. :-(

+++ b/core/lib/Drupal/Core/Database/Install/Tasks.php
@@ -160,10 +158,7 @@ public function runTasks() {
-      // This doesn't actually test the connection.
-      db_set_active();
-      // Now actually do a check.
-      Database::getConnection();
+      drupal_container()->get('database');
       $this->pass('Drupal can CONNECT to the database ok.');

Ibid. (And several others I won't mention individually.)

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new2.41 KB
new84.21 KB
FAILED: [[SimpleTest]]: [MySQL] 32,059 pass(es), 995 fail(s), and 301 exception(s).
[ View ]

Ok, fixed the unit tests and figured out what the problem is. It's open database connections. We need to make sure to close all connections that we opened. The patch attempts to do that, but I'm still running into limits, a bit later than before, though. And faster than on HEAD.

Status:Needs review» Needs work

The last submitted patch, database-dic-1811730-8.patch, failed testing.

sun’s picture

Some of the *TestBase changes look bogus to me, especially those that are changing the order of setUp()/tearDown() operations. Reverting them should resolve the test failures for Drupal\system\Tests\Database\ConnectionUnitTest which most probably fixes most of the other test failures, too.

Damien Tournoud’s picture

Ideally, we would get rid of Database completely, and register all the database connections as independent services, linking the current connection with a service aliases.

I would like to see:

'database.[key].[target]' => the database connection for key [key] and target [target]
'database.[key]' => alias to the default target of key [key]
'database' => alias to 'database.default'

We can remove the concept of "active" connection. The procedural wrappers (db_*) can always point to the default key from now on. If you want to use another key, use the connection object directly.

Crell’s picture

Damien: I'd like to end up there as well, but one step at a time. That requires a bit trickier use of the DIC. Let's fix the bug around database-initialization ordering first, then we can flesh out the service IDs.

Berdir’s picture

StatusFileSize
new29.63 KB
new104.62 KB
FAILED: [[SimpleTest]]: [MySQL] 30,586 pass(es), 1,447 fail(s), and 537 exception(s).
[ View ]

@sun: Those changes are necessary IMHO, because the database switch now happens at different times than before (Essentially when changing the controller). Can explain the changes in detail later and add comments, still trying to figure out how this all needs to play together to work.

Still fighting with the open database connections. Having a number of problems here. One big problem is that every time we create a new container, we also create a new database connection if a query is executed while that container is active. And we create quite a few containers and replace them while running tests. I'm currently playing around with closing the connections in drupal_container() when the container is replaced, but that also results in problem because in some cases, we attempt to re-use the previous container.

I've also tried to rely on Database::__destruct(), which we can now actually use because we instantiate the database class and when the container is not used anymore, so is that. But there are two problems with that, as you can see in the current implementation of that method:
a) For multiple containers, it's only called at the end of script execution, which means that we keep connections open for every test method and some test classes have *lots* of them.
b) In some cases, other shutdown functions and __destruct() methods (CacheArray for example) are called later, try to execute a query and if that happens after destroy() was called, then things break in ugly ways. I have no idea how we can know when we can safely close a connection and when we can't.

This won't get any easier if Database would be removed completey. Note that adding a __destruct() to Connection can not replace destroy(), the purpose of destroy() is to remove circular references so that the connection actually can be destructed, it needs to be called manually.

Fixed some of Crell's review, some feedback to that:
- Improved comment and grouping in drupal_container().
- Fixed wrong mapping for escapeField().
- Added a @todo for the drupal_container() calls in the cache backend.
- "We can probably rename this to DatabaseManager at this point. It's not something anyone will be typing directly.". Actually we do. There are still the RETURN_* constants on that class, not sure where we're supposed to move them if they shouldn't be there.
- Removed the finals.
- Removed the $database_info from openConnection(), I think that already existed and was used in the installer...
- drupal_container() calls in the Database classes. Yes, ugly, not yet sure how to get rid of them. We might need to inject them, just like we inject the connection into the statement. Leaving them for now, will tacke that once this is green.

Berdir’s picture

Status:Needs work» Needs review

Go testbot.

Status:Needs review» Needs work

The last submitted patch, database-dic-1811730-14.patch, failed testing.

Crell’s picture

Rather than closing and reopening connections, once a service is defined I believe we can use $container->set() to override it. So perhaps we could do something like:

<?php
$new_container_for_test
->set('database', $original_container->get('database'));
?>

To hand the connection object down from one to the next?

Berdir’s picture

That sounds interesting, but I'm not sure if we're able to correctly define that in all cases. Sometimes containers are created that we have no control over. Especially the whole installation process which executes stuff like install_run_tasks() to verify that the database is supported and sets up a new connection explicitly for that.

I'll try to play with that and improve unnecessary creation of new containers...

chx’s picture

After a discussion with Crell we should add functionality to Connection to allow for prefix changes and thus avoid the open-close all the time.

Berdir’s picture

I'm not sure if that would help.

We actually need the original connection without the test prefix, to store the assertions during a testrun.

chx’s picture

Switch back.

Crell’s picture

What I don't understand here is why it's a problem to just keep the connection objects open and move them around in the database keys list, and pass them as a synthetic service to a test case's container. Why doesn't that work? We're already doing the former now, aren't we?

Berdir’s picture

@Crell: There's no problem with doing that other than that my initial attempts failed with strange errors. But @chx mentioned that there are now better ways to do it after all these refactorings than when I tried it. This part needs to happen anyway, that's where the real problem is. As you said, the new connection for a new prefix is what we're doing already, so while that might be nice, it probably should happen in a follow-up.

Berdir’s picture

StatusFileSize
new94.07 KB

Here is an updated patch. Rerolled and started to convert to Settings. This is currently completely messed up and does not work at all, did a quick re-roll against 8.x after a long time and there's probably a lot of dumb stuff in there :)

This needs to be done IMHO:
- Complete the conversion to Settings as using container parameters does not work anymore as that is dumped, which we don't want AFAIK.
- As we inject settings, we might need to add support for changing them without creating a new objecrt.
- Make database a persisted service.
- Add support for writing $settings arrays with drupal_write_settings() so that we can write the database settings.
- Add support to change the prefix of a connection.
- Probably needs a solution of the ugly ugly database messing during the installation. IMHO, doing the database checks during the test-installation is utterly pointless, so I think we should just skip it.
- Make it work again. Hope hat the above changes will solve the open connections problem during concurrent test runs.
- Decide on what we exactly want to do with the services, alternative connections and so on.

chx’s picture

Assigned:Unassigned» chx
chx’s picture

Assigned:chx» Unassigned

I spent considerable time on getting this to work and I am giving up the refactoring of Database. Now that I have seen the inside of the beast, what's the point? We can already inject Connection objects and we do that. On the other hand, during testing the installer needs a kernel to test whether the database settings exist (created in install_begin_request) and then one during the install itself (created in drupal_install_system) and the test class itself uses a different kernel. So one test method creates three wildly disjoint containers and we most definitely do not want to close and re-create PDO objects each time. So we need to store them somewhere. But where? What about the static properties of a class? Erm... we already have that. And, on the other hand, why bother with creating a database manager service? It's not like we ever want to change it -- it's marked final, what does that tell you about the intentions and point-ness of extending it?

Getting rid of the various Database:: calls is a worthy goal on the other hand and I will examine that, probably in a different issue or even issues, piecemeal and I begun that with some Views issues.

jhedstrom’s picture

Version:8.0.x-dev» 8.1.x-dev
Issue summary:View changes

8.1 at least I think.