Allow db drivers outside of 'core'

Critical IMO. We can't force oracle and mssql to require core patches.

Well, its not really a patch, but it is requiring modules to put their code inside of /includes which is just as bad as a core patch.

I don't know what Damien is talking about by us promising that new drivers could be modules. We said they could be added without hacking core, which is already true.

Moshe, please don't mark new issues critical without verifying that what you're saying is even true. It's not "as bad as a core patch". It's no worse than what we have to do for install profiles. Drivers are not modules; we just lack a "DB Driver" project type in project.module.

This is also a duplicate anyway; We closed a previous issue on this subject months ago.

Install profiles don't require you to copy code into a core directory since they are already packaged that way. So its a bit different IMO. I'm not going argue about priority. Normal is fine. I do think we should endeavour to fix this. I'll see if I can find that original discussion.

#778310: Update status for db drivers is missing has been marked as a duplicate by Damien, but isn't really.

This is a work in progress but something is happening. I came up with a directory name 'db_drivers' to be put alongside modules and themes in the Drupal root, this is not something I plan to pre-create unlike modules and themes. The screenshot is from an earlier version where i called it 'database', nonetheless, it shows the install screen working.

This is it. You can put a db_driver in a db_drivers directory in Drupal root. It needs to implement a function isCore() { return FALSE; } in the Install\Tasks.php file and the rest is taken care of: the $database array gains a db_driver key which is taken into consideration in openConnection and getDriverClass.

I took great pains to make sure the effects are extremely small, the only file operations added are in install.

The only question is, should I rename db_driver to drupal_driver?

Assigning to Crell for a review.

Ps. getDriverClass is broken 'cos if $driver_class doesn't exist it will try to instantiate a namespace-less class. Larry, should we declare the base classes abstract to mandate their overriding and simplify this function or fix this function? I know it's a separate issue but I didn't want to push two issues to you :)

Let's do this instead. This uses function isCustom { return TRUE;} and writes accordingly into settings.php. It's a lot cleaner.

Updated issue summary.

To test, you can move the mysql driver from the core directory under db_drivers/test/lib/Drupal/test/ and apply patch. As you can see, aside from the namespace changes, only Connection::driver and the Tasks needed changes.

Updated issue summary.

Here's one that peeks at the Tasks classname during install to figure out whether it's a core driver and gets rid of the Tasks method. Less code.

dawehner says, strpos would better. And I have changed the string to test to Drupal\Core\Database just in the case some enterprising developer names a driver "Core" (although that would be a particularly stupid idea).

