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. :-)
Comment | File | Size | Author |
---|---|---|---|
#38 | 688704-db-auto-d7.patch | 1.46 KB | andypost |
#36 | 688704-db-auto-d7.patch | 5.72 KB | andypost |
#27 | db-autoload.patch | 11.81 KB | bcn |
#22 | 688704-db-autoload.patch | 12.06 KB | boombatower |
#20 | 688704-db-autoload.patch | 11.42 KB | Crell |
Comments
Comment #1
Crell CreditAttribution: Crell commentedAnd 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).
Comment #3
carlos8f CreditAttribution: carlos8f commentedsubscribing
Comment #4
Crell CreditAttribution: Crell commentedRight, 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.
Comment #5
carlos8f CreditAttribution: carlos8f commentedThis 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 ;)
Comment #6
Crell CreditAttribution: Crell commentedclass_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.
Comment #7
Crell CreditAttribution: Crell commentedAnd 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.
Comment #8
boombatower CreditAttribution: boombatower commentedLets try this one.
If this works, then the key change is to move:
above
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.
Comment #9
boombatower CreditAttribution: boombatower commented#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.
Comment #11
Crell CreditAttribution: Crell commentedHm. 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.
Comment #12
boombatower CreditAttribution: boombatower commentedI 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.
Comment #13
Crell CreditAttribution: Crell commentedAh! 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!
Comment #14
boombatower CreditAttribution: boombatower commentedBecause 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.
Comment #15
Crell CreditAttribution: Crell commentedHm. Yuck. :-)
Comment #16
carlos8f CreditAttribution: carlos8f commentedI 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?
Comment #17
boombatower CreditAttribution: boombatower commentedThis issue occurs in SimpleTest...not specific to bot.
Comment #18
Crell CreditAttribution: Crell commentedWhat 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.
Comment #19
Anonymous (not verified) CreditAttribution: Anonymous commentedsubscribe.
Comment #20
Crell CreditAttribution: Crell commentedOK, 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.
Comment #21
Crell CreditAttribution: Crell commentedOMG it's actually working! Quick, someone review it and RTBC before it breaks again! :-)
Comment #22
boombatower CreditAttribution: boombatower commentedDo you actually want:
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.
Comment #23
Crell CreditAttribution: Crell commentedD'Oh! I knew I'd leave something like that in. Thanks, boombatower!
Comment #25
boombatower CreditAttribution: boombatower commented#22: 688704-db-autoload.patch queued for re-testing.
Comment #27
bcn CreditAttribution: bcn commented$class_type should be just $class no? Attached patch makes only this change...
121 critical left. Go review some!
Comment #28
bcn CreditAttribution: bcn commentedMeh! And for the bot...
Comment #29
boombatower CreditAttribution: boombatower commentedApplied and re-patched (to get same order as mine) and diffed....looks good.
Comment #30
Dries CreditAttribution: Dries commentedI don't understand why this is critical. Care to list what issues are blocked by this or to explain why this is critical?
Comment #31
Crell CreditAttribution: Crell commentedDries: 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.
Comment #32
marcvangendDries: 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).
Comment #33
Crell CreditAttribution: Crell commentedPatches this one is blocking, both critical:
#757214: D6->D7 update.php failure: Fatal error: Class 'MergeQuery_mysql'
#715108: Make Merge queries more consistent and robust
Comment #34
Dries CreditAttribution: Dries commentedFinally had the time to look at this in more depth, and it looks great. Committed to CVS. Great work. :)
Comment #35
andypostSuppose there should be a follow up patch to clean
Why this function is named different? "drupal_autoload_db" semm more reasonable
Indent is wrong
seems like typo
Comment #36
andypostpatch
Comment #37
Crell CreditAttribution: Crell commentedNo, 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.
Comment #38
andypostPatch to fix typos
Comment #39
Crell CreditAttribution: Crell commentedNo code changes, just whitespace, so I can't see the bot objecting.
Comment #40
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #41
chx CreditAttribution: chx commentedis this contrib or what? "may put"... beh! it belongs to select.inc, period.
{$base_path}/{$driver}/{$file}
why the {} just$base_path/$driver/$file
Comment #42
catchDemoting from critical queue now this is just cleanup.
Comment #43
Crell CreditAttribution: Crell commented"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.
Comment #45
Damien Tournoud CreditAttribution: Damien Tournoud commentedWhat's the reasoning behind the realpath() here? Sounds like a useless syscall for me.