Use the database.replica service where appropriate

Fix copy paste error

I like this issue in that it makes working with the replica database easier (or possible ;-)). I wonder, though, since we are already using Database::getConnection() as a factory for the database service, wouldn't it be easier for consuming code to define a database.replica service, that directly yields the replica connection? What do you think?

1. +++ b/core/lib/Drupal/Core/Database/DatabaseFactory.php
   @@ -0,0 +1,25 @@
   + * Test database factory.
   

   The default database factory

2. +++ b/core/modules/comment/comment.services.yml
   @@ -11,7 +11,7 @@ services:
   +    arguments: ['@database', '@current_user', '@entity.manager', '@state', '@database.factory']
   

   no reason to inject database & factory same time

3. +++ b/core/modules/comment/src/CommentStatistics.php
   @@ -52,20 +60,23 @@ class CommentStatistics implements CommentStatisticsInterface {
   +    $connection = $accurate ? $this->database : $this->connectionFactory->getDatabaseConnection('replica');
   

   $connection = $this->connectionFactory->getDatabseConnection($accurate ? 'default' : 'replica') more descriptive

btw maybe better to add new "factory method" to existing database class instead of new class?

@tstoeckler Shouldn't we provide the factory so custom code can connect to arbitrary databases, not just default and replica?

@longwave On the one hand, I think if people are using additional database targets they could also provide respective services for those. On the other hand, I wouldn't necessarily be opposed to providing a dedicated factory class+service, but in that case, wouldn't it make sense to deprecate Database::getConnection() and actually move its contents into DatabaseFactory::get()? That way we could actually use the new database.factory service as the factory key for the database service declaration.

Just thinking out loud here, I don't mind if we proceed as is...

Fixed points from #10 and some suggestions from #13. Deprecating Database::getConnection() looks tricky as it needs many of the parts encapsulated elsewhere in the class. Maybe we could just remove the abstract and make Database itself the factory, although renaming it would be nice,

Fixed typo.

Whoops, bad patch. Interdiff is ok though.

Looks nice!

1. +++ b/core/modules/comment/tests/src/Unit/CommentStatisticsUnitTest.php
   @@ -82,7 +89,15 @@ protected function setUp() {
   +    $this->databaseFactory = $this->getMockBuilder('Drupal\Core\Database\DatabaseFactory')
   +      ->disableOriginalConstructor()
   +      ->getMock();
   +
   +    $this->databaseFactory->expects($this->any())
   +      ->method('get')
   +      ->will($this->returnValue($this->database));
   +
   +    $this->commentStatistics = new CommentStatistics($this->databaseFactory, $this->getMock('Drupal\Core\Session\AccountInterface'), $this->getMock('Drupal\Core\Entity\EntityManagerInterface'), $this->getMock('Drupal\Core\State\StateInterface'));
   

   Since we're touching this, shall we use the ClassName::class notation here? i.e. DatabaseFactory::class instead of 'Drupal\Core\Database\DatabaseFactory' and others involved too.

2. +++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
   @@ -150,7 +157,7 @@ public static function create(ContainerInterface $container, array $configuratio
   +   * @param \Drupal\Core\Database\DatabaseFactory $connection_factory
   

   Can we unify on a naming convention for the variable? $database_factory or $connection_factory?

I think the new class deserves a test of its own.

Fixed #19 and added some basic tests. Not sure what else we can add without duplicating ConnectionTest.

One more tiny docs fix.

+++ b/core/tests/Drupal/KernelTests/Core/Database/DatabaseFactoryTest.php
@@ -0,0 +1,32 @@
+  public function testGet() {
+    $factory = new DatabaseFactory();
+
+    $connection = $factory->get('default');
+    $this->assertInstanceOf(Connection::class, $connection);
+
+    // Ensure that when called with the same argument, the same instance is
+    // always returned.
+    $this->assertSame($connection, $factory->get('default'));
+  }

Maybe we can also test that $connection is the same as what you get via Database::getConnection() and via \Drupal::database()?

Rerolled, used the new service in the temp conversions made in #2873684: Replace all calls to db_select, which is deprecated, addressed #22.

We already tried to properly container-ize the database code a very long time ago in #1811730: Refactor Database to rely on the DIC solely.

That was closed as outdated, but that's the wrong status, the reality is more like "Closed (we give up, can't work)" ;)

This is I guess a small step in making it a bit better, but making the factory itself a service and not just a static method called by the container is most likely not going to happen, ever, as we need the database to be able to initialize the container, also fun during the installer.

@Berdir: Yeah, I entirely agree that we can't move this entirely into the container because of the chicken-and-egg problem around bootstrap and install. However this fix (or at least something like this) is needed to get an alternative connection injected for classes that need this. The other suggested option was to hardcode the @database.replica service and ask users who need further database connections to set up the service definition themselves.

I guess the question is: is there a use case for dynamically selecting an arbitrary database target, in which case we probably need a factory service? If not, we can just provide the replica service in core to solve this issue.

Yeah, I mostly just wanted to add some context/background.

+++ b/core/modules/comment/src/CommentStatistics.php
@@ -64,8 +73,8 @@ public function __construct(Connection $database, AccountInterface $current_user
    */
   public function read($entities, $entity_type, $accurate = TRUE) {
-    $options = $accurate ? [] : ['target' => 'replica'];
-    $stats = $this->database->select('comment_entity_statistics', 'ces', $options)
+    $connection = $this->databaseFactory->get($accurate ? 'default' : 'replica');

isn't this exactly what you are asking for, this does dynamically select default or replicate?

Also, I'm wondering why DatabaseFactory needs to extend from Database? That means you could call all those other methods of Database on it too, which actually seems wrong?

Why not just make it a standalone class that calls Database::getConnection() instead of static::getConnection() ?

Well, in that case we could just inject both database and database.replica and decide between the two, so it's not fully dynamic - I was thinking of the case where somehow we get an arbitrary string as a connection target and need to resolve that.

Second point is a good idea, let's do that.

If I understand #27 correctly and the plan is to reduce scope and simply provide a database.replica service in addition to the existing database service, then I think that's a good idea. It will

1. make the comment statistics code nicer
2. be useful for other places in core and contrib that want to properly work with replicas
3. does not stop us from adding a proper database factory in the future

So it turns out we already have a database.replica service, it has existed for several years - it is just buried elsewhere in core.services.yml. This patch moves it next to the database service and injects it into the comment and search modules.

Wow, I didn't know that. That's very bizarre, but I think it's a nice solution anyway, even if it's apparently a sort of "pre-existing" solution. From my point of view this is RTBC if it comes back green, but another review couldn't hurt, so not touching the status just yet.

+++ b/core/modules/comment/comment.services.yml
@@ -11,7 +11,7 @@ services:
   comment.statistics:
     class: Drupal\comment\CommentStatistics
-    arguments: ['@database', '@current_user', '@entity.manager', '@state']
+    arguments: ['@database', '@database.replica', '@current_user', '@entity.manager', '@state']
     tags:
       - { name: backend_overridable }

I wonder whether it's OK to add the new argument in between previous ones or we should rather add it at the end of the constructor (with a NULL default). For BC. Same in the other injection in NodeSearch.

+1 to 31, see also my comment/rant last night in #2949018-14: [policy, no patch] Make removal of deprecated usages a feature release blocker.

NW for #31/32, we can easily provide a sensible fallback here in the default connection.

Updated IS.

We should also address the other conversions that were made in #2873684: Replace all calls to db_select, which is deprecated using Database::getConnection:

core/modules/search/src/Plugin/views/argument/Search.php:      $this->searchQuery = Database::getConnection('replica')->select('search_index', 'i')->extend(ViewsSearchQuery::class);
core/modules/search/src/Plugin/views/filter/Search.php:      $this->searchQuery = Database::getConnection('replica')->select('search_index', 'i')->extend(ViewsSearchQuery::class);
core/modules/search/src/SearchQuery.php:    $count = Database::getConnection('replica')->select($inner->fields('i', ['sid']), NULL);
core/modules/tracker/tracker.pages.inc:    $query = Database::getConnection('replica')->select('tracker_node', 't')

None of those are suitable for DI, so what's wrong with Database::getConnection() there?

Can't we use \Drupal::service('database.replica')? That would allow swapping the service, which using the static call does not. See (with appropriate adjustments) https://www.drupal.org/node/2993033

(which tells me we should reflect something of this issue in that CR...)

Actually I think SearchQuery can just use $this->connection.

    $query = $this->databaseReplica
      ->select('search_index', 'i')
      ->extend('Drupal\search\SearchQuery')
      ->extend('Drupal\Core\Database\Query\PagerSelectExtender');

extend() passes its own connection object into the SearchQuery constructor, so it should already be using the replica connection?

#39 sounds right, good catch :)

Addressed all points of review.

#41 looks good to me but not sure I can RTBC as I already worked on a lot of it.

It looks good to go now, not sure about moving service in core.services but it makes sense to keep databases near

Yeah, I actually think the moving of the service is a pretty nice touch. I actually have a project where I defined that service myself in a custom module because I hadn't found it in core. Arguably that may say more about me than about core, but I still think it makes sense in the scope of the issue to "promote" the usage of the service.

I think it definitely says something about the sprawl of core.services.yml, that this was defined but nobody knew it was there. Should we have a coding standard for the order of services.yml files? Is there anything that makes sense other than alphabetical, as multiple related services are generally namespaced anyway?

Committed cff48a2 and pushed to 8.7.x. Thanks!

Fixed a couple of code style issues on commit.

Edited the draft CR https://www.drupal.org/node/2993033, adding the findings here.