Deprecate creating an instance of the class Drupal\Core\Database\Query\Condition with the new keyword

+++ b/core/modules/views/src/Plugin/views/query/Sql.php
@@ -1111,13 +1110,26 @@ protected function buildCondition($where = 'where') {
+    $target = 'default';
+    $key = 'default';
+    // Detect an external database and set the
+    if (isset($this->view->base_database)) {
+      $key = $this->view->base_database;
+    }
+
+    // Set the replica target if the replica option is set
+    if (!empty($this->options['replica'])) {
+      $target = 'replica';
+    }
+
+    $connection = Database::getConnection($target, $key);

You have copied this part from the method query(). The problem now is that we have the same code twice. It is a bit of a no-no for programmers. See: https://en.wikipedia.org/wiki/Don%27t_repeat_yourself. Create a new helper method and put the double code in the helper method, then use the helper method instead of the double code.

Shall we change all occurrences of new Condition in the views module in this issue. Other occurrences are:
- modules/views/src/ManyToOneHelper.php
- modules/views/src/Plugin/views/display/EntityReference.php
- modules/views/src/Plugin/views/filter/BooleanOperator.php
- modules/views/src/Plugin/views/filter/StringFilter.php

@beakerboy: My apologies , I was not trying to offend you.

The helper method does not need testing. Make it a public method and call it something like "getConnection()". By make it public we can use it in the plugins for the other condition changes.

1. +++ b/core/modules/views/src/Plugin/views/filter/BooleanOperator.php
   @@ -41,6 +42,42 @@ class BooleanOperator extends FilterPluginBase {
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, Connection $connection) {
   +    parent::__construct($configuration, $plugin_id, $plugin_definition);
   +    $this->connection = $connection;
   

   We're going to need to allow this to be constructed without a Connection object for the purposes of BC. So it should be Connection $connection = NULL and then if it is NULL get the database connection using \Drupal::service().

2. +++ b/core/modules/views/src/Plugin/views/query/Sql.php
   @@ -1095,6 +1094,22 @@ public function placeholder($base = 'views') {
    
   +  public function getConnection() {
   

   This needs documentation - and why public? I think it can be protected.

3. Re the ManyToOneHelper - we need to change the constructor to accept a database connection and deprecate not passing one it. And then change all calls to pass one it.

Fix #11.1 and reroll for 9.1.x core/modules/views/src/Plugin/views/query/Sql.php.rej

+++ b/core/modules/views/src/Plugin/views/filter/BooleanOperator.php
@@ -48,6 +49,26 @@ class BooleanOperator extends FilterPluginBase {
+    $instance->connection = $container->get('database');

I think this is wrong. Most of the time this will be the right database connection. It goes wrong when the view is to an external database that is of an other type then the main database. For instance the main database is MySQL and the external view is to MS SQL Server. I think the database connection to use should be: $this->view->query->getConnection(). That is also why the method should be public.

+++ b/core/modules/views/src/Plugin/views/filter/BooleanOperator.php
@@ -48,6 +49,26 @@ class BooleanOperator extends FilterPluginBase {
+    $instance->connection = $container->get('database');
+    return $instance;

First off, I''m not a fan of this pattern inside the create method in core. It's great for contrib and custom code to prevent breaking on constructor changes, but core itself should not need this pattern since if it breaks any constructors inside core, we can just update them too. And we have BC patterns in place to deal with and report constructor changes. Exactly like @alexpott pointed out in #11.1

The whole 'replica' logic seems to have zero test coverage, so refactoring that and turning it into public API seems like a bad idea within the scope of this issue. So I would say keep it protected and only use it in Sql. And if we feel the need to make it public we should do it in a follow up and then add proper test coverage for this. So to answer @Beakerboy in #7 "If I create a new helper method, does that method then need a test?", I think "yes" if we want to make it public and has zero coverage currently.

The coverage really goes for all the other changes as well. Did somebody check that we have coverage for these changes? I would love to say that we have 100% coverage for all things in Views, but uhhh I'd be lying. So we should really check if we have existing coverage for all the updates, and if not, we need some tests to prove that we are not breaking anything with this (they might already exist, really don't know).

Created #3139353: Add public method Drupal\views\Plugin\views\query\Sql::getConnection().

#3139353: Add public method Drupal\views\Plugin\views\query\Sql::getConnection() landed, so we can start working on this issue.

Re-roll and a bit of clean-up

The conditions needs to use \Drupal\views\Plugin\views\query\Sql::getConnection() as their database connection.

All the new Condition() in the views plugins are changed to using the method \Drupal\views\Plugin\views\query\Sql::getConnection(). There are tests added for the changes.

+++ b/core/modules/views/src/Plugin/views/query/Sql.php
@@ -1111,13 +1111,13 @@ protected function buildCondition($where = 'where') {
+    $main_group = $this->getConnection()->condition('AND');
+    $filter_group = $this->groupOperator == 'OR' ? $this->getConnection()->condition('OR') : $this->getConnection()->condition('AND');
...
     foreach ($this->$where as $group => $info) {
...
       if (!empty($info['conditions'])) {
...
+        $sub_group = $info['type'] == 'OR' ? $this->getConnection()->condition('OR') : $this->getConnection()->condition('AND');

Please revert $connection = $this->getConnection(); out of loop, no reason for extra function call

Other then this optimization it looks awesome ready!

@daffie Thank you for great test coverage!!!

For #24: Fixed. Good find @andypost! I think I was to much focused on writing the tests. :)

For #26: I think we should handle them in a separate issue. There are more problems with the those query extensions. The solution from this issue is properly be part of the solution over there, only at the moment I am not sure how exactly.

I think the unit testing being added here is not that great and a bit fragile. When you're mocking the world to see what class something is using without actually doing any testing of what the class is supposed to be doing then there's a problem.

I think we need to re-think how we're handling this. Given that there will plenty of code in contrib that does $conditions = new Condition('OR'); (see http://grep.xnddx.ru/search?text=new%20Condition%28&filename=&page=6) we need to deprecate instantiating the class in this way. Yes this issue is a good step on the road to doing this but if we could deprecate this then we'd not have the urge to add the unit tests and we'd be able to fix all of core in one go, not regress in core by mistake, and prompt contrib to fix themselves.

So how might we do this? I think we need to move the current \Drupal\Core\Database\Query\Condition to \Drupal\Core\Database\Query\CoreCondition and then add a new \Drupal\Core\Database\Query\Condition that extends \Drupal\Core\Database\Query\CoreCondition but issue a deprecation in the constructor.

I think this is worth it because it's is the only db class that has been used like this and we're now changing best practice.

Created a patch for the by @alexpott suggested solution.
Removed the added tests, as @alexpott was not very happy with them.
Lets see what the testbot thinks of this patch.
I did not add an interdiff file as this a very different solution.

123 failures looks promising but interesting

Lets see if this patch makes the testbot more happy.

Same patch as the one from comment #34, only this one uses git "Optimize diffs for renamed and copied files" from https://www.drupal.org/documentation/git/configure. Thank you @alexpott for pointing this out.

Added the method GetConnection() to the class Drupal\Core\Database\Query\Query. It is needed because in the EntityQuery condition class we have the Select query object and from there we need to get the connection object.
Hopefully this will make the testbot happy.

Edit: Also changed the method Drupal\Node\NodeGrantDatabaseStorage::buildGrantsQueryCondition() to a regular function instead of a static one.

1. +++ b/core/lib/Drupal/Core/Database/Query/Condition.php
   @@ -2,418 +2,18 @@
     * Generic class for a series of conditions in a query.
     */
   -class Condition implements ConditionInterface, \Countable {
   

   I asked in Slack if we needed @deprecated annotation anymore, and it looks like there is some confusion. We do need to continue to add it per @alexpott.

2. +++ b/core/lib/Drupal/Core/Database/Query/Condition.php
   similarity index 99%
   copy from core/lib/Drupal/Core/Database/Query/Condition.php
   
   copy from core/lib/Drupal/Core/Database/Query/Condition.php
   copy to core/lib/Drupal/Core/Database/Query/CoreCondition.php
   

   This line in the diff was easy to miss, but means that we have the same code that was "removed" in Condition. +1

For #39.1: Fixed. Added @deprecated to the docblock.

Sorry, only nitpicks and standards questions here.

1. +++ b/core/lib/Drupal/Core/Database/Query/Condition.php
   @@ -2,418 +2,24 @@
   +   * @deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Creating an
   +   *   instance of this class is deprecated.
   

   Should this be in the class docblock instead as the entire class is deprecated? Or is only the __construct method deprecated?

   It seems there is a new phpcs rule about adding @see directly below @deprecated, which is probably the URL to the Change Record.

2. +++ b/core/lib/Drupal/Core/Database/Query/Condition.php
   @@ -2,418 +2,24 @@
   +    @trigger_error('Creating an instance of this class is deprecated in drupal:9.1.0 and is removed in drupal:10.0.0. See https://www.drupal.org/node/3159568', E_USER_DEPRECATED);
   

   For some reason phpcs and coder (installed in vendor) isn't liking this even though it seems to be accurate. I tried to manually change it, but nothing I did made coder happy. drupalci doesn't seem to care about this from the previous test run so I don't think there's anything to change here. I think it's a bug in the sniff.

3. +++ b/core/lib/Drupal/Core/Database/Query/Query.php
   @@ -179,4 +179,13 @@ public function &getComments() {
   +  }
    }
   

   From drupalci testbot:

   line 190	Expected 1 blank line after function; 0 found
   191	The closing brace for the class must have an empty line before it
   

@mradcliffe: Thank you for your reviews!

For #41.1 and #41.3: Fixed.

For #41.2: It is properly a bug in the sniff, only I do not run PHPCS and coder on my system.

Updating the title and the IS.

Updated the added comment in the method Drupal\Core\Database\Connection::condition().

Updated the CR for the API addition for the method Drupal\Core\Database\Query\Query::getConnection().

Added reroll of patch #44.

@ravi.shankar: Thank you for rerolling the patch. Only when you look at the size differences between my patch and your patch, your patch is 13KB bigger. To fix it you must create the patch the following change in your .gitconfig file:

[diff]
  renames = copies

For more info see: https://www.drupal.org/documentation/git/configure. And yes I made the same mistake myself. :)

@daffie Thank you so much.

I have again added a reroll of patch #44.

@beakerboy: Thank you for the review and the RTBC!

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -1164,6 +1165,12 @@ public function schema() {
   +    // Creating an instance of the class Drupal\Core\Database\Query\Condition
   +    // with the new keyword is deprecated. Instead use the class
   +    // Drupal\Core\Database\Query\CoreCondition.
   

   This comment is a bit misleading. It should be more about the why and less of the how.

2. +++ b/core/lib/Drupal/Core/Database/Query/Condition.php
   @@ -2,418 +2,26 @@
   +class Condition extends CoreCondition {
   

   Doing some thinking. It'd be great to avoid adding the CoreCondition class. And I think we can. We can add an extra argument to the constructor of $trigger_deprecation and default it to TRUE. In \Drupal\Core\Database\Connection::condition() we can pass false to this and not trigger the deprecation.

   That way we've not added a class which we have to think about from an API perspective.

3. +++ b/core/lib/Drupal/Core/Database/Query/Condition.php
   @@ -2,418 +2,26 @@
   +    @trigger_error('Creating an instance of this class is deprecated in drupal:9.1.0 and is removed in drupal:10.0.0. See https://www.drupal.org/node/3159568', E_USER_DEPRECATED);
   

   This message should tell people what to do. Ie.

   'Creating an instance of this class is deprecated in drupal:9.1.0 and is removed in drupal:10.0.0. Use Database::getConnection()->condition() instead. See https://www.drupal.org/node/3159568'

Fixed all 3 points from @alexpott in comment #53.

Committed f124d52 and pushed to 9.1.x. Thanks!

Great work on this, thanks!