[DX][BC] Replace module_load_include() with new, more generic function

[jenlampton]

The argument order in module_load_incude is frustratingly stupid.

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

It requires devs (even those of us who use this thing all the time) to continue to look up the order because it's so non-intuitive. Ideally, this function would not be specific to modules, and would allow you to specify the entire file name as a single argument. something like...

backdrop_load_include('module', 'node.admin.inc');

or

backdrop_include_file('theme', 'basis', 'template.php');

We could even go one step further and always limit the search for .tpl.php files to the modulename/templates or themename/templates directory, .css files to modulename/css or themename/css and .js files to modulename/js or themename/js. This pattern can be extended to anything else we decide to neatly place into subdirectories (.test files in modulename/tests, etc).

This issue will depend on the addition of the BC layer, and the dupal.inc file.

[ghost]

I like the idea of a simplified, generic function, however I think this'll constitute an API change right? Setting to 2.x-future then.

And I think more feedback would be helpful.

[bugfolder]

I completely agree about the non-intuitive-ness of module_load_include(), and @jenlampton's suggested backdrop_load_include() makes good sense. Could we introduce backdrop_load_include() in a 1.x release, so that devs could start using it now? Then as we approach 2.x release, add deprecation warnings in docs (or even in watchdog log)?

[ghost]

Yes, re-reading this issue, I don't see why this needs to wait until 2.x. We can just add a new function now and start using it, while leaving module_load_include() in place for backwards-compatibility (with an @deprecated noticed).

[ghost]

Here's a PR: https://github.com/backdrop/backdrop/pull/3512

I added a deprecated notice to the old function (but otherwise left it as is), and made a new function. The new function:

• Is in common.inc because it's no longer module-specific
• Is called backdrop_load_file() because it's not specific to 'include' (*.inc) files
• Has 3 parameters: $project_type, $project_name, $filename because we need to load the path to the module, theme, layout, etc. first (so we need to know the project type and name), then prepends the filename to that
• Is otherwise more-or-less the same code as module_load_include()

I didn't change the 120+ calls to module_load_include() to the new function yet, until we get the name and parameters locked in 🙂

[ghost]

I'm changing the type of this issue from task to feature request, since I think this should go into a minor release, not a bug-fix release.

[docwilmot]

What do we do about form_load_include()?

[indigoxela]

Not sure yet, if this change would be possible in a minor release, but it's an addition, so maybe yes. Function module_load_include is widely used, though... (See https://github.com/search?q=org%3Abackdrop-contrib+module_load_include&type=code)

I'm pretty sure, that this one's sort of related: https://github.com/backdrop/backdrop-issues/issues/4684 (using __DIR__ instead of backdrop_get_path()).

[klonos]

Why not change module_load_include('inc', 'node', 'node.admin'); to a wrapper that calls the new function + logs a watchdog message? Something like:

function module_load_include($type, $module, $name = NULL) {
  if (!isset($name)) {
    $name = $module;
  }

  watchdog_deprecated_function('common', __FUNCTION__, t('Change record:') . ' ' . l(t('Replaced module_load_include() function with a generic backdrop_load_file() function'), 'https://api.backdropcms.org/change-records/blah-blah-blah', array('attributes' => array('target' => '_blank'))));

  backdrop_load_file('module', $module, $name . '.' . $type);
}

What do we do about form_load_include()?

We don't have to, but it seems to me that we could add another &$form_state parameter to the new function, and then deprecate that one as well.

Use this function instead of module_load_include() from inside a form constructor or any form processing logic as it ensures that the include file is loaded whenever the form is processed. In contrast to using module_load_include() directly, form_load_include() makes sure the include file is correctly loaded also if the form is cached.

I am reading all that ^^ as basically making developers think more and do more work, when we could be doing things automagically for them.

[docwilmot]

I dont think we can combine the functions. But maybe, to match this change, something like: function form_load_file(&$form_state, $project_type, $project_name, $filename).

[ghost]

PR updated with @klonos' suggestion for making module_load_include() a wrapper for backdrop_load_file().

I suggest moving the form_load_include() discussion to a follow-up issue.

[ghost]

Note that the link to the change record doesn't exist yet, so the record needs to be created just before we merge this, then the PR updated with the actual link.

[klonos]

I suggest moving the form_load_include() discussion to a follow-up issue.

Yup 👍🏼

I've reviewed the code in the PR, and it looks good. I've left a couple of suggestions, but the main thing is that tests are failing, so setting to NW.

[argiepiano]

A couple of quick comments:

