Dave reid pointed this in one of my patches yesterday. This patch patches field and file modules which are the only remaining modules containing require(DRUPAL_ROOT ...)

CommentFileSizeAuthor
#12 599122_require_drupal_root_file_12.patch2.86 KBscor
#11 599122_require_drupal_root_file_11.patch1.95 KBscor
#9 socrappywecantevenuseit3.patch1.94 KBscor
#8 socrappywecantevenuseit2.patch1.94 KBscor
#6 socrappywecantevenuseit.patch1.67 KBcatch
conv2module_load_include.patch1.33 KBscor

Comments

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Yes. We have APIs, and even when they're as crappy and confusing as drupal_load() and module_load_include(), we should use them.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Yeah, I have to agree I find the old code more readable, but this at least makes it consistent.

Committed to HEAD. Thanks!

Status: Fixed » Closed (fixed)

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

damien tournoud’s picture

damien tournoud commented
Status: Closed (fixed) » Active

Calling module_load_include() in a global context is a really bad idea (module_load_include() will fail if called before common.inc is loaded). Let's revert those.

pwolanin’s picture

pwolanin commented

yes, revert please

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new socrappywecantevenuseit.patch1.67 KB

Straight revert attached. The unreliability should be documented in module_load_include(), but I couldn't think of a good way of putting it - maybe something like "only use this in situations where loading another module's include file, where you cannot be sure of the location.".

Status: Needs review » Needs work

The last submitted patch failed testing.

scor’s picture

scor commented
Status: Needs work » Needs review
StatusFileSize
new socrappywecantevenuseit2.patch1.94 KB

Adding some documentation to module.inc so this does not happen again.

scor’s picture

scor commented
StatusFileSize
new socrappywecantevenuseit3.patch1.94 KB

fix typo

yched’s picture

yched commented

Bump. Note that I reuploaded basically the same patch in #685572-3: system_update_7008() is broken.
Meanwhile, modules/field/tests/field_test.module needs to be fixed as well.

scor’s picture

scor commented
Title: Convert all require(DRUPAL_ROOT . 'file') to module_load_include() » Do not use module_load_include() in global context
StatusFileSize
new 599122_require_drupal_root_file_11.patch1.95 KB

rerolling

scor’s picture

scor commented
StatusFileSize
new 599122_require_drupal_root_file_12.patch2.86 KB

merging #685572-3: system_update_7008() is broken into the previous patch #11 (contains fix for field_test.module).

yched’s picture

yched commented
Status: Needs review » Reviewed & tested by the community
catch’s picture

catch
he/him
Primary language English
commented
Category: task » bug
Priority: Normal » Critical

#685572: system_update_7008() is broken was duplicate, and critical upgrade path bug.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

I guess we should look at removing this stupid, confusing, and useless function in Drupal 8.

Thanks, committed to HEAD.

scor’s picture

scor commented

I guess we should look at removing this stupid, confusing, and useless function in Drupal 8.

where are we keeping track of these things? would that deserve a D7 patch with a @todo? or is there a page/meta-issue for "WTF removal for Drupal 8"?

catch’s picture

catch
he/him
Primary language English
commented

There's a Drupal 8 version tag, no harm opening one against that.

scor’s picture

scor commented

follow-up issue for D8 #697946: Properly deprecate module_load_include() and move it into \Drupal::moduleHandler() service

Status: Fixed » Closed (fixed)

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