See http://drupal.org/node/259623#comment-1026200 for origin of this.

The PHP define statement is repeated in update.php, install.php, index.php, xmlrpc.php, cron.php and also in run-tests.sh.

There are also a few votes for a drupal_include(), which would probably result in everything being require_once'd instead of using a mixture of include, require, include_once and require_once (see #65 and #6, #7 in the other issue).

CommentFileSizeAuthor
#4 319154.patch3.86 KBkbahey

Comments

gpk’s picture

gpk commented

See also the section Including Code at http://drupal.org/coding-standards. The distinction between require_once and include_once passes over my head somewhat :P

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Subscribe. Thanks, gpk!

kbahey’s picture

kbahey commented

Subscribing

kbahey’s picture

kbahey commented
Status: Active » Needs review
StatusFileSize
new 319154.patch3.86 KB

Here is a patch that is tested and works.

The only exception is that install.php calls requires install.inc first, then requires bootstrap.inc after that, so I had to use ./includes/install.inc here.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs review » Needs work

Wait wait wait. We don't want to remove DRUPAL_ROOT. We want to just define it in one place.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs work » Needs review

Ohhh nevermind. I need to read patches. ;)

damien tournoud’s picture

damien tournoud commented

I'm not sure I like this.

Sure we have a little code duplication by setting DRUPAL_ROOT at the beginning of every entry point. But there are not many of them, and we already have other shared code (namely include_once 'includes/bootstrap.inc' and 'drupal_bootstrap()'). With this patch we will go backward in term of consistency, because now there are some './' ... and some DRUPAL_ROOT . '/' ...

My call: this patch doesn't worth it.

kbahey’s picture

kbahey commented

The reason for this patch is to have DRY (Don't Repeat Yourself). This way, command line modules like drush don't have to define DRUPAL_ROOT either. See Moshe's comment in the linked issue.

bootstrap.inc is always required using the ./ syntax, so it is consistent.

The only exception is install.inc when included in install.php, and that is because it is the first include. If it can be moved after bootstrap.inc is included in install.php, we will be 100% consistent, but I am not sure if that is feasible (if it was, someone would have done it already).

dries’s picture

dries commented

I'm with Damien on this -- it is probably not worth it.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs review » Closed (won't fix)

Yeah, upon reviewing Damien's comments, and especially with Dries's -1, I think it's safe to put this in the "won't fix" bucket. Sorry for the trouble. It sounded good on paper, but it introduces lots of special cases which is going to lead to poor DX and developer confusion.