Make the database autoloading more robust

True enough. Probably the easiest approach is to simply have the registry rebuild process skip includes/database entirely, and then copy the list of potential classes from the DB autoloader and have the registry autoloader just return if it sees one of those.

Any chance one of you can write the patch for this? Its a blocker for mssql driver.

This code hurts my eyes. Can we put all statics on top of the file, separate classes from interfaces and have something like

$base_name = strtok($type, '_');
if ($type == 'class' && (class_exist($name) || isset($database_classes[$base_name])))  {
  return TRUE;
}
if ($type == 'interface' && (interface_exist($name) || isset($database_interfaces[$base_name])))  {
  return TRUE;
}

Actually we probably shouldn't be separating interfaces and classes at all for the autoloader; that's my fault, really. Separate issue, though.

I don't quite get what the strtok() is for here, though.

($type == 'class' || $type == 'interface')

This is a tautology. There is nothing else that can be autoloaded so it will always be true.

Otherwise this looks OK to me.

You know, we really need to stop using that pattern. A parameter has meaning. We shouldn't have to guess that meaning from another parameter. That's fundamentally bad design.

But that's to be fixed another time. Let's do.

+    'QueryAlterableInterface' => TRUE,
+    'QueryAlterableInterface' => TRUE, 'SelectQueryInterface' => TRUE, 'SelectQuery' => TRUE, 'SelectQueryExtender' => TRUE,

Any reason to duplicate 'QueryAlterableInterface'

Nope, none whatsoever.

