Add public method Drupal\views\Plugin\views\query\Sql::getConnection()

Let's use more readable code as the method is new, also fix CS

This new function fulfills the needs, and the tests provide suitable code coverage.

This seems like a fine refactoring and also a nice code cleanup at the same time. When I read the title I thought we should have a subsystem maintainer sign off on the change, but I'm a Views subsystem maintainer and this seems totally a safe change for me. Good test coverage, also. I have just a few nitpicks for us to address:

1. +++ b/core/modules/views/src/Plugin/views/query/Sql.php
   @@ -1248,6 +1248,24 @@ protected function compileFields($query) {
   +   * Get the database connection to use for the view.
   

   Should be "Gets".

2. +++ b/core/modules/views/tests/src/Kernel/Plugin/SqlQueryTest.php
   @@ -79,4 +80,49 @@ public function testExecuteMetadata() {
   +   * @covers \Drupal\views\Plugin\views\query\Sql::getConnection
   +   *
   +   * This cannot be made as a PHPUnit test, because the tested method uses the
   +   * method \Drupal\Core\Database\Database::getConnection() which is a 'final'
   +   * method and therefor cannot be mocked.
   

   This should have a one-line summary.

3. +++ b/core/modules/views/tests/src/Kernel/Plugin/SqlQueryTest.php
   @@ -79,4 +80,49 @@ public function testExecuteMetadata() {
   +    // Cleanup the created database connections.
   

   "Clean up" (two words).

Thank you!

Updated patch as per the changes suggested in #6, please review.

All recommendations for changes to comments have been made. No changes to code so back to RTBC.

Thanks for the quick fixes for my nitpicks. :) One point needs a little more work:

+++ b/core/modules/views/tests/src/Kernel/Plugin/SqlQueryTest.php
@@ -83,9 +83,7 @@
   /**
    * @covers \Drupal\views\Plugin\views\query\Sql::getConnection
    *
-   * This cannot be made as a PHPUnit test, because the tested method uses the
-   * method \Drupal\Core\Database\Database::getConnection() which is a 'final'
-   * method and therefor cannot be mocked.
+   * This cannot be made as a PHPUnit test as it uses the 'final' method.

This change is not quite right. :) The full paragraph here is necessary and useful.

What's missing is a one-line summary above the @covers that describes what the method does. This should be followed by the full detail of the existing paragraph, and then the @covers goes below that.

Thanks!

@xjm For tests @covers should be enough https://www.drupal.org/node/1354#tests (there was policy accepted at least for phpunit tests)

@covers is self explanatory - means testing specific method

I pointed out to @andypost that that bullet links a reference that only allows omitting a one-line summary for a class in lieu of just using @coversDefaultClass. There's nothing about @covers replacing the one-line summary for methods; in fact, it says:

Every test class MUST have a PHPDoc summary line. Only PHPUnit tests MAY skip the PHPDoc summary line if their PHPDoc specifies a @coversDefaultClass. See PHPUnit @coversDefaultClass annotation documentation

https://www.drupal.org/node/2116043#s-examples--3

Nothing about skipping it for @covers. Even if there were, though, this particular method is way longer than just a 1:1 test of a simple method and required a whole paragraph to explain, so I would still ask for a one-line summary here that's more human-readable.

Also NW still because #8 is definitely not right regardless; we shouldn't be deleting most of the useful paragraph. My review meant add a sentence at the top.

As I got we should use @covers only for unittests which has @coversDefaultClass

So make sense to file follow-up and maybe policy to introduce sniff in coder

+++ b/core/modules/views/tests/src/Kernel/Plugin/SqlQueryTest.php
@@ -79,4 +80,51 @@ public function testExecuteMetadata() {
+   * This cannot be made as a PHPUnit test, because the tested method uses the

Nitpick. it is a PHPUnit test, just not a Unit test.....strictly speaking it should probably say something like 'This is a kernel test because....'

Great to see test coverage for this!

Oops, yep, let's fix #15. If it were just swapping one word I might fix it on commit, but rewrapping whole paragraphs on commit is error-prone. NW for that too. Thanks @Lendude!

So, that would be something like this

Pushed to 9.1.x and published the CR. Thanks!

This issue continues work started in #3113403: Make Drupal\Core\Database\Query\Condition driver overridable and finished in #3130655: Deprecate creating an instance of the class Drupal\Core\Database\Query\Condition with the new keyword, however, it has not been committed to 8.9.x despite #3113403: Make Drupal\Core\Database\Query\Condition driver overridable has been, so again supporting a single codebase for D8 and D9 is not possible, but it can be with a little effort.

I can provide a backported patch for D8.9, anyone interested (i.e. will it be committed)?