Make Drupal\Core\Database\Query\Condition driver overridable

I was able to use a custom Condition by overriding the Select constructor. My new constructor first called the parent, then replaced $this->condition with my custom condition class.

You might be right. I’ll have to make a custom test case that uses a regular expression within a nested Condition.

It looks like Select, Delete, Update, and Merge need to call $connection->condition() inside their constructors.

I already made one:
https://github.com/Beakerboy/sqlsrv/blob/Custom-condition/tests/src/Kern...

This test passes with your patch in #2, but after removing my custom constructor, all the core REGEXP tests fail, which led me to marking this issue as “needs work”.

Updated Patch. I was making this against 8.8.x, but it'll probably work as-is on 9.x

It looks like the Unit Test OrderBy::setUp() needs to be able to get a Condition stub from the mocked connection.

Updated Patch

Thanks for opening this and for the patch! Since this is necessary to fix #2031261: Make SQLite faster by combining multiple inserts and updates in a single query, recategorizing it as a task, and in-scope for 9.0.x (and 8.9.x, but it needs to get into 9.0.x first).

Has anybody reviewed the 8.8.x patch (#14)? It would be great to have it moved to RTBC.

You can review the patch.

@daffie...That is my patch, I can't review my own patch. Feel free to move this issue forward. I have faith in you!

@daffie, yes, I know about the review process...we've both been working with drupal for nearly the same length of time. I don't have a good test set-up for D9, so I've been working off D8.8. Your patch appears fine, but I have not actually tested it myself. In reviewing the code it looks like I missed the ConditionGroupFactory, so I can add that to my patch, test it, consider it the same as yours, but re-rolled to D8 and give it RTBC.

@daffie would if make sense to change Query/Connection.php to

public function conditionGroupFactory($conjunction = 'AND') {
  return new static($conjunction);
}

Wouldn’t this prevent each class having to implement this function.

Patch, #16 rolled back to 8.8 and using late static binding.

Try late static binding.

@daffie I reviewed your #16 patch, and I thought it looked good. I made my suggested changes and that patch has several failures. However, I retested yours and it also has many failures.

MySQL 5.5 is not supported by D9, moving back to Needs Review.

I made the patch wrong. I needed to run `git diff HEAD`. This is with late static binding.

@daffie I didn’t forget it. Calling `new static()` is the whole point of this. By taking advantage of late static binding, the parent will always return an instance of the child class, and we do not have to implement Condition::conditionGroupFactory() in every driver. If every child must do new self(), you can instead have the parent do new static(). As an extra safety precaution, should the Query\Condition class be marked abstract(Also Delete, Merge, Select, Transaction, Truncate, Update, and Upsert)?

Here's the same concept as #30 on 8.8.x-dev

I tested #16 with SQL Server and it passes my custom condition tests along with the core kernel test suite. I feel #30 is an improvement though. @daffie, could you offer your opinion?

This needs a change record.

Draft change record has been created. The only people I can imagine this would impact are database driver developers. It should be invisible to everybody else.

@alexpott if the Condition class is now a required component of each database driver, should it be an abstract class? What about Delete, Select, Update, etc? Marking them abstract would make it clear to developers that it must be implemented at the driver level, but would require the unit testing suite to stub each of them.

We also need to put this in D8.9. Here's a patch that fixes the conflicts in core/tests/Drupal/Tests/Core/Database/OrderByTest.php

Committed 7a943a3 and pushed to 9.0.x. Thanks!
Committed 9397737 and pushed to 8.9.x. Thanks!

diff --git a/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
index eb4b24dbef..d909f7c50b 100644
--- a/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
@@ -2,7 +2,6 @@
 
 namespace Drupal\Core\Database\Driver\mysql;
 
-use Drupal\Core\Database\Query\Condition;
 use Drupal\Core\Database\SchemaException;
 use Drupal\Core\Database\SchemaObjectExistsException;
 use Drupal\Core\Database\SchemaObjectDoesNotExistException;
diff --git a/core/lib/Drupal/Core/Database/Schema.php b/core/lib/Drupal/Core/Database/Schema.php
index 56317af7e1..1987177a3e 100644
--- a/core/lib/Drupal/Core/Database/Schema.php
+++ b/core/lib/Drupal/Core/Database/Schema.php
@@ -2,7 +2,6 @@
 
 namespace Drupal\Core\Database;
 
-use Drupal\Core\Database\Query\Condition;
 use Drupal\Core\Database\Query\PlaceholderInterface;
 
 /**

Fixed unused uses on commit.

Had to revert this to get a bit more discussion on BC layers.

So \Drupal\Core\Database\Connection::getDriverClass() is not helping us like it should. Unfortunately decisions were taken in #2461239: Database driver class fallback is broken and stubbed instead to require code to duplicated rather than to have it resolve to the base class. This was in an effort to keep thing simple. However that means this change will break every contrib driver because it is missing a condition class. Given that the condition class is just going to be

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Condition.php
@@ -0,0 +1,10 @@
+<?php
+
+namespace Drupal\Core\Database\Driver\mysql;
+
+use Drupal\Core\Database\Query\Condition as QueryCondition;
+
+/**
+ * MySQL implementation of \Drupal\Core\Database\Query\Condition.
+ */
+class Condition extends QueryCondition {}

That feels really really wrong.

So the simplest thing in order to maintain BC and comply with policy is to so something like:

diff --git a/core/lib/Drupal/Core/Database/Connection.php b/core/lib/Drupal/Core/Database/Connection.php
index 691a6ae84c..874037da71 100644
--- a/core/lib/Drupal/Core/Database/Connection.php
+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -2,6 +2,8 @@
 
 namespace Drupal\Core\Database;
 
+use Drupal\Core\Database\Query\Condition;
+
 /**
  * Base Database API class.
  *
@@ -781,6 +783,10 @@ public function getDriverClass($class) {
       }
       $driver_class = $this->connectionOptions['namespace'] . '\\' . $class;
       $this->driverClasses[$class] = class_exists($driver_class) ? $driver_class : $class;
+      if ($this->driverClasses[$class] === 'Condition') {
+        @trigger_error('blah di blah. See change record.', E_USER_DEPRECATED);
+        $this->driverClasses[$class] = Condition::class;
+      }
     }
     return $this->driverClasses[$class];
   }

That way if we can't resolve the Condition class we trigger a deprecation error (only once) and contrib db drivers get to continue. We can even add the @trigger_error in a follow-up for 9.1 so we can commit this to 8.9.x - we would need a test of the fallback though.

@alexpott Most of the classes in the MySQL driver namespace are stubs. The new condition class will just be following precedent.

Is there a way getDriverClass() can be changed which would be backwards compatible, while also not creating a special exception for the Condition class? It would be nice to allow all drivers to drop any unextended Query classes.

@Beakerboy I totally think we should do that I disagree with the reasoning giving in the other issue about not doing that. But that's a follow-up. If we want to get this in D9 (and D8) then the path suggested above is the quickest and scoped correctly for this issue.

New patch with changes suggested by @alexpott

• update getDriverClass
• removed tests for driver Condition classes
• No deprecation warning if the long-term goal is to remove all driver stubs

Edit:
I have no idea why this patch is not applying. It looks right...

OrderByTest.php changed a couple hours before I uploaded, but after I tested. Need to reroll.

Updated patch against latest 9.0 branch

odd...try uploading again.

My use statement was above my namespace declaration.

Edit:
If anyone knows why this is failing, post below. I can’t figure it out. As far as I can see, Condition::class can only refer to one namespace, the Query\Condition from the use statement. Any other case requires that the class actually exists, so there should never be a “Class 'Condition' not found” error.

We need to put the same BC layer in \Drupal\database_statement_monitoring_test\LoggedStatementsTrait::getDriverClass(). We're going to be able to get rid of that in #3115388: PP-1: Deprecate \Drupal\Core\Database\Database::getDatabaseDriverNamespace as it is no longer needed but for now we need to duplicate :( but at least it is only test code.

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -865,6 +867,9 @@ public function getDriverClass($class) {
+      if ($this->driverClasses[$class] === 'Condition') {
+        $this->driverClasses[$class] = Condition::class;
+      }

Need a @todo pointing to a 9.1.x followup to add an @trigger_error.

updating \Drupal\database_statement_monitoring_test\LoggedStatementsTrait::getDriverClass()

Added @todo

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -852,6 +854,8 @@ protected function expandArguments(&$query, &$args) {
+   * @todo https://www.drupal.org/project/drupal/issues/3120036
+   *

@@ -865,6 +869,9 @@ public function getDriverClass($class) {
+      if ($this->driverClasses[$class] === 'Condition') {
+        $this->driverClasses[$class] = Condition::class;
+      }
     }

Here i'd inline the @todo... something like

      if ($this->driverClasses[$class] === 'Condition') {
        // @todo Deprecate the fallback for contrib and custom drivers in 9.1.x
        //   in  https://www.drupal.org/project/drupal/issues/3120036.
        $this->driverClasses[$class] = Condition::class;
      }

Changed with suggestions from @alexpott

@daffie, the tests are missing because this latest version no longer requires all drivers to have a stubbed Connection class. I’ve moved the tests to my driver code to ensure that it returns its custom Condition. I’m sure there’s a way to mock a custom driver with a custom Condition to prove that it is returned when appropriate, but I do not know how to do that.

Hello @daffie
I have added the test.
Please review the patch.

@neelam_wadhwani thank you for your swift assistance. This patch still needs the fix for the whitespace issue referred to in #66. There is also 2 coding standards issues with your patch (https://www.drupal.org/pift-ci-job/1617547)

Also @daffie,
This just tests that the default Condition will be returned. Since the point of this issue is to allow a contrib driver to override the Condition, shouldn’t we test THAT. Maybe something like this:

  /**
   * Test that the core Condition can be overridden.
   */
  public function testContribCondition() {
    $this->mockCondition = $this->getMockBuilder('Drupal\Core\Database\Query\Condition')
      ->setMockClassName('MockCondition')
      ->setConstructorArgs([NULL])
      ->disableOriginalConstructor()
      ->getMock();
    $contrib_namespace = 'Drupal\Driver\Database\mock';
    $mocked_namespace = $contrib_namespace . '\\Condition';
    class_alias('MockCondition', $mocked_namespace);

    $this->options['namespace'] = $mocked_namespace;
    $this->options['prefix']['default'] = '';

    $this->mockPdo = $this->createMock('Drupal\Tests\Core\Database\Stub\StubPDO');

    $connection = new Connection($this->mockPdo, $this->options);
    $condition = $connection->condition();
    $this->assertIdentical($mocked_namespace, get_class($condition));
  }

Hello @Beakerboy
Updated the coding standard.
Please review patch.

Hello @daffie
I have fixed the coding standard issue and attach the other test of @Beakerboy.

KIndly review patch.

Hello @daffie
After adding that test in ConnectionTest.php, it got failed.
So i removed that test and updated the patch.

@neelam_wadhwani My test was just written off the top of my head and typed in on my phone, so I know there would be a couple problems. The error you received can be resolved with an appropriate use statement to the core Connection object. I think it also needs:

-   $this->options['namespace'] = $mocked_namespace;
+   $this->options['namespace'] = $contrib_namespace;

Hello @Beakerboy
I have updated changes.
Kindly review patch.

We cannot instantiate an abstract class (Core\Connection). Mocking it instead. Should this be in Unit Tests instead of Kernel Tests? This new test passes the namespace in with the options parameter. The whole situation would be more realistic if, instead, the connection and the Condition shared the same namespace and the options array did not have a namespace set. Opinions anyone?

• Moved testContribCondition() to unit tests.
• Removed leading slash on namespace in LoggedStatementsTrait.
• Using Class::class instead of hardcoding strings.
• Using getMockForAbstractClass()
• Prevent deprecation warning by stubbing Connection::identifierQuote()

$contrib_namespace is assigned to $options['namespace'], so it is used in two places.

This error is saying that the assertIdentical() method is undefined. I fail to see the problem. There must be a typo somewhere that my brain just is not picking up.

Edit: Unit tests have migrated away from the legacy assertions. All of these should be changed to assertSame().

Using assertSame().

Using the correct class name.

Class alias is necessary.

We need a 8.9.x patch and \Drupal\Tests\Core\Database\OrderByTest no longer needs to change because we fallback now. We might need to change it when we deprecate the fallback but that's okay we handle it then.

Committed and pushed f156b5c6b1 to 9.1.x and e02296ec61 to 9.0.x. Thanks!

Committed 6f339f2 and pushed to 8.9.x. Thanks!

The commit hashes in the comment above lead to 404 pages. Is there a staging area where commits sit before being pushed to GitLab? Maybe the testbot also pulls from GitLab and they are not fully public yet.

@beakerboy @daffie I forgot to push so we're back to rtbc.

Okay I've committed this to 9.1.x. We need a release manager +1 to backport this to 8.9.x and 9.0.x

This looks safe to backport and enables fixing a bug. Removing tag.

Thanks @catch. Committed and pushed e02296ec61 to 9.0.x and 6f339f2 and pushed to 8.9.x. Thanks!