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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RobLoach’s picture

Issue tags: -DrupalWTF

Let's move DRUPAL_ROOT definition to bootstrap.inc instead! Thanks sun.

RobLoach’s picture

Title: Operation Remove getcwd() Yuckies » Move DRUPAL_ROOT into bootstrap.inc
Status: Needs work » Needs review
FileSize
10.12 KB

Getting "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.

Status: Needs review » Needs work

The last submitted patch, paths.patch, failed testing.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
9.09 KB

Let's use set_include_path() instead of gross chdir() calls.

Status: Needs review » Needs work

The last submitted patch, 1540136.patch, failed testing.

gpk’s picture

I 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.

gpk’s picture

One 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...

RobLoach’s picture

With #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.

sun’s picture

Relying 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.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
8.7 KB

I 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.

Crell’s picture

Remember 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.

RobLoach’s picture

#10: 1540136.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1540136.patch, failed testing.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
9.25 KB
sun’s picture

Status: Needs review » Needs work

I'm still not really sure/convinced of this.

Regardless of that:

+++ b/core/update.php
@@ -372,12 +364,13 @@ ini_set('display_errors', FALSE);
-require_once DRUPAL_ROOT . '/core/includes/bootstrap.inc';
-require_once DRUPAL_ROOT . '/core/includes/update.inc';
...
+$root = dirname(__DIR__);
+require_once $root . '/core/includes/bootstrap.inc';
+require_once $root . '/core/includes/update.inc';

For clarity, it would help to revert the subsequent $root changes after including bootstrap.inc to DRUPAL_ROOT.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
7.48 KB

It does make manually loading up the bootstrap easier as you don't have to define DRUPAL_ROOT yourself.

RobLoach’s picture

Issue summary: View changes

Updated issue summary.

RobLoach’s picture

FileSize
7.44 KB

Simplifies some of the includes.

RobLoach’s picture

FileSize
7.6 KB

Moved the directory change to drupal_environment_initialize().

RobLoach’s picture

Title: Move DRUPAL_ROOT into bootstrap.inc » Simplify index.php by moving DRUPAL_ROOT to bootstrap.inc
Priority: Normal » Minor
RobLoach’s picture

Issue summary: View changes

Updated issue summary.

RobLoach’s picture

Issue tags: -PHP 5.3
RobLoach’s picture

Issue tags: +PHP 5.3

Add the PHP 5.3 tag since __DIR__ is one of the new constants added to 5.3.

webflo’s picture

sun’s picture

That 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

Carsten Müller’s picture

works 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

Jaza’s picture

Pete B’s picture

Rerolling for latest core.

Status: Needs review » Needs work

The last submitted patch, simplify-index-remove-drupal-root-1540136-26.patch, failed testing.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
7.61 KB
RobLoach’s picture

FileSize
7.68 KB

amateescu in IRC asked for clarification on the dirname and __DIR__ usage. neclimdul helped with adding an additional helpful comment.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Yeah, the comment about double dirname() usage is helpful. Overall, this looks like a really nice cleanup.

sun’s picture

Priority: Minor » Normal

+1

I'm a bit concerned about performing the chdir() in drupal_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 the chdir() 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.

Crell’s picture

Wait, what? Changing the working directory by including a file? That's a bad practice. Files should declare symbols or do things, but not both.

RobLoach’s picture

Let'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.

RobLoach’s picture

Issue tags: -PHP 5.3

#29: 1540136.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1540136.patch, failed testing.

RobLoach’s picture

Status: Needs work » Needs review
Issue tags: +PHP 5.3

#29: 1540136.patch queued for re-testing.

RobLoach’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

moshe weitzman’s picture

Status: Reviewed & tested by the community » Needs work

We 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.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
7.21 KB

Thanks for the review, Moshe. The attached patch doesn't change any of the chdir's.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Looks fine.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

This 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!

webchick’s picture

Title: Simplify index.php by moving DRUPAL_ROOT to bootstrap.inc » Change notice: Simplify index.php by moving DRUPAL_ROOT to bootstrap.inc
Priority: Normal » Critical
Status: Fixed » Active
Issue tags: +Needs change record

Mmm. Actually this could probably use a change notice for providers of foo.php scripts.

RobLoach’s picture

Status: Active » Needs review
moshe weitzman’s picture

Title: Change notice: Simplify index.php by moving DRUPAL_ROOT to bootstrap.inc » Simplify index.php by moving DRUPAL_ROOT to bootstrap.inc
Priority: Critical » Normal
Status: Needs review » Fixed
Issue tags: -Needs change record

Looks good.

Status: Fixed » Closed (fixed)

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

tstoeckler’s picture

donquixote’s picture

I 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.

donquixote’s picture

Issue summary: View changes

sdf