So in theory, the database layer will self-load the files it needs really early on and then rely on "some other autoloader" to load the rest of itself. In D7, of course, that's the registry, which is, you guessed it, database dependent. For the most part that does work right now, but it's a bit unstable.

As an example, we keep getting other errors with the installer that manifest themselves as "cannot find class MergeQuery_*" or similar class-not-found errors for DB classes. The lack of the class itself is usually not the issue, but it ends up masking what the underlying issue is. It would be cleaner all around if we just added a DBAPI-specific autoloader that doesn't need the database to work. PHP easily supports that.

Unless Drieschick vetoes this and bumps it to Drupal 8, I'll try to write up a patch shortly. :-)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

Status: Active » Needs review
FileSize
8.42 KB

And patch. Turns out there were some interesting knock-on effects, one of which is that the installer actually gets slightly simpler. :-) There's some oddity involved, but it's all documented inline As God Intended(tm).

Status: Needs review » Needs work

The last submitted patch, 688704-db-autoload.patch, failed testing.

carlos8f’s picture

subscribing

Crell’s picture

FileSize
33.52 KB

Right, I don't quite know what was wrong with the last patch, but lots of code changed since then. Here's what I'm working with now. It lets me install and everything works, but simpletest fails completely as it cannot find DatabaseSchema_mysql. I've no idea why. There's some simpletest debug code still in here. Input welcome.

carlos8f’s picture

This is the next thing on my list to review.

One thing I really don't like is that X_exists() triggers the autoloader, I think that is actually undesirable. It makes more sense for those functions to check the current namespace, to assist you in manually loading the files. But oh well, PHP has lots of oddities like that.

Looks like you've got a bunch of hunks for overlay, locale and shortcut modules, I hope that's not intentional ;)

Crell’s picture

class_exists() checking autoload makes perfect sense, IMO. "Can I instantiate this class? If so, great, if not, do something else." You can instantiate it if you can autoload it, so it tries to autoload.

The extra hunks... that would be my still undeveloped skillz and git merging. *sigh* I'll try to reroll soon.

Crell’s picture

FileSize
12.46 KB

And here's a fresh patch that is rolled against a copy of HEAD that is not over a month old... Still has some debugging lines in it, because it still doesn't work.

Right now, I'm able to install fine but when I run tests I immediately get an error that the registry table doesn't exist... which makes my brain hurt because nothing this patch does should have anything to do with that at all.

boombatower’s picture

Status: Needs work » Needs review
FileSize
13.3 KB

Lets try this one.

If this works, then the key change is to move:

spl_autoload_register('db_autoload');

above

spl_autoload_register('drupal_autoload_class');
spl_autoload_register('drupal_autoload_interface');

Since the db_prefix is changed during install and the registry tables do not exist. Thus the first autoload that is called try to select from registry and that blows up.

boombatower’s picture

#8 kills the modules page thus the error by bot, but it does seem to allow tests to run so I believe my reasoning is write. The solution although is not as clear.

1) (easiest) Pre load all installation files before switching db-prefix.
2) Do some magical registry hax.
3) Pure magic.

Status: Needs review » Needs work

The last submitted patch, 688704-db-autoload.patch, failed testing.

Crell’s picture

Hm. Why would that kill the modules page? And which install files are we preloading, exactly? This patch removes the preloading of DB files and relies on DB autoload, specifically because we want to remove the DB autoloading from the registry.

boombatower’s picture

I haven't looked into the module page issue, but the debugging explains why it failed when run inside SimpleTest. With the patch the db classes are initial checked against the registry (which doesn't exist). So if you trying to prevent that the db_autoload() needs to be called first, otherwise for SimpleTest's case you need to manually load the db files before running install to prevent the registry lookup.

As for the modules page...I'm certain the can be solved, but I'll leave that to your decision/thought.

Crell’s picture

Ah! OK, I think I follow. We're still doing a bootstrap as part of the install, which registers the registry-based autoloaders. Then we add the DB autoloader. But since now the DB classes aren't being force-loaded, it tries to autoload them, hits the registry first, and dies. Kaboom!

Although why that works during manual install I still don't know. At least it's a direction. I will keep investigating. Thanks boombatower!

boombatower’s picture

Because the registry table exists...when run manually. But when run during testing it switches db prefix and table does not exist at that point since it is trying to load the db in order to create the tables. Thus the auto load SELECT query craps.

Crell’s picture

Hm. Yuck. :-)

carlos8f’s picture

I wasn't able to get my PIFR installation up to reproduce the failure, so I started tweaking and going in my own direction.

I think there's a lot of room to simplify the current patch. The objective seems pretty simple to me:

1. Add an autoloader to run before the {registry}-based ones.
2. It includes generic database classes/interfaces, and then classes specific to the driver in use. It simply searches the appropriate dir's for *.inc files.
3. Then returns class/interface_exists().
4. Remove cruft, being careful that the autoloader is available at that part of the code.

Boom?

boombatower’s picture

This issue occurs in SimpleTest...not specific to bot.

Crell’s picture

What you describe in #16 is pretty much exactly what this patch does, with the caveat that autoload callbacks do a require/include to load a file, after which PHP "knows" that the class is now available.

Although actually because we don't know that a class is available, just what file it would be in were it available, it's still possible for us to load a file and then get class_exists() FALSE. For example, if a driver overrides UpdateQuery but not InsertQuery, it would still try loading driver/query.inc if you checked for InsertQuery_driver. That doesn't cause any issues in normal page runs as far as I've found. I have no idea if that would cause issues for simpletest, though.

Anonymous’s picture

