Problem/Motivation
Certain Drupal core functions depend on the existence of core database tables, and thus should not be invoked, either directly or indirectly, from an install profile.
Most of these core functions are found in includes/path.inc, which may be replaced by setting $conf['path_inc'].
Proposed resolution
The install system should supply a includes/path.inc replacement that does not depend on the existence of core database tables, or check drupal_get_bootstrap_phase() before using the database.
Remaining tasks
The two alternative patches are both awaiting review:
- #6: Use a replacement
includes/path.incplugin during installation. - #9: Check
drupal_get_bootstrap_phase()before using the database.
User interface changes
Certain fatal errors during profile installation will no longer be possible.
API changes
Code in $module.install files will no longer have to check the result of drupal_installation_attempted() or drupal_get_bootstrap_phase() before calling includes/path.inc functions.
| Comment | File | Size | Author |
|---|---|---|---|
| #14 | path-bootstrap-1311774-13.patch | 4.13 KB | xjm |
| #9 | path.inc-1311774-9.patch | 4.57 KB | pillarsdotnet |
| #6 | path.null_.inc-1311774-6.patch | 8.88 KB | pillarsdotnet |
| #4 | 1311774-4-path-null-install.patch | 2.04 KB | tstoeckler |
| #1 | path.fake_.inc-1311774-1.patch | 2.25 KB | pillarsdotnet |
Comments
Comment #1
pillarsdotnet commentedInitial patch for discussion and review.
Comment #2
chx commentedNote that I suggested this so flame me instead of pillarsdotnet who rolled it into a patch.
Comment #3
tstoecklerOver in #1082328: Provide a proper no-op cache.inc it was decided to use "DrupalNullCache". Not that I personally care, but I think this should be consistent, so maybe path.null.inc. Ideally, of course, the path system should be turned into an interface, but I don't think that needs to happen in this issue.
Comment #4
tstoecklerWell, not to hold up this issue on a triviality, I rolled it with path.null.inc :)
Comment #5
pillarsdotnet commentedClosed as duplicate: #1149580: Prevent url() from expanding aliases while the installer is running.
Comment #6
pillarsdotnet commentedCopied the real path.inc and then eliminated only the functionality that depends on database queries.
Comment #7
David_Rothstein commentedWhy would we prevent path aliases during installation?
By the time hook_install() implementations are called, Drupal is already fully bootstrapped, and the normal path.inc works fine. That is the vast majority of module/profile code that gets run during the installer, and it wouldn't be good to force a stub path.inc for that.
It seems to me that only stuff like hook_requirements() (described in #1149580: Prevent url() from expanding aliases while the installer is running.) and a few other edge cases have this problem - in other words, code that runs before Drupal is fully bootstrapped. And trying to use path-related functions before Drupal is fully bootstrapped can break your site even outside of the installer. For example, try calling url() from within hook_boot() even on an already-installed site...
So I think this requires a more general solution.
Comment #8
pillarsdotnet commentedI look forward to reviewing your patches.
Comment #9
pillarsdotnet commentedPerhaps this was what you meant by a "more general solution" ?
Comment #9.0
pillarsdotnet commentedfix
Comment #10
David_Rothstein commentedYeah, something like that makes sense to me. It's unfortunate to have to do that (especially since a couple of those functions are in the critical path and we're adding extra function calls to them), but at least as a stopgap fix it looks like it's on the right track, and short of rewriting all of path.inc I can't think of another way to do it.
Comment #11
pillarsdotnet commentedIs there some kind of canonical list of the functions that are "on the critical path" or is that phrase just a matter of opinion?EDIT: According to chx, the critical path is the code ran to serve a cached page.
The drupal.org website really needs a glossary. So many terms have a specific meaning here that is different from the rest of the world.EDIT(2): found it. Added critical path entry.
Comment #12
David_Rothstein commentedHa, well, I thought the phrase was just a matter of opinion :)
I don't think these functions actually run during cached page requests, but on regular pages they can run an awful lot (e.g. every time a link displayed on the page is processed). Overall, it's probably not too big of a deal.
Comment #13
pillarsdotnet commentedProbably needs benchmarks to be sure. Unfortunately I have not yet acquired the relevant (xhprof) skillset.
Comment #14
xjmRerolled for core/.
Comment #15
kscheirer#14: path-bootstrap-1311774-13.patch queued for re-testing.
Comment #17
xjmComment #17.0
xjmTwo alternative solutions.
Comment #26
catchPath aliases have been moved to a regular module, so this is no longer relevant.