Document the recommended ways to obtain the database connection object

\Drupal::database() and Database::getConnection() are equivalent if 'database' is in the container. Because \Drupal::database() is:

  public static function database() {
    return static::getContainer()->get('database');
  }

and the service definition for 'database' is:

  database:
    class: Drupal\Core\Database\Connection
    factory: Drupal\Core\Database\Database::getConnection

Well method requires the container to exist and one does not. But they do return exactly the same object.

>>> spl_object_id(\Drupal::database());
=> 136
>>> spl_object_id(\Drupal\Core\Database\Database::getConnection());
=> 136

Obviously in most object we prefer dependency injection but for procedural code and tests is there really any difference? I'm not sure there is. Both \Drupal::database() and Database::getConnection() represent anti-patterns - ie. using the container as a service locator and having a static singleton so it doesn't feel as if there is a right answer.

I think it might be okay to shrug here and just say we'll cope with both when and if we get to a place when either the \Drupal class can be removed or the Database::getConnection() static can be.

My 2 cents:

in runtime code, since we are preaching injection, use \Drupal::database whenever it’s not possible to inject (like procedural code), and the container is initialised.

in test code, add a $connection property to base test classes like KernelTestBase and BrowserTestBase and then use that for any db operation, so we won’t have to bother.

@mondrake I'm not too keen on adding a connection property to all tests because it is something that must be kept in sync and whilst we don't have any tests that change real database connection credentials it is possible to write such tests.

Code which depends on the database should use $container->get('database') and IoC, or \Drupal->database() for functional code. That way the database could be a mocked service for testing purposes, consistently across all the codebase. That's a big part of why one bothers with a container in the first place.

We shouldn't add a connection property to tests because there are already two 'right' ways to get the database. :-)

So there's one question to ask: Is there a container available? That is, are we at sufficient bootstrap level to have a container?

Yes: Use either the DI $container->get('database') or \Drupal::database(), in OOP or procedural code. Use \Drupal::database() if there is no opportunity to inject, as with a procedural function.

No: Use Database::getConnection().

In a test which boots the container (functional, kernel), use $this->container->get('database').

@Mile23 you know that tests use of $this->container->get('database') is totally not agreed - especially in Browser or Javascript tests where that object can get hopelessly out of date. We should be discouraging all container access in those type of tests.

Is it feasible or desired to abstract the details into one of the test bases?

Seems only the logic for test code still needs to be agreed upon. Started adding the non-test part to the CR @ https://www.drupal.org/node/2993033.

Reviews/updates there appreciated.

Re #10: @Mile23 you know that tests use of $this->container->get('database') is totally not agreed

That's the title of this issue, right? I refrained from updating the IS for this reason. :-)

If the test expects the container to be rebuilt as a consequence of what it's doing, then it should either call $this->rebuildContainer() or manage the expected container changes in some other way.

If the test doesn't expect the container to change, then containers going out of sync is a bug that is proven by the test.

Alternately, if we always expect the container to be in an unknown state, (even the one the kernel keeps track of), then either add FunctionalTestSetupTrait::container() which always fetches a new one, or remove BTB::container and normalize on \Drupal.

Adding 'needs followup' because whatever we decide should inform an issue which changes how tests work.

in procedural code the suggestion is to minimize calls to \Drupal::database() via using local variable

btw For tests there's \Drupal\Core\Test\TestSetupTrait::getDatabaseConnection which has no usage except simpletests

@Mile23 I guess you mean #2066993: Use \Drupal consistently in tests about follow-up

#15

in procedural code the suggestion is to minimize calls to \Drupal::database() via using local variable

code example in the draft CR is like that already

@andypost: Based on @alexpott's concerns in #10, it seems that functional tests are broken, because they should expect the container to change during the request. Therefore we need to change the way tests work, so that they don't keep a reference to the container.

However, if we expect we can keep a reference to the container during a functional test, then a) we shouldn't change how tests work, and b) @alexpott's concerns in #10 illustrate that there is a bug in core which changes the container.

If #7 1.2 is correct, then it means we don't care about the test fixture container being the 'right' one, so the bug is in the test.

I'd argue (and just did... :-) that #7 1.2 is incorrect, and the bug is in core.

In fact, I'll just update the IS to fix #7 1.2 and add the 'correct' way to do a test. If that's not the correct way to do a test, then there's a bug either in core or in the test system. It now says this:

In kernel and functional test classes we have $this->container->get('database'). Some test authors might discover that the container referenced by the test class will be out of sync with the current container during the request in a functional test. In that circumstance, a test author might call $this->rebuildContainer() and then access $this->container->get('database') again.

It looks that kernel and functional test should use same method to access test database

Actually container can get rebuild in kernel tests as well... so after contacting a child site and expecting container rebuild you need to call static method \Drupal\Core\Database\Database::getConnection()

As example - each test case will have own static connection independent from container
As example #2850033-30: Replace all calls to db_delete, which is deprecated

Tagging as 'blocker' - need a conclusion on the approach in test contexts to proceed with issue related to deprecation of db_* functions.

Actually container can get rebuild in kernel tests as well... so after contacting a child site and expecting container rebuild you need to call static method \Drupal\Core\Database\Database::getConnection()

Correct. That's the bug I'm talking about. That's not how things should work.

EITHER: a) Tests shouldn't expect the container to be consistent (in which case BTB::container is a lie and should be removed), OR: b) Breaking the container model during a request is a bug that needs to be fixed.

So a project maintainer needs to decide which is which, and we can proceed in a follow-up.

I vote for b, but I'm not a maintainer.

Why?

Install-time hooks are invoked from a service that is located using the container. It's true that someone *could* make a bespoke installer that does it another way, but I'd think that would be unsupported.

