Database driver class fallback is broken and stubbed instead [#2461239]

Problem/Motivation

The getDriverClass central method is still trying to use called with the following

Proposed resolution

Make the fallback work again. Remove the unnecessary classes.

Remaining tasks

None.

User interface changes

None.

API changes

None. These classes were never meant to be used directly.

Comment	File	Size	Author
#6	2461239_6.patch	7.41 KB	chx
#6
#4	2461239_4.patch	6.83 KB	chx
#4
#2	driver_fallback.patch	6.7 KB	chx
#2
	driver_fallback.patch	6.47 KB	chx

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

28 March 2015 at 03:46

Status:

Needs review

» Needs work

The last submitted patch, driver_fallback.patch, failed testing.

Comment #2

chx CreditAttribution: chx at MongoDB commented 28 March 2015 at 03:57

Issue summary:	View changes
Status:	Needs work	» Needs review

File	Size
driver_fallback.patch	6.7 KB

1 file was hidden/shown/deleted

File	Size
driver_fallback.patch	6.47 KB

We pass in Install\Tasks but also Select and Select is in Query by core but not in Query per driver so the fallback logic is not flat.

Comment #3

28 March 2015 at 04:14

Status:

Needs review

» Needs work

The last submitted patch, 2: driver_fallback.patch, failed testing.

Comment #4

chx CreditAttribution: chx at MongoDB commented 28 March 2015 at 05:05

Status:

Needs work

» Needs review

File	Size
2461239_4.patch	6.83 KB

1 file was hidden/shown/deleted

File	Size
driver_fallback.patch	6.7 KB

OK... So ->extend('Drupal\Core\Database\Query\PagerSelectExtender') (and similar) was breaking it. After this patch we can change all callers to use ->extend('PagerSelectExtender') if we so want.

Also, here is a fun one, I never realized the driver can override the extender by creating a class called Drupal\drivernamespace\Drupal\Core\Database\Query\PagerSelectExtender. Probably that wasn't the intention but it is certainly so in HEAD and with this patch too.

Comment #5

28 March 2015 at 05:24

Status:

Needs review

» Needs work

The last submitted patch, 4: 2461239_4.patch, failed testing.

Comment #6

chx CreditAttribution: chx at MongoDB commented 28 March 2015 at 05:44

Status:

Needs work

» Needs review

File	Size
2461239_6.patch	7.41 KB

1 file was hidden/shown/deleted

File	Size
2461239_4.patch	6.83 KB

That's just a unit test trying to pick up the now nonexistent class. Better luck with pgsql.

Comment #7

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran commented 28 March 2015 at 12:34

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -659,15 +659,22 @@ protected function expandArguments(&$query, &$args) {
+      do {
...
+      } while (!class_exists($this->driverClasses[$class]) && $namespaces);

hmmm interesting.

Comment #8

Crell CreditAttribution: Crell at Palantir.net commented 30 March 2015 at 22:19

Status:

Needs review

» Needs work

The issue summary is incomplete and breaks off mid-sentence. I think I follow what is being done here from the patch. However, this is a regression. Early in the D8 cycle we switched the child classes to always be required in order to simplify the code. I think I made that change as part of the namespace-ification.

I don't see a reason to go back here. The empty classes hurt no one, and the patch here introduces a while loop into the driver handling that seems completely unnecessary.

I'm inclined to won't-fix.

PS: Yes, driver-specific extender classes are a deliberate design decision. I don't know that they're often used, but I am pretty sure that was deliberate.