http://api.drupal.org/api/function/module_load_include/6
Currently:
$name Optionally, specify the file name. If not set, the module's name is used.
What is meant is specify the file name without extension. However, the term "file name" includes the file extension: http://wikipedia.org/wiki/Filename. Therefore, the text should be corrected to
$name Optionally, specify the file name without extension. If not set, the module's name is used.
From looking at the parameters you might guess that the file name should be specified without an extension, but the parameter description should be explicit and precise so no guessing is needed.
| Comment | File | Size | Author |
|---|---|---|---|
| #22 | 485350_D6.patch | 1.15 KB | jhodgdon |
| #19 | 485350-followup-D7.patch | 963 bytes | dave reid |
| #11 | 485350.patch | 1.02 KB | jhodgdon |
| #2 | 485350.patch | 697 bytes | jhodgdon |
Comments
Comment #1
threexk commentedComment #2
jhodgdonShould be patched in D7 and then ported back to D6.
Here's a patch.
Comment #4
jhodgdonSetting back to Needs Review in hopes that the patch will get retested. The test bot was apparently malfunctioning (I had about 7 patches fail in that time period, all were doc, none broke the HEAD install I am pretty sure).
Comment #6
jhodgdonTest bot still not working?
Comment #7
threexk commentedThanks for making a patch. It applied fine for me. My Drupal HEAD broke when I updated it from CVS and I had to drop the database and reinstall to get things working. Maybe the test bot has the same problem? (Or maybe I'm just updating from CVS incorrectly -- cvs update -PCdR)
Comment #8
moshe weitzman commentedComment #9
webchickYou know, in reading this, it's sufficiently confusing that we could really do with a @code example to make it more clear.
Comment #10
jhodgdonThat's a thought... although if you are on api.drupal.org, you can of course click on one of the 17 functions that call this function to get an example... I'll see about adding an example though.
Comment #11
jhodgdonHow about this patch: same text, addition of two examples?
Comment #12
threexk commentedMight it be better if the syntax of the function was changed to:
module_load_include($module, $filename)So for example:
module_load_include('node', 'node_admin.inc');This would cause some redundancy in the parameters like:
module_load_include('node', 'node.install');But is it really that bad? This form of the function seems a whole lot clearer to me and would be easier to document. It also seems like the third parameter to the current function gets used a lot.
Comment #13
threexk commentedI grepped one of my 6.x sites. There are 82 calls to module_load_include(). Of those, 68 use the third parameter to module_load_include(). That's 83%.
So most of the time the current form of module_load_include isn't useful. I think the two-parameter form I suggested would be better because it's simpler and it's briefer for the common case.
Comment #14
jhodgdon+1 on this idea. However, I think it should be filed as a separate issue against the core of Drupal 7 (there is zero chance it would be changed in Drupal 6, due to the way it would break a ton of contrib modules). The present issue is a doc issue that affects D6 and D7 (in its current state).
Comment #15
webchickI agree with both. The function signature makes absolutely no sense and should be changed in 7.x, and the documentation that's currently there for the function is confusing. So let's keep this issue to just the docs stuff, and start a separate issue for "Make module_load_include() actually make sense" :) Please reference threexk's findings, as that's a very compelling case for changing the function signature.
Committed to HEAD. Still applies to 6.x with offset, so marking RTBC there.
Comment #16
threexk commentedAdded http://drupal.org/node/507396 for the module_load_include() change proposal.
Comment #17
dave reidBTW, the example to load an install file should not be using module_load_include(). It should be module_load_install() since that also loads install.inc. A better example would be
module_load_include('inc', 'node').Comment #18
threexk commentedDave Reid: You're right, but there's no node.inc ...
Regarding the second example, couldn't you do
module_load_include('admin.inc', 'node');instead of
module_load_include('inc', 'node', 'node.admin');I should try it, but if it works I'm not sure the current example demonstrates the necessity of the third parameter. What about:
module_load_include('inc', 'node', 'content_types');Comment #19
dave reidFollowup HEAD patch for review.
Comment #20
jhodgdonHa! I took that .install example from module_load_install(). :) Your patch of my patch looks good.
Then we should make a new one for D6.
Comment #21
webchickThanks, committed this to HEAD.
Marking down for 6.x.
Comment #22
jhodgdonHere's a patch for D6, incorporating the patches in #11 and #19 above.
Comment #23
jhodgdonThis patch was approved for D7, so updating status.
Comment #24
gábor hojtsyThanks, committed to Drupal 6 too.