Deprecate ModuleHandler::loadAllIncludes

This is a blocker for 📌 Convert template preprocess in system.module Active

This is on my list to fully review.
First pass this looks great.

One thing though:

which are already deprecated too.

that is not quite right, hook_requirements isn't deprecated yet.

RE: #4 @nicxvan I cannot see where that text appears? Has it been corrected now?

It's in the issue summary.

Made 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'?

RE: #6 updated IS.

I don't follow your changes to the issue summary.

What do you mean by

all these are already deprecated.

nothing there is deprecated.

And again

There is one in xmlsitemap, which calls its own requirements hooks. However it is not yet deprecated.

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.

All 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.

@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?

Yes, 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.

There is one in xmlsitemap, which calls its own requirements hooks. However it is not yet deprecated.

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.

RE: #10 I also edited the CR.

@nixon RE:10 The source of my confusion was the idea of 'Include files'. If it had said '.inc' files that makes immediate sense to me.

.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.

The new CR draft is good. RTBTC.

The 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.

catch → closed merge request !12751

Committed/pushed to 11.x, thanks! Published the CR.