Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
When initializing Drupal (in index.php, core/update.php, etc), we manually define DRUPAL_ROOT
and chdir()
to it. This adds bloat to index.php when it really doesn't need to be there.
Proposed resolution
Move DRUPAL_ROOT
to bootstrap.inc, by using the __DIR__ constant, new to PHP 5.3.
Remaining tasks
Get the patch RTBC.
User interface changes
No interface changes.
API changes
When bootstrapping Drupal, instead of chdir()ing to the Drupal root and defining DRUPAL_ROOT, you just have to include bootstrap.inc
. It defines the constant for you there.
Comment | File | Size | Author |
---|---|---|---|
#39 | 1540136.patch | 7.21 KB | RobLoach |
#29 | 1540136.patch | 7.68 KB | RobLoach |
#28 | 1540136.patch | 7.61 KB | RobLoach |
#26 | simplify-index-remove-drupal-root-1540136-26.patch | 6.38 KB | Pete B |
#18 | 1540136.patch | 7.6 KB | RobLoach |
Comments
Comment #1
RobLoachLet's move DRUPAL_ROOT definition to bootstrap.inc instead! Thanks sun.
Comment #2
RobLoachGetting "Fatal error: Call to undefined function user_access() in /var/www/drupal/8/core/includes/menu.inc on line 627". When the menu system includes its files, it may not be resolving relative paths. Hmmm.
Comment #4
RobLoachLet's use set_include_path() instead of gross
chdir()
calls.Comment #6
gpk CreditAttribution: gpk commentedI have a feeling we used __DIR__ for a while but it was changed to getcwd() for some reason... Will have a look if I have a moment.
Comment #7
gpk CreditAttribution: gpk commentedOne of the trails starts here I think: #376091: Add DRUPAL_ROOT to drupal_get_path() but not sure if I've found the one where getcwd came in...
Comment #8
RobLoachWith #22336: Move all core Drupal files under a /core folder to improve usability and upgrades, we needed to chdir() so that getcwd() would report the correct path. Not susre why we'd want to use chdir() and getcwd() instead of __DIR__ and set_include_path() though.
Comment #9
sunRelying on the include_path potentially means multiple filestats per included file. I disagree with that direction.
Having a constant/variable that defines the context/application root directory is actually nothing uncommon or special in general; even more so if you look outside of PHP. I'm not sure why we want to do this change.
Comment #10
RobLoachI agree that set_include_path() is probably not the right direction. Maybe simply moving DRUPAL_ROOT definition to bootstrap.inc is better. Simplify the Drupal bootstrap process.
Comment #11
Crell CreditAttribution: Crell commentedRemember that a constant also is an environmental global dependency, and therefore makes things less separable and testable. To be fair, though, the number of places we need to even care about such things should, in theory, go down as we move more toward injected PSR-0 classes and DI.
Comment #12
RobLoach#10: 1540136.patch queued for re-testing.
Comment #14
RobLoachComment #15
sunI'm still not really sure/convinced of this.
Regardless of that:
For clarity, it would help to revert the subsequent $root changes after including bootstrap.inc to DRUPAL_ROOT.
Comment #16
RobLoachIt does make manually loading up the bootstrap easier as you don't have to define DRUPAL_ROOT yourself.
Comment #16.0
RobLoachUpdated issue summary.
Comment #17
RobLoachSimplifies some of the includes.
Comment #18
RobLoachMoved the directory change to
drupal_environment_initialize()
.Comment #19
RobLoachComment #19.0
RobLoachUpdated issue summary.
Comment #20
RobLoachComment #21
RobLoachAdd the PHP 5.3 tag since __DIR__ is one of the new constants added to 5.3.
Comment #22
webflo CreditAttribution: webflo commentedRelated or duplicate issue: #1792310: Wrong DRUPAL_ROOT with non-standard code structure
Comment #23
sunThat DRUPAL_ROOT symlink problem is interesting. I'm not sure whether this patch here helps in any way with that, or possibly makes it even worse?
We should handle the __DIR__ constants in a dedicated issue, since that is a separate task/discussion of its own and only adds noise here:
#1827448: Use __DIR__ instead of DRUPAL_ROOT where possible/sensible
Comment #24
Carsten Müller CreditAttribution: Carsten Müller commentedworks for me including a setup containing symlinks for the core and contrib files
I think this issue also solves #1792310 - install.php has wrong DRUPAL_ROOT when core directory is a symbolic link
Comment #25
Jaza CreditAttribution: Jaza commentedRelated issue: #1928072: Bootstrapping Drupal from outside the Drupal root directory
Comment #26
Pete B CreditAttribution: Pete B commentedRerolling for latest core.
Comment #28
RobLoachComment #29
RobLoachamateescu in IRC asked for clarification on the dirname and __DIR__ usage. neclimdul helped with adding an additional helpful comment.
Comment #30
amateescu CreditAttribution: amateescu commentedYeah, the comment about double
dirname()
usage is helpful. Overall, this looks like a really nice cleanup.Comment #31
sun+1
I'm a bit concerned about performing the
chdir()
indrupal_environment_initialize()
, which seems to be a rather magical position that is deeply buried in the code and thus not really obvious. I think I would have expected thechdir()
to be contained in bootstrap.inc directly, right after the constant definition.Speaking of the constant definition, I think we should move that to the very top of the bootstrap.inc file.
With these two tweaks, it would be immediately clear to everyone what including bootstrap.inc means: It defines DRUPAL_ROOT and changes the working directory.
We can adjust that in a follow-up patch/issue, but I also wouldn't mind if we'd adjust it before commit.
Comment #32
Crell CreditAttribution: Crell commentedWait, what? Changing the working directory by including a file? That's a bad practice. Files should declare symbols or do things, but not both.
Comment #33
RobLoachLet's leave the
chdir()
discussion to #1792310: Wrong DRUPAL_ROOT with non-standard code structure or a different follow up. This is simply about cleaning up index.php, update.php, statistics.php, etc.Comment #34
RobLoach#29: 1540136.patch queued for re-testing.
Comment #36
RobLoach#29: 1540136.patch queued for re-testing.
Comment #37
RobLoachBack to RTBC.
Comment #38
moshe weitzman CreditAttribution: moshe weitzman commentedWe shouldn't do the chdir() in bootstrap.inc. That's seriosly messing with CLI tools and anything that uses Drupal but does not go through index.php. The reply in #33 lacks substance.
I think moving DRUPAL_ROOT is reasonable but not the chdir(). Guess we'd need to refactor in order to move the constant but keep the chdir() in index.php.
Comment #39
RobLoachThanks for the review, Moshe. The attached patch doesn't change any of the chdir's.
Comment #40
moshe weitzman CreditAttribution: moshe weitzman commentedLooks fine.
Comment #41
webchickThis looks like good clean-up, and I like using PHP constants rather than Drupalisms (at least in the base foo.php files ;)).
Committed and pushed to 8.x. Thanks!
Comment #42
webchickMmm. Actually this could probably use a change notice for providers of foo.php scripts.
Comment #43
RobLoachhttp://drupal.org/node/1945476
Follow up for chdir() discussion over here:
#1792310: Wrong DRUPAL_ROOT with non-standard code structure
Comment #44
moshe weitzman CreditAttribution: moshe weitzman commentedLooks good.
Comment #46
tstoecklerOpened #2056845: Stale documentation in drupal_bootstrap() regarding DRUPAL_ROOT as a follow-up.
Comment #47
donquixote CreditAttribution: donquixote commentedI am not really sure about this change.
Having DRUPAL_ROOT in bootstrap.inc means that every script that needs it has to include bootstrap.inc, or define it itself.
And any script that did already define DRUPAL_ROOT by itself can no longer include bootstrap.php.
I am thinking specifically about PHPUnit.
Comment #47.0
donquixote CreditAttribution: donquixote commentedsdf