subscribe.

Crell’s picture

Priority: Normal » Critical
Status: Needs work » Needs review
FileSize
11.42 KB

OK, let's see if this works...

I am also bumping this to critical as it seems a number of other issues are blocked by it.

Crell’s picture

OMG it's actually working! Quick, someone review it and RTBC before it breaks again! :-)

boombatower’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
12.06 KB

Do you actually want:

-    include_once "{$file->uri}/install.inc";
-    include_once "{$file->uri}/database.inc";
+    //include_once "{$file->uri}/install.inc";
+    //include_once "{$file->uri}/database.inc";

Otherwise looks good and comments read well.

Required re-roll due to core changes in database.inc...I also fixed above item. Marking RTBC pending test results.

Crell’s picture

D'Oh! I knew I'd leave something like that in. Thanks, boombatower!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 688704-db-autoload.patch, failed testing.

boombatower’s picture

Status: Needs work » Needs review

#22: 688704-db-autoload.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 688704-db-autoload.patch, failed testing.

bcn’s picture

FileSize
11.81 KB
+++ includes/database/database.inc	5 Apr 2010 06:19:20 -0000
@@ -741,7 +741,9 @@
+      if (class_exists($class_type)) {
+        $this->schema = new $class_type($this);

$class_type should be just $class no? Attached patch makes only this change...

121 critical left. Go review some!

bcn’s picture

Status: Needs work » Needs review

Meh! And for the bot...

boombatower’s picture

Status: Needs review » Reviewed & tested by the community

Applied and re-patched (to get same order as mine) and diffed....looks good.

Dries’s picture

I don't understand why this is critical. Care to list what issues are blocked by this or to explain why this is critical?

Crell’s picture

Dries: See the original post. Right now, the DB system will break horribly if the registry table is not available and fully populated, because it uses the registry to load all of the query builders. So you can't write to the database or issue built queries or mess with the schema unless the registry table is built.

Of course, there's lots of cases where you need to write to the database when the registry isn't fully initialized: Install, Update, any time anything went wrong during a registry rebuild (menu enable/disable and a few other times), etc. The first two HEAD is manually working around with lots of manual include_once() statements. That doesn't help the other cases, though, and still leaves us with an ugly circular dependency where the DB driver doesn't work unless your DB is already populated with Drupal-specific stuff.

This patch removes that circular dependency and makes the DB system more robust and fault-tolerant. Since there's about 4 issues floating about right now where you can't install or update due to not being able to autoload parts of the DB layer (search for MergeQuery_mysql to find several), including the update from D6 not working at all, I believe this warrants a critical tag and should be fixed post-haste.

marcvangend’s picture

Dries: it also fixes the bug that update.php produces a fatal error when Views is enabled (see #646456: Class 'views_object' not found when running update.php).

Crell’s picture

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Finally had the time to look at this in more depth, and it looks great. Committed to CVS. Great work. :)

andypost’s picture

Status: Fixed » Needs work

Suppose there should be a follow up patch to clean

+  spl_autoload_register('db_autoload');
   spl_autoload_register('drupal_autoload_class');
   spl_autoload_register('drupal_autoload_interface');

Why this function is named different? "drupal_autoload_db" semm more reasonable

+  	// Since class_exists() will likely trigger an autoload lookup,
+  	// we do the fast check first.
+    if ($value === 1 && class_exists($class_name)) {

Indent is wrong

 function _registry_check_code($type, $name = NULL) {
   static $lookup_cache, $cache_update_needed;
-
   if ($type == 'class' && class_exists($name) || $type == 'interface' && interface_exists($name)) {

seems like typo

andypost’s picture

Status: Needs work » Needs review
FileSize
5.72 KB

patch

Crell’s picture

Status: Needs review » Fixed

No, that's an unnecessary cleanup. The DB layer uses a db_ prefix, not a drupal_ prefix, because it is, as much as humanly possible, not Drupal-specific. The db_autoload() function has no Drupal dependencies at all, so should not be pulled into the drupal pseudo-namespace.

andypost’s picture

Status: Fixed » Needs review
FileSize
1.46 KB

Patch to fix typos

Crell’s picture

Status: Needs review » Reviewed & tested by the community

No code changes, just whitespace, so I can't see the bot objecting.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

chx’s picture

Status: Fixed » Needs work
+    // Drivers have an extra file, and may put their SelectQuery implementation
+    // in the main query file since it's so small.
+    $driver_files = $files;
+    $driver_files['query.inc'][] = 'SelectQuery';
+    $driver_files['install.inc'] = array('DatabaseTasks');
+
+    foreach ($driver_files as $file => $classes) {
+      if (in_array($base, $classes)) {
+        require_once "{$base_path}/{$driver}/{$file}";
+        return;
+      }
+    }

is this contrib or what? "may put"... beh! it belongs to select.inc, period. {$base_path}/{$driver}/{$file} why the {} just $base_path/$driver/$file

catch’s picture

Priority: Critical » Normal

Demoting from critical queue now this is just cleanup.

Crell’s picture

Status: Needs work » Fixed

"May put" because only one driver overrides select right now, and it puts it in query.inc because it's really really small. There's no need to force an extra disk hit for it, especially when it turns out that every page is using a built query anyway. That's not something for this issue, as it's just documenting what core already does.

And the extra {} make the string easier to read and parse visually. It's a good habit to get into for more readable code.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Damien Tournoud’s picture

    if (empty($base_path)) {
      $base_path = dirname(realpath(__FILE__));
    }

What's the reasoning behind the realpath() here? Sounds like a useless syscall for me.