• I believe the word 'include' in module_load_include makes it easy to relate this function to PHP's include_once, which is a staple of PHP. In some ways module_load_include is an extension of include and include_once. So, I'm wondering if it would make sense to call the new function backdrop_include_file instead of backdrop_load_file. Personally, with the D7 and B1 API having so many functions, it's easier for me to find (with my IDE's autofill help), and easier for me to remember functions when they are somehow related, or use some of the same words, as the related PHP function
• When testing this addition and change, please look at update.php and the migration process. (I haven't done this myself). update.php is very selective in loading core 'inc' files when migrating or updating a site. If we switch module_load_include to backdrop_load_file (or backdrop_include_file) we want to be sure that the file where this function is defined is loaded during the update process.

[klonos]

...I'm wondering if it would make sense to call the new function backdrop_include_file instead of backdrop_load_file.

I like that suggestion 👍🏼

[ghost]

I've updated the PR with suggested changes. Tests are still failing, and it's weird that PHP 8 works fine, but PHP 7 & 5 don't... Could use some help with debugging that please.

[indigoxela]

and it's weird that PHP 8 works fine

It does not. Tests fail even harder when starting the profile cache with "Preparing database and configuration cache for profiles TypeError: array_merge(): Argument 1 must be of type array, null given".

[ghost]

Then I'm confused...

[indigoxela]

Then I'm confused...

That's understood, it's an edge case: Simpletests didn't even start to run. You see that when you expand the section.

There must be a severe regression somewhere, but at least Backdrop still seems to install.

[ghost]

Ah, thanks!

[bugfolder]

I created a new PR, https://github.com/backdrop/backdrop/pull/4520, based on https://github.com/backdrop/backdrop/pull/3512 so that I could play with it and see if I could figure out why tests were failing.

Spelling was failing because PHPCS doesn't like "(optional)" (uncapitalized) as a parameter comment. (There's a ton of existing examples of that in core code.)

[bugfolder]

After some tinkering and testing...

• There was a bug in the previous PR. The function module_load_include() still needs its return value (which is the return value from backdrop_include_file()). This fixes a few of the test failures.

However, all automated tests were still failing. And, puzzlingly, they all passed when run locally. So I did a bunch of code edits pushed to the PR to repeatedly restart the automated tests with different "what changed?" variations, and eventually narrowed it down to the watchdog_deprecated_function() call. When commented out, all tests pass (except spell-check, which doesn't like the long comment); when uncommented, all tests fail as described above.

[bugfolder]

I also noticed a related function, module_load_all_includes(), which cycles through all modules and calls module_load_include() for each. This is going to call watchdog_deprecated_function() for every module, which seems sort of wasteful (fortunately, that function doesn't post repeated watchdog messages for the same file:line combination).

The function module_load_all_includes() is only called in a single place in core (in bootstrap.inc), and it only contains 4 lines of code. (I did a search through a bunch of contrib modules and didn't find it used in any.) So I would suggest that this function, too, should be deprecated, and its one usage in core replaced with the explicit code that uses backdrop_include_file().

[argiepiano]

I'm having second thoughts about this change. This will affect tons of contrib modules and custom modules - a search on backdrop-contrib turns up more than 800 instances. It will surely create a lot of work for maintainers and developers. Replacing the old function with the new one is not just a simple find/replace - the parameters and their order have also changed. People's dblogs will fill up with deprecated notice.

If there there was a substantial gain from this change, I'd be supportive. But the reason for the change (some of us having a hard time remembering the syntax of module_load_include()) just doesn't seem strong enough to justify the cost it will create. I'm not supportive of this.

[bugfolder]

I've removed the usage of module_load_all_includes() and added the same deprecation notice in it (referencing the same change record). I've confirmed in local testing that both of the watchdog_deprecated_function() notices place the appropriate notices in watchdog when deprecated notices are enabled. However, uncommenting watchdog_deprecated_function() in module_load_include definitely kills all automated tests (even though local tests pass).

[bugfolder]

People's dblogs will fill up with deprecated notice.

@argiepiano, only if they've turned on deprecated notices, which are not turned on by default. But your other point is well taken.

Having isolated the test failure issue (and it is rather tedious to continue testing things via PR+automated tests), I think at this point I'll hold off on further activity to give the originator @jenlampton and others who have chimed in over the years (!) a chance to comment further. (Maybe bring this up at a dev meeting.)

[indigoxela]

@bugfolder I think, we all agree, that the module_load_include() function has a weird syntax. But I'm with @argiepiano here re deprecating it. It affects a lot of contrib modules and also custom code we're not even aware of. And it also would be a hurdle for D7 to B upgrades with custom code.

Our philosophy states: "Backdrop values the needs of the contrib developer over the needs of the core developer." And I think, that statement's relevant here.

• Providing an easier to read and use alternative? Yes, good idea.
• Deprecating a widely used function for little gain? I don't think so.

[docwilmot]

Why would devs need to search replace occurrences of module_load_include()? The PR suggests that calls to that function would still work normally (except for the watchdog notice).

[indigoxela]

Why would devs need to search replace occurrences of module_load_include()

If we deprecate module_load_include(), we'd require them to do so. :wink: Functional tests would fail and we shouldn't encourage devs to just ignore deprecations – be it only "watchdog" or not. :wink:

[docwilmot]

So the solution recommended here is to remove the deprecation notice?

[argiepiano]

My suggestion is just to keep using module_load_include() and not bother with a new function that does the same thing with a different syntax. In other words, close this issue. module_load_include() is well established and has been used since Drupal 6.