Change module_load_include() parameters

+1

I think the name of the function itself is bad too. Why is it 'module' and not 'drupal'? Also, 'load' and 'include' are both verbs, unless the author was intending that 'include' refer to the file to be included, but an install file is not an include file it's an install file. ;)

What about drupal_include_file()?

I don't think this is going to happen for 7, though.

We could even make the $module parameter optional, so the module name would be guessed.

<?php
module_include('node.something.inc');
module_include('subdir/mymodule.somethingelse.inc');
?>

Would automatically detect that "node" / "mymodule" must be the module name.

In case of doubt, we add the $module as a second parameter.

<?php
module_include('subdir/content_types.inc', 'content');
?>

Implementation:

<?php
function module_include($name, $module = NULL) {
  if (!isset($module)) {
    $fragments = explode('/', $name);
    $fragments = explode('.', end($fragments));
    $module = $fragments[0];
  }
  $file = './' . drupal_get_path('module', $module) . "/$name";

  if (is_file($file)) {
    require_once $file;
  }
  else {
    return FALSE;
  }
}
?>

--------------

I am not so sure about this.
Maybe the more explicit form is the better option, with module_include($module, $path).

But I agree that having the $type as a first param is awkward.

Hm, actually...
i notice I have never used module_load_include() to its full potential!
You can module_load_include('something.inc', 'mymodule') to get
path/to/mymodule/mymodule.something.inc
Unfortunately, this does break with files in subfolders.

c.f. #697946: Properly deprecate module_load_include() and move it into \Drupal::moduleHandler() service

See also #1945072: reorder the paramters of module_load_include() to make sense. Which could probably be folded into this -- the gist over there is that the current order of parameters as 'SUFFIX, FOLDER, FILENAME' is just crazy.

Marked #1945072: reorder the paramters of module_load_include() to make sense as a duplicate; copying over the tags.

Just sticking to a reordering. I think this is clearer for 90% of the usage

