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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch 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 🇨🇦
CreditAttribution: webchick 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 CreditAttribution: 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 CreditAttribution: pwolanin commented

yes, revert please

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Active » Needs review
FileSize
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 CreditAttribution: scor commented
Status: Needs work » Needs review
FileSize
socrappywecantevenuseit2.patch1.94 KB

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

scor’s picture

scor CreditAttribution: scor commented
FileSize
socrappywecantevenuseit3.patch1.94 KB

fix typo

yched’s picture

yched CreditAttribution: 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 CreditAttribution: scor commented
Title: Convert all require(DRUPAL_ROOT . 'file') to module_load_include() » Do not use module_load_include() in global context
FileSize
599122_require_drupal_root_file_11.patch1.95 KB

rerolling

scor’s picture

scor CreditAttribution: scor commented
FileSize
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 CreditAttribution: yched commented
Status: Needs review » Reviewed & tested by the community
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch 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 🇨🇦
CreditAttribution: webchick 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 CreditAttribution: 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
CreditAttribution: catch commented

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

scor’s picture

scor CreditAttribution: 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.