There is a single call to ModuleHandler::loadAllIncludes() in core and never should have been using that in the first place. Searching in contrib, there are a few module handler subclasses and decorators and maybe 2-3 somewhat valid calls for loading all install files, one is schema module.
There is one in xmlsitemap, which calls its own requirements hooks.
Deprecate for D13, remove call in search module.
ModuleHandler::loadAllIncludes() is now deprecated.
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !12751
Comments
Comment #3
berdirThis is a blocker for #3518903: Convert template preprocess in system.module
Comment #4
nicxvan commentedThis is on my list to fully review.
First pass this looks great.
One thing though:
that is not quite right, hook_requirements isn't deprecated yet.
Comment #5
oily commentedRE: #4 @nicxvan I cannot see where that text appears? Has it been corrected now?
Comment #6
nicxvan commentedIt's in the issue summary.
Comment #7
oily commentedMade the CR a bit clearer, though now Im not sure if @berdir means '... loadAllInclude() can be used in a loop for modules'? maybe 'used in a loop for includes'?
Comment #8
oily commentedComment #9
oily commentedRE: #6 updated IS.
Comment #10
nicxvan commentedI don't follow your changes to the issue summary.
What do you mean by
nothing there is deprecated.
And again
My point was that hook_requirements is not deprecated, there is no need at all to mention deprecation regarding it.
I've updated the IS to remove references to deprecation.
@berdir the one thing I'm struggling with is if removing
$this->moduleHandler->loadAllIncludes('admin.inc');works, why don't we just do that in the other issue.What is the purpose of deprecating this here and now? Is it to just move down the road of discouraging .inc files?
Or is it more that this is so rarely used and fairly easy to replicate that we might just want to deprecate to slim down ModuleHandler a bit?
We might want to update the CR in that case to mention that deprecating include files is more future, the CR kind of implies they are deprecated with this change.
Comment #11
nicxvan commentedAll that said I reviewed the MR, all deprecations are correct, tests are updated. This is the way to do it if we move forward here.
I'm not going to go as far as tagging this needs framework manager review, but it might need confirmation of the direction we're going.
This just feels different than the magic loading we were doing with hook_info. This is more of a utility method.
Again, not opposed to this, just looking for a bit more on it.
Comment #12
berdirYes, it's one small step toward removing cruft from ModuleHandler.
But also, by leaving deprecated .inc files in core, we soft-deprecate this already. Once the system issue is in, you couldn't call it with 'admin.inc' without triggering a deprecation. And once for example the views issue is in, you couldn't call it with 'theme.inc'.
And last, removing that call in the system.module issue is IMHO out of scope, so I'd want to do a separate issue anyway.
It's relevant because I reviewed usages for this method in contrib. There are exactly 3 calls, all of them use it to load all .install files. One of those to call requirements hooks.
hook_requirements() isn't officially deprecated, but it's kind of a formality at this point. We have all replacements in place, core is fully converted except one test hook (no alter hook test though, that was converted too). We discussed that in the requirements deprecation issue, IMHO the current state is a problem because it's unclear how modules are supposed to convert while remaining compatible with older versions, which I think is important. I'll try to make a proposal on that issue, but yes, all is unrelated to this issue.
The only bit that's related is that we do want to and will deprecate that, so that code will need to move way from loadAllIncludes() anyway.
Comment #13
oily commentedRE: #10 I also edited the CR.
Comment #14
xjmComment #15
oily commentedRE:10 The source of my confusion was the idea of 'Include files'. If it had said '.inc' files that makes immediate sense to me. On the other hand the function allows the file extension to be specified so not only '.inc' extension. This seems to be why hooks are mentioned in the IS..
But then looking in core, the one instance of loadAllIncludes() is
loadAllIncludes(type: 'admin.inc')but the 'type' parameter is supposed to be the file extension, not the whole filename. To load all files named 'admin.inc' the function would need to be
loadAllIncludes(type: 'inc', name: 'admin')So it seems this one instance of the function in core does nothing at all, not even throw an exception.
Comment #16
nicxvan commented.admin.inc is the file extension.
I made an attempt to clarify the CR that could use another review.
Once that is done I think the code side of this is ready.
Comment #17
oily commentedComment #18
oily commentedThe new CR draft is good. RTBTC.
Comment #19
nicxvan commentedComment #20
berdirThe arguments to the load include functions are super confusing, IIRC you'd only use the name argument if it wouldn't start with the module name.
Comment #23
catchCommitted/pushed to 11.x, thanks! Published the CR.