+++ b/core/includes/form.inc
@@ -543,7 +543,7 @@ function form_get_cache($form_build_id, &$form_state) {
-            module_load_include($file['type'], $file['module'], $file['name']);
+            module_load_include($file['module'], $file['name'], $file['name']);

Looks like you've lost ['type'].

+++ b/core/includes/module.inc
@@ -175,10 +175,10 @@ function module_load_install($module) {
- * @param $type
- *   The include file's type (file extension).
  * @param $module
  *   The module to which the include file belongs.
+ * @param $type
+ *   The include file's type (file extension).
  * @param $name
  *   (optional) The base file name (without the $type extension). If omitted,
  *   $module is used; i.e., resulting in "$module.$type" by default.

This improves things, but we're still not at the actual order that the pieces go together to make the full filename.

Though I see from looking at the calls in your patch that code often cheats a bit, and uses $type to give more than just 'inc', eg 'language.inc' to load MODULE.language.inc.

If that's the expected usage, then I suppose we hardly ever need to use $name, as a module's files should be prefixed with the module name in general.

What it does mean though is that better documentation is needed to explain the intended use of this -- I have always read $type to be a strict filetype extention, so only 'inc', so I always call it with $name of the form 'MODULE.admin', say.

The purpose of the original thing is that in e.g. "../modules/views/views.admin.inc" you don't have to type "views" twice.
This is the only reason for having 3 parameters instead of 2.
module_load_include('admin.inc', 'views');
Saves you some typing, but does not look very intuitive.

The idea already breaks if the file is in a subdirectory: "../modules/views/admin/views.admin.inc", here you would need 3 parameters and combine them in a wonky way.
module_load_include('admin.inc', 'views', 'admin/views');
OR
module_load_include('inc', 'views', 'admin/views.admin');

The natural thing would be this instead:
module_load_include('views', 'views.admin.inc'); OR
module_load_include('admin.views.inc', 'views');

There is only one way I could imagine to save the shortcut:
module_load_include('.admin.inc', 'views'); -> ../views/views.admin.inc
module_load_include('admin.inc', 'views'); -> ../views/admin.inc
module_load_include('views.admin.inc', 'views'); -> ../views/views.admin.inc
module_load_include('.admin.inc', 'views', 'admin/'); -> ../views/admin/views.admin.inc

but I think even that feels quite arbitrary.
Probably better to just spell the module name twice if it needs to be.

I still think this is an improvement.

To load node/node.admin.inc and node/content_types.inc

Before:

module_load_include('admin.inc', 'node');
module_load_include('inc', 'node', 'content_types');

After:

module_load_include('node', 'admin.inc');
module_load_include('node', 'inc', 'content_types')

That's why we have the third param. If we want to rename content_types.inc to node.content_types.inc and nix the third param altogether, fine, but I don't think that really matters.

May I humbly suggest we favor readability over brevity in this instance? I see this issue is already tagged "increases learning curve", and I think intuitability is a key impact of this decision. I would recommend something like this module_load_include($module, $file);

e.g.

module_load_include('locale', 'locale.bulk.inc');
module_load_include('views', 'includes/ajax.inc');
module_load_include('whatever', 'some/arbitrary/path/to.a.inc');

There's a logical narrowing of focus from left to right (include something > from this module > at this path), each component makes sense on its own, and there's really only one way to do split it. I'm not a heavyweight contributor or anything, but perhaps my input will serve to illuminate how a common developer might expect things to work—which I think is what this issue is aiming to accommodate. :)

TravisGarden +1

@timplunkett (#11) could you clarify (via edit) which files you are actually including there?
Funny thing, #12 does not actually need any such clarification :)

The order proposed in the OT certainly is better than the status quo, but even more I support the more readable proposal in #12.

But IMHO we're actually missing a point:

module_load_include('node', 'node.install');

will never be necessary as we're having the very straightforward module_load_install().

In the end, we don't want module_load_include() resp. ModuleHandler::loadInclude() load any code file, but we want it to load '.inc' files only. At least in core, we're not even using it for files with other extensions at all. Hardcoding the extension would make things more consistent and finally less error prone. If we want, we could then also leave out the ".inc" extension, so it would read:

module_load_include('node', 'content_types');

DX improvement rather than new functionality, therefore recategorizing as task rather than feature.

Actually, @Pancho, module_load_install() currently uses module_load_include(), so unless we wanted to rewrite its internals ad hoc, module_load_include() still needs to support arbitrary, non-.inc. files. But perhaps more importantly, it seems to me that it's more readable and easier to intuit if the parameter is a full filename instead of some permutation of one—especially if it contains a path, which really makes it look like something's missing if you omit the extension. Take this line for example:

module_load_include('views', 'includes/ajax');

At first glance, doesn't it look like a typo?

so unless we wanted to rewrite its internals ad hoc

Rewrite it's internals sounds like a huge task, while it's actually a 1 minute exercise:
Move 8 lines of code into a helper, say _load_include() and call it from both module_load_include() and module_load_install().
Or, duplicate the 8 lines of code, if that should perform a tiny little bit better.

But perhaps more importantly, it seems to me that it's more readable and easier to intuit if the parameter is a full filename instead of some permutation of one

I said, if we want, we could then also leave out the ".inc" extension. If we don't want, we don't. I don't care about that.
What I do care about is:
a) simplify parameters
b) separate functions for separate types, so we're avoiding errors and can do things like implementing loadInstall() only in something like a ModuleInstaller class (see #2010380-11: Deprecate module_load_install() and replace it with ModuleHandler::loadInstall and #2004784: Move module install/uninstall implementations into ModuleInstaller), while loadInclude() is only implemented in ModuleHandler. Stuff like that adds to a much more robust API.

We need to settle for a solution, ASAP. Less than 48 hours left.
We should disentangle module_load_install() from module_load_include(), restrict the latter on includes, and otherwise go for Travis' proposal in #12 which seems to be less controversial.
I'm going to do a patch later today.

Here's a simple suggestion for an updated function, just to keep the ball rolling.

module_load_include() is being deprecated #697946: Properly deprecate module_load_include() and move it into \Drupal::moduleHandler() service

Unassigning this from me.

Looks better to fix #697946: Properly deprecate module_load_include() and move it into \Drupal::moduleHandler() service