Looks there's agreement that kernel tests should use services (database, database.replica) when they accessible
Functional tests should use Database::getConnection()
What's left here?

I think we should update CR in #2947946: Create change record for all deprecated db_* functions with cons from here

#26: Functional tests should use Database::getConnection()

Incorrect. See IS or update it.

Looks it needs some patch to update api.d.o and some docs pages

1. +++ b/core/lib/Drupal/Core/Database/database.api.php
   @@ -220,6 +220,25 @@
     * also the @link container Services and Dependency Injection topic. @endlink
   + * In OOP code:
   + *  - If it possible, use DI to use @database service or
   + *    $container->get('database'); to inject the database connection;
   + *  - If not possible (as in a static method of a class),
   + *    use \Drupal::database();
   + *  - If services are not yet available,
   + *    \Drupal\Core\Database\Database::getConnection() can get a database
   + *    connection;
   + *  - In unit tests, we do not have a booted kernel or a built container. Unit
   + *    tests should generally not access a database. A unit test which need
   + *    a database service should be converted to a kernel test;
   + *  - In kernel and functional test classes we have
   + *    $this->container->get('database'). Some test authors might discover that
   + *    the container referenced by the test class will be out of sync with
   + *    the current container during the request in a functional test. In that
   + *    circumstance, a test author might call $this->rebuildContainer() and
   + *    then access $this->container->get('database') again;
   + * In the Procedural code, i.e. *.module, *.inc or script files:
   + *  - Use \Drupal::database(); to get database connection;
     *
   

   I think there's too much detail here. We should just list how to get a database in order of preference, but also it should include some kind of code examples.

2. +++ b/core/lib/Drupal/Core/Installer/Form/SiteConfigureForm.php
   @@ -9,6 +9,7 @@
    use Drupal\Core\State\StateInterface;
    use Drupal\user\UserStorageInterface;
   +use Drupal\user\UserInterface;
    use Symfony\Component\DependencyInjection\ContainerInterface;
    
    /**
   @@ -175,7 +176,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   
   @@ -175,7 +176,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
        $form['admin_account']['account']['name'] = [
          '#type' => 'textfield',
          '#title' => $this->t('Username'),
   -      '#maxlength' => USERNAME_MAX_LENGTH,
   +      '#maxlength' => UserInterface::USERNAME_MAX_LENGTH,
          '#description' => $this->t("Several special characters are allowed, including space, period (.), hyphen (-), apostrophe ('), underscore (_), and the @ sign."),
   

   This looks like a patch for a different issue crept in.

Add details about the follow up to the Remaining Tasks.
Since this is about documentation, and according to Issue tags -- special tags, changing to the documentation component.

1. +++ b/core/lib/Drupal/Core/Database/database.api.php
   @@ -220,6 +220,42 @@
   + * In OOP code:
   

   OOP has not been defined prior to this so should not be used. Can this be 'Object Oriented code'?

2. +++ b/core/lib/Drupal/Core/Database/database.api.php
   @@ -220,6 +220,42 @@
   + *  - If it possible, use DI to use @database service or
   

   In this documentation there are a couple of instances of 'dependency injection' and no use of an acronym for it. Therefor, this should also use the full text, 'dependency injection'.

   And s/If it/If/

3. +++ b/core/lib/Drupal/Core/Database/database.api.php
   @@ -220,6 +220,42 @@
   + *  - If not possible (as in a static method of a class),
   

   Suggest this instead
   "If it is not possible to use dependency injection, e.g. in a static method,"

4. +++ b/core/lib/Drupal/Core/Database/database.api.php
   @@ -220,6 +220,42 @@
   + * In the Procedural code, i.e. *.module, *.inc or script files:
   

   s/In the Procedural/In procedural/

5. +++ b/core/lib/Drupal/Core/Database/database.api.php
   @@ -220,6 +220,42 @@
   + *    ```
   

   s/```/@code/
   This is needed in other places to identify the code examples.

6. +++ b/core/lib/Drupal/Core/Database/database.api.php
   @@ -220,6 +220,42 @@
   + *    ```
   

   s/```/@endcode/
   This is needed in other places to identify the code examples.

Just a few things but I would like to get other eyes on this as well.

Seems to have CI failures.

Have not reviewed.

There doesn't appear to be any open threads and reviewing the text I think that makes sense.

The examples for when to use for what test type is good. Wonder if this should also be updated https://www.drupal.org/docs/drupal-apis/database-api/instantiating-a-dat...

Will leave the Follow up tag but it's from 5 years ago I think the issue being discussed then may be fixed as far as referencing the database from functional?

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Not sure how to fix the problem with phpstan. When I try to manually check the changed file it tells me [ERROR] No files found to analyse.

For now let's tell the bot to skip this one.

Added some comments to the MR.

+ * In Object Oriented code:
+ *  - If possible, use dependency injection to use "database" service or
+ *    $container->get('database'); to inject the database connection;

The indentation is wrong, isn't it? The dash should be in the same column as the "I" on the previous line.
Also, ending bullet points with a semi-colon is weird. They should end in a period. If this were all one sentence without bullet points, then perhaps using a semi-colon to separate the clauses would be acceptable, but it would be a really long and confusing sentence with all those clauses. Better IMO to stick with the bullet points and the conventional bullet point punctuation.

+ *  - In unit tests, we do not have a booted kernel or a built container. A
+ *    unit tests which need a database service should be converted to a
+ *    kernel test;

Instead of "A unit tests which need" it should be "A unit test that needs" or "Unit tests that need". Note the use of the singular vs plural forms, as well as the use of "that" instead of "which".

Are we still using i.e. and e.g. in documentation? I thought the community was against it, going back more than 10 years now ... I have no problems with this usage, just pointing this out because we don't want to have a regression in our usage. See for example #2760911: Use of "e.g." and "i.e.".