+++ b/core/includes/install.incundefined
@@ -139,6 +139,16 @@ function drupal_get_database_types() {
+  if (is_dir(DRUPAL_ROOT . '/db_drivers')) {

I think it's okay to not document here how to create a custom db driver, as that's an advance topic, which could be maybe written somewhere else.

All these code lines looks fine, but yeah in a perfect world you would have a fully decoupled system, where you don't need to distinct between core and custom drivers, but this patch is already a huge improvement.

Documentation wise, we (do not) have a page at http://drupal.org/node/310087 which I will amend.

Also, Damien and me I already are on the issue and I already emailed Andrea Gariboldi (and David Strauss for good measure). Also, I will contact the DB2 and the Firebird people should this go in.

Updated issue summary.

+++ b/core/lib/Drupal/Core/Database/Database.php
@@ -374,7 +374,13 @@ public static function addConnectionInfo($key, $target, $info) {
+      drupal_classloader_register($driver, "db_drivers/$driver");
+      $driver_class = "Drupal\\{$driver}\\Connection";

This looks mostly reasonable, aside from this line as we cannot hard code Drupal functions into the Database core classes.

Instead, I wonder if we couldn't specify a class prefix (namespaces) in the database definition in settings.php, and then just rely on the autoloader on its own? If you're setting up a 3rd party driver, it should be expected that you need to take care of its autoloading. Consider, eg, if 3rd party drivers were brought in via composer. Then all you need to define is the name prefix the classes, use, which don't even have to be in the Drupal namespace technically.

> as we cannot hard code Drupal functions into the Database core classes.

And what part of that is not possible? Are you still chasing the "DBTNG as an independent component' dream when the Drupal namespace is hardwired into HEAD and indeed without this patch, the Drupal Core namespace is?

Or we can't call functions from a class? Why not? Did someone from a burning bush hand you a stone table with this etched into it?

Here's a compromise: let's add a @todo that once the Database is moved into the DIC , we inject the classloader. We can't really do that now (or if we did it'd be superb ugly).

> Consider, eg, if 3rd party drivers were brought in via composer

If they are to work with Drupal they need to be in a certain namespace which we hardwire right in this patch -- before it was hardwired to Drupal core. So, if the namespace is already hardwired, why (and how?) would they take of autoloading? Manually edit settings.php? What for? That's the biggest virtue of this patch: you dont need to edit settings.php, instead install.php works.

Please reconsider your veto on this.

Injecting the classloader is ugly, too. The database manager shouldn't have to think about autoloading. That's not its job.

What if we put the /db_drivers/ check in where we setup the classloader? If it exists, register it as a possible source for \Drupal\Core\Database\Driver classes. That's one file_exists() check, but in the grand scheme of things there are far bigger performance fish to fry. Then the DB manager can just use the name of the class as normal (keeping the magic naming just as we do now), and the autoloader can do what the autoloader is good at: Load classes.

Also, Re #8, I'd be fine with the base classes being abstract if the language would let us. But I don't think you can make a class abstract unless it has at least one unimplemented method, which we have no need for as most driver-provided classes don't need to override anything. So, meh. If $driver_class doesn't exist, that's a broken state anyway. We either let it fail hard, or we catch it and throw an exception. I'm fine either way.

> The database manager shouldn't have to think about autoloading. That's not its job.

In theory perhaps. Practice shows otherwise. It shouldn't hardwire namespace either, for that matter.

> What if we put the /db_drivers/ check in where we setup the classloader?

So the database manager can't know about the classloader (while it at least acknowledges its existence -- in D7 Database was loading classes) but the classloader should know about database? Isn't this completely backwards?

> What if we put the /db_drivers/ check in where we setup the classloader?

Absolutely not. We are not adding file checks to the critical path. Unlike this-is-not-nice-in-theory, imagine a network filesystem with apc.stat 0. You just killed the site's performance.

So, I do not really like this solution but your stubborn theorist stance does not let me come up with anything nicer. I would much appreciate you accepting #12 instead. Considering that Database practically uses globals (even if it's just class statics, it's not really different) I am amazed by your resistance.

Well, enough of this. I know that dawehner only didn't RTBC this because we wanted Larry to see this and he said "This looks mostly reasonable, aside from this line".

Dries, I ask you to decide whether Larry is is right and preferably commit #12 (or #18 if you really must). While #12 indeed adds a call outside of the DBTNG directory this should be no problem -- just do not add a custom => TRUE to your config, done. If the time comes to separate DBTNG out, adding a function_exists there won't be a problem. There is, or should be, a reason of why we have a Drupal\Core and a Drupal\Component directory and as of right now Database is still in Core.

As I stated in the past: I find the double measure of Drupal 8 absolutely unfair where people can freely add any hundreds or thousands of code that does not fit Drupal in any way or form (and, ironically then I work hard to make it fit somehow). and I can't fix anything if it does not fit their agenda. Well, I try really hard not to complain any more for the sake of progress. Can we cut a one line slack for me -- once again, in the name of progress?

Assuming this is the bit in question.

-      $driver_class = "Drupal\\Core\\Database\\Driver\\{$driver}\\{$class}";
+      if (!empty($this->connectionOptions['custom'])) {

Why can't the driver be referenced by the full namespace (either in settings.php or wherever before it gets here), then there's no hard coding of Drupal\Core anyway?

This is the result of an extremely fruitful conversation with catch. Small, pretty and extremely powerful. You can now put memcache and whatnot in 'drivers/lib'.

Updated issue summary.

So the top dir wants to be 'drivers' as in moduleS, themeS, siteS but the lib directory is Component not ComponentS so there.

I'm ok in principle but it seems like the code could be cleaned up a bit.

Specifically, I'm ok with adding a 'drivers' directory -- not sure I'm fond of 'db_drivers'. We've kept the Drupal root free of underscores, which is kinda nice.

+++ b/core/includes/install.core.incundefined
@@ -1018,6 +1018,10 @@ function install_settings_form($form, &$form_state, &$install_state) {
+  $drivers = drupal_get_database_types();
+  if (strpos(get_class($drivers[$driver]), 'Drupal\\Core\\Database') !== 0) {
+    $database['custom'] = TRUE;

Should we call this function 'drupal_get_database_drivers()' instead? Seems weird to request 'types' and to call the result 'drivers'.

+++ b/core/includes/install.core.incundefined
@@ -1090,8 +1094,13 @@ function install_settings_form_submit($form, &$form_state) {
+  $extras = '';
+  if (!empty($form_state['storage']['database']['custom'])) {
+    $driver = $form_state['storage']['database']['driver'];
+    $extras = "drupal_classloader_register('$driver', \"db_drivers/$driver\");\n";
+  }
 
-  drupal_rewrite_settings($settings);

Wouldn't this make more sense to register the classloader where we already register other classloaders?

+++ b/core/includes/install.incundefined
@@ -139,6 +139,16 @@ function drupal_get_database_types() {
+  if (is_dir(DRUPAL_ROOT . '/db_drivers')) {
+    $files = file_scan_directory(DRUPAL_ROOT . '/db_drivers', '/^[a-z_]+$/i', array('recurse' => FALSE));
+    foreach ($files as $uri => $file) {
+      $file->uri .= "/lib/Drupal/$file->filename";
+      if (file_exists($file->uri . '/Install/Tasks.php')) {
+        $drivers[$file->filename] = $file->uri;
+        drupal_classloader_register($file->filename, "db_drivers/$file->filename");
+      }
+    }

I'm not sure I like this. Could you add some code comments?

I'd like to take a stab at a cleanup.

+++ b/core/includes/bootstrap.incundefined
@@ -3166,7 +3166,7 @@
+      'Drupal\Driver' => DRUPAL_ROOT . '/drivers/lib',

It's good to be consistent here with both the namespace and the folder!

+++ b/core/includes/install.incundefined
@@ -134,9 +134,11 @@
+  $mask = '/^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*$/';

Maybe it would be just be helpful to put http://www.php.net/manual/en/language.variables.basics.php as reference.

+++ b/core/lib/Drupal/Core/Database/Connection.phpundefined
@@ -624,6 +624,7 @@
+        // Fallback for Drupal 7 settings.php.

+++ b/core/lib/Drupal/Core/Database/Database.phpundefined
@@ -378,6 +378,7 @@
+      // Fallback for Drupal 7 settings.php.

Thanks for adding the comment.

sorry this wasn't indented.

I think Dries reviewed some old patch cos #22 already addressed everything in #23 :D

#22 does pretty much what I was thinking. I might want to review this again (I'm not sure about the reflection class), but no need for it to be assigned to me.

ack. too much xposting.

Hold it for a minute, talking to Damien, will post a new one in a minute.

OK we discussed at length and agreed on actually not changing anything. This just adds a comment to link to the PHP docs. Yes there are warts: we do "drivers/lib/Drupal/Driver/Database/*" while elsewhere we would have "drivers/*/lib/Drupal/Driver/Database" but resolving that would be a rather complicated process -- we would need to register the drivers/* roots for this which would require another settings.php global or even worse drupal_classloader_register calls in settings.php. Neither is a good idea for such an edge case.

#33 looks good by me.

Looks like it's been committed: http://drupalcode.org/project/drupal.git/commit/466fac1.

Change notice is at http://drupal.org/node/1918778

Updated issue summary.

Follow-up patch #2151241: Database Log does not support drivers outside core to fix the rest