+++ includes/bootstrap.inc
@@ -2698,6 +2698,24 @@ function _registry_check_code($type, $name = NULL) {
+    'QueryConditionInterface' => TRUE, 'DatabaseCondition' => TRUE,
+    'Query' => TRUE, 'DeleteQuery' => TRUE, 'InsertQuery' => TRUE, 'UpdateQuery' => TRUE, 'MergeQuery' => TRUE, 'TruncateQuery' => TRUE,
+    'QueryAlterableInterface' => TRUE, 'SelectQueryInterface' => TRUE, 'SelectQuery' => TRUE, 'SelectQueryExtender' => TRUE,

Can we write those like all the other array elements on separate lines, please?

+++ includes/registry.inc
@@ -53,6 +53,9 @@ function _registry_update() {
+    if (preg_match('@^includes/database/@', $filename)) {

This looks like a simple strpos() === 0 to me; doesn't require a slow preg_match(). Also, can we add a short inline comment here?

Powered by Dreditor.

Lucky #13...

We just spent like thirty minutes trying to fill me in why is this patch necessary and why no other way would suffice.

Here is an an example which is sorely missing from the whole issue: let's try to run a db_delete very early. DeleteQuery_driver is queried for. _registry_check_code runs a cache_get. That, in core, runs ... a db_delete in the garbageCollection called from the get method. Opsie! Another issue (this is what Damien got) the database layer tries to load DatabaseTransaction_sqlsrv, which causes _registry_check_code() to be called early during the installation, even before the {registry} table has been created.

What the patch misses are comments. Nowhere does the patch say why it needs to bug out early. Nor does it have a @TODO for creating a proper fix in Drupal 8 where the autoloaders communicate with each other about which classes they deal with (that's something that PHP should do but I digress).

No changes from #13 except comments. I did not add a @todo because I do not understand how autoloaders could even communicate with each other in anything resembling a performant way without support from PHP itself. Let's not hold up a major patch on that discussion.

Larry and chx and I talked about this issue.

This seems like a really weird pattern to introduce, because it requires us to change things in two places (and we'll likely forget about this second place) when we extend the db system further. Also, one of the goals in D8 is to make DBTNG its own standalone library. This feels akin to us itemizing every single function in jQuery or something like that. Larry acknowledged that although autoloading was originally intended to reduce the amount of code loaded, that in practice we load the whole thing anyway, so this doesn't buy us much. (This would probably make Rasmus happy, too - Crell ;)).

chx pointed out that there are only single points of entry for DBTNG, so doing require_once in a central place should solve the problem.

I am only 98% happy with this because of the require_once statements on top of select.inc and schema.inc but can't help those really. In fact if you remove the one from schema.inc Drupal won't install (but the tests will still pass). Other from that, I think this is nice. The actual line of code change is 34 LoC removed.

OK I have no idea, whatsoever, why we added db_autoload in drupal_web_test_case.php DrupalUnitTestCase... but as it does not exist any more, surely it's not needed now :D that's the cause of all these failures.

db_load_file(): We don't need another function. Really. I moved this into the Database class. That's still ugly but at least it's less ugly.

The $use_registry flag is ugly, hideous, horrific, and I cannot think of an alternative at the moment. :-( At least we can generalize it a little bit and say "use autoload", since we could theoretically be using a non-registry autoload system. I have made that change.

The OS and PHP are smart enough to handle require_once() and file_exists() caching smarter than we can. It's 2010. OSes are smart these days. :-)

Also corrected a documentation typo.

Otherwise, I am OK with this approach.

Whether you call it db_load_file or Database::loadDriverFile I do not care this close to RC.

Committed to CVS HEAD. Thanks.

This patch seems to have broken SQLite installations. Dries discovered this while testing out another patch (and then I reproduced it on fresh HEAD also):

http://drupal.org/node/346494#comment-3760516

The error occurs after you submit the database configuration form:

Fatal error: Class 'SelectQuery' not found in /includes/database/sqlite/query.inc on line 17

subscribing

Using 7-x dev dated 11/27 timestamp 17:11 generates a wsod when sqlite chosen. No error messages. The database contains one entry under Master Table: sqlite_master, which is empty. Using OS X 10.6.5. Found following system error log:
PHP Fatal error: Class 'SelectQuery' not found in /Users/rocks/Sites/dru1127/includes/database/sqlite/query.inc on line 17, referer: http://localhost/~rocks/dru1127/install.php?profile=standard&locale=en

I am not sure about this, but is the file 'select.inc' actually available to the line:

$class = $this->getDriverClass('SelectQuery', array('query.inc', 'select.inc'));

Is the current path 'includes/database' or 'includes/database/sqlite'?

Does postgresql also fail? I can't test.

Rolled back #22 until we figure this out. Downgrading from critical.

And let's make sure this doesn't get bumped back to RTBC until it's tested on sqlite and pgsql, hmmmm? ;)

The proper solution is to move SelectQuery_* to select.inc. I always had some grumbles about the discoverability of DBTNG and this really did not help. Now is the last chance to move those classes to their respective files. And if the sqlite select.inc is small so what? install.inc is not big either and yet noone wanted to enroll it into anything else.

While #22 broke stuff, there is a trivial -and pretty fundamental- patch in the NR queue that would not work without it: #978144: cache_get_multiple() inconsistent with cache_get() would end in Fatal error: Class 'SelectQuery_mysql' not found in C:\xampplite\htdocs\drupal\includes\database\database.inc on line 685.

Installed 7-x dev dated 11/28 with timestamp 12:08 on OS X 10.6.5 and using SQLITE with no errors. Applied patch from #31 to fresh copy of same 7-x dev. Again no errors. Enabled some modules, added a user, switched themes, but that's all. Can't test postgres and really can't check what the patch is supposed to accomplish but it no longer WSOD's when installing new drupal with SQLITE.

Well bah and double bah.

For those playing at home, the issue here is that the loadDriverFile() method loads the common version of whatever file is being requested, followed by the driver-specific version if it exists. It does that for each file in turn. That results in the following order for SQLite/Postgres:

query.inc
sqlite/query.inc
select.inc
sqlite/select.inc

Unfortunately sqlite/query.inc contains SelectQuery_sqlite, which extends SelectQuery, which is in select.inc, which is not loaded yet. Fail. There is no SelectQuery_mysql class, so MySQL never had this problem.

The reason we're putting driver select classes into query.inc is for IO performance. Hitting disk again for a very small file is extra cost we don't need, especially when it turns out in practice that the code there will always be needed. Of course... we have no hard data on what the cost of that extra disk hit is on a modern OS. At least I don't.

So that leaves us with 2 options:

1) Force driver select classes to be in select.inc, and thereby eliminate the catch-22 above. That's what chx's patch in #31 does.

2) Reverse the loop in loadDriverFile() so that we instead load query.inc, select.inc, driver/query.inc, driver/select.inc.

Lacking any hard data as above at the moment, I am OK with #31 at this point. I don't like it, but I don't have more than an intuition that it would be a performance hit. If someone wants to implement #2 I'm OK with that too, but we already have a patch in #31 that resolves the issue (per comments above).

Note that this "loading small files I/O hit" has been proved to be false about the time we added phptemplate to core.

Correction. As chx just pointed out to me in IRC, option 2 would still fail when trying to load, say, InsertQuery_sqlite for the same reason. So that leaves #31 as the only viable option. Per #33 this now works.

Asked to clarify #35, these small files (exactly because they are small) will sit on the OS filesystem cache practically forever making the I/O hit a memory hit. That's pretty close to be not measurable.

And can I get confirmation that someone tested this on SQLite and postgreSQL?

I have installed postgresql using the instructions in https://help.ubuntu.com/community/Drupal (worked perfectly, one 8.1 needed to be changed to 8.4) applied the patch above and installed Drupal. That worked.

#33 tested on SQLite but I tested too when posted the patch.

And for extra good measure, I tested on pgsql 9.0.1 on OS X too.

I'm now finally satisfied that this doesn't break anything. :) Committed to HEAD. Thanks!

The new files for sqllite and pgsql from #31 are not committed...

Oh son of a monkey.

This will teach me to say stuff like "I'm now finally satisfied that this doesn't break anything. :)" Hahahaha!

Committed to HEAD.

Ahum... I should have said "not added"? ;-)

*&@#!

I fail at life today.

OK, they're definitely there now. :)