The documentation of drupal_system_listing() in core/includes/common.inc in Drupal 8.x says:

 * this function will search the site-wide modules directory (i.e., /modules/),
 * your install profile's directory (i.e.,
 * /profiles/your_site_profile/modules/), the all-sites directory (i.e.,
 * /modules/), and your site-specific directory (i.e.,
 * /sites/your_site_dir/modules/), in that order, and return information about
 * all of the files ending in .module in those directories.

This is definitely wrong -- it mentions searching /modules/ twice. What happened is that the directory structure changed in D8 and this documentation was updated incorrectly.

So looking at the current D8 code, what it's actually searching is (in this order):
- /core/modules
- (testing profile directory if a test is being run)/modules
- (active installation profile directory)/modules -- this could be in core/profiles/(profile dir)/modules or /profiles/(profile dir)/modules by the way!
- /modules [which is now the standard place to put your contrib/custom modules]
- /sites/all/modules [the legacy place to put your contrib/custom modules]
- /sites/(your specific site)/modules
That's the search order -- and later directories in the search order take precedence, so the precedence order is the reverse of that.

So, we need a patch that corrects this in the documentation. (note: the above are *notes* and someone needs to write it into reasonable documentation -- don't use that list word-for-word. :) ) Probably a good Novice project?

See also these issues:
#1539940: Encourage best practices in /sites/README.txt, /modules/README.txt, /profiles/README.txt and /themes/README.txt (where the README files for D8 were standardized)
#1799116: [policy/patch] Standardize on "installation profile" instead of "install profile" (where I noticed this issue)
change notices: http://drupal.org/node/1724216 and http://drupal.org/node/1766160

CommentFileSizeAuthor
#15 drupal-correct_documentation-1802280-10.patch1.38 KBmbrett5062
#10 drupal-correct_documentation-1802280-10.patch1.38 KBmbrett5062
#8 drupal-correct_documentation-8.patch1.45 KBmbrett5062
#6 drupal-correct_documentation-1802280-6.patch1.38 KBmbrett5062
#4 drupal-correct_documentation-1802280-4.patch913 bytesmbrett5062
#1 drupal-correct_documentation-1802280-1.patch1.39 KBmbrett5062
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Issue summary: View changes

fix links

mbrett5062’s picture

mbrett5062 CreditAttribution: mbrett5062 commented
Assigned: Unassigned » mbrett5062
Status: Active » Needs review
FileSize
drupal-correct_documentation-1802280-1.patch1.39 KB

Took a stab at this. Hope this is something along the lines you were thinking.

jwilson3’s picture

jwilson3
Primary language English
Location Ecuador
CreditAttribution: jwilson3 commented
Status: Needs review » Needs work

Mostly this is very good! Thanks!

"Multisite" has no dash in core documentation.

you mention that the code searches in a specific order and there is a part that explains how the list is keyed. I havent looked at the surrounding code, but we should check to ensure we're documenting why these two things are important. Namely, Extensions with the same name will be overwritten by items found lower in the list.

jwilson3’s picture

jwilson3
Primary language English
Location Ecuador
CreditAttribution: jwilson3 commented

Also, you've introduced new ways of talking about directory tree patterns with the "[your site]" and the "[Profile dir]" syntax. My vote goes to stay with the existing way for consistency, and file a follow up to change this labeling syntax in all the documentation.

Edit. I should clarify, I like this syntax, but I think it should be updated everywhere, not just here.

mbrett5062’s picture

mbrett5062 CreditAttribution: mbrett5062 commented
Status: Needs work » Needs review
FileSize
drupal-correct_documentation-1802280-4.patch913 bytes

As per #2 & #3

multi-site changed to multisite

and

directory tree pattern reverted to the original syntax i.e. /sites/your_site_dir/modules

By the way, I can find no documentation of this directory tree pattern, and some times the above is used, then elsewhere the following is used. See later in common.inc (line #5154 with my patch applied)

sites/<domain>

Also, the rest of the documentation immediately following my change explains the precedence and consequences.

Status: Needs review » Needs work

The last submitted patch, drupal-correct_documentation-1802280-4.patch, failed testing.

mbrett5062’s picture

mbrett5062 CreditAttribution: mbrett5062 commented
Status: Needs work » Needs review
FileSize
drupal-correct_documentation-1802280-6.patch1.38 KB

Try that again. Will get the hang of this one day :)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

Thanks for the patch(es)! Here's the current text from the latest patch:

+ * the function will search the following directories in this order,
+ *    /core/modules
+ *    /core/profiles/testing/modules (Only if a test is being run!)
+ *    /core/profiles/profile_dir/modules or /profiles/profile_dir/modules
+ *    /modules (Now the standard directory for your contrib/custom modules)
+ *    /sites/all/modules (Legacy contrib/custom modules directory, see above)
+ *    /sites/your_site_dir/modules (for multisite installations)
+ * and return information about all of the files ending in .module in those
+ * directories.

It looks correct, but it needs a little clean-up:

a) Needs to be formatted as a list. Standards: http://drupal.org/node/1354#lists
b) Has a few punctuation and capitalization issues (. at end of each line; don't capitalize Now or Legacy in the parenthetical remarks, don't use ! in documentation, needs : before the start of the list)
c) I would take out the word Now in (Now the standard directory for your contrib/custom modules)

mbrett5062’s picture

mbrett5062 CreditAttribution: mbrett5062 commented
Status: Needs work » Needs review
FileSize
drupal-correct_documentation-8.patch1.45 KB

Updated to Drupal standards, re #7

Thanks @jhodgdon

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

Thanks, much better!

One thing:

+ * - /core/profiles/profile_dir/modules or /profiles/profile_dir/modules.

I think this is a bit unclear. It needs to get across the idea that it searches the modules directory under the active installation profile's directory.

Also, at the end:

+ * and return information about all of the files ending in .module in those
+ * directories.
  * The information is returned in an associative array, which can be keyed on

It needs to have a blank line to indicate a new paragraph.

mbrett5062’s picture

mbrett5062 CreditAttribution: mbrett5062 commented
FileSize
drupal-correct_documentation-1802280-10.patch1.38 KB

Revised inline with #9

mbrett5062’s picture

mbrett5062 CreditAttribution: mbrett5062 commented
Status: Needs work » Needs review

Forgot to send for test.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Assigned: mbrett5062 » jhodgdon
Status: Needs review » Reviewed & tested by the community

Thanks, looks good! I'll get it committed sometime soon.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal-correct_documentation-1802280-10.patch, failed testing.

mbrett5062’s picture

mbrett5062 CreditAttribution: mbrett5062 commented

hhmmm let me clean up and resend for testing, should not have broken. But I still don't get git fully.

mbrett5062’s picture

mbrett5062 CreditAttribution: mbrett5062 commented
Assigned: jhodgdon » mbrett5062
Status: Needs work » Needs review
FileSize
drupal-correct_documentation-1802280-10.patch1.38 KB

OK try again.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Assigned: mbrett5062 » jhodgdon
Status: Needs review » Reviewed & tested by the community

Still RTBC if the test bot agrees. :) [assigning to me to commit]

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Fixed

Thanks for seeing this through to the end mbrett5062 ! Committed to 8.x. If this was your first core patch, congratulations (actually, even if it wasn't your first one)!

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

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

fix another link,oops