Document that a module's classes are not available during hook_requirements() 'install' phase

You're talking about hook_requirements() having a phase, not hook_install(), right? Although probably the classes are also not available during hook_install().

Proposed documentation for both hooks: add a paragraph something like this:

During installation (when $phase == 'install'), this hook will be called before the module has been registered with the autoloader, so if you need to load a class from your module, you'll need to either load the class include file directly or register the module with the autoloader. Both drupal_get_path() and module_load_include() will work; use drupal_classloader_register() to register the module.

[and we should leave out the () part in the hook_install() docs, as that only applies to hook_requirements()]

Probably a good novice task?

OK let's only do this for hook_requirements() then.

I'm working on it

I have included the paragraph above, but I'm not sure whether I included it in the right place, please @jhodgdon could you guide me to avoid mistakes?

Thanks!

That looks right to me, thanks!

Not 100% on this one. We avoid registering classes with the autoloader on purpose in case hook_requirements() prevents the module from being installed. What's the case that needed access to classes in the module that provoked this issue?

Hm. That's a bood point in #8 though, it could leave the auto-loader in a bad state. Is there a way to un-register the autoloader afterwards? If not, maybe we should just recommend the module_load_include() method and tell people to load just the class files they need?

What is the status of this issue?
Can I do something?
Thanks!

It seems like a more acceptable patch would just suggest people use module_load_include() to load the files they need, rather than suggesting that they register their module with the autoloader.

I try to fix #12.
Could be a first approach?

Fix #14.

On #15 I have a doubt.
Another issue?

I am updating the patch @dimaro has submitted with the fix on the issue found by @joachim.

Already did the patch against 8.2.x.

Thanks.

Thanks! Looking pretty close...

+++ b/core/lib/Drupal/Core/Extension/module.api.php
@@ -888,6 +888,10 @@ function hook_updater_info_alter(&$updaters) {
+ * from your module, you'll need to either load the class or include the file
+ * directly. Both drupal_get_path() and module_load_include() will work.

Just say "... you'll need to include the class file directly". Otherwise... what does "load the class" mean?

Patch to apply changes mentioned on #20.

Thanks! +1 for RTBC (again).

Changing version to 8.2.x to verify it applies there. Should be committed to all 3 8.x branches. Adding backport tags accordingly, although the "port" should consist of just "apply the same patch" I think.

We're discouraging use of drupal_get_path(), for example in #2351919: Replace uses of drupal_get_path() with __DIR__ where possible. Since the classes are internal to the module, using __DIR__ would be fine.

Also downgrading from major bug since the behaviour itself is by design.

+++ b/core/lib/Drupal/Core/Extension/module.api.php
@@ -888,6 +888,10 @@ function hook_updater_info_alter(&$updaters) {
+ * from your module, you'll need to include the class file directly. Both
+ * drupal_get_path() and module_load_include() will work.

@jhodgdon We only have to replace drupal_get_path() for __DIR__? or it needs some more work?

RE #26, it isn't really major.

RE #25, I think that it doesn't make sense to say "_DIR_ will work" because that is a PHP function so of course it will work. I think it's probably best to just say that module_load_include() will work.

Have made the asked changes.

That looks fine to me, thanks!

Hm... I'm wondering if we should even mention module_load_include() though. Looking at the issue summary for #2351919: Replace uses of drupal_get_path() with __DIR__ where possible it is saying we should not be using

drupal_get_path() in where the simple use of __DIR__ would have been sufficient and more performant.

It seems like this would also be true for module_load_include()? What do you think? Maybe we should just not say anything about module_load_include() either. I don't know... @catch are you reading this?

@jhodgdon Finally, Can I do something more on this issue? or we are waiting to @catch response?

Let's just take out the module_load_include mention sentence.

Apply changes mentioned on #32.

Thanks! This looks fine to me.

Committed 36f657b and pushed to 8.1.x and 8.2.x. Thanks!