Allow to skip OOP hooks and services for modules that are not installed

Declaring all your hooks as extensions of Hook would definitely solve this as we can then support a "source" property on the base Hook class that we can scan for as described in the IS. The only drawback I see is we'd be creating tons of one-of attributes. They would be contained to namespaces that have Hook in them, so I suppose that could be fine.

So maybe add an optional "source" property to Hook that we can specify in the Hook constructor as a named argument and then extensions of Hook can prefill it for you? The default value could be empty or "drupal" to indicate that it's a core hook.

Anyways, big +1 to the idea. We shouldn't add classes to the container if we know they will never be called.

There is a related issue I can't find at the moment. I think this needs to be solved with a condition, I'm wondering if it should be a separate attribute or a parameter. Both options have pros and cons.

But either way, rather than a simple module exists it could be a callback.

There would be some serious restrictions, but you could conditionally add the hooks based on that result.

For example define the hook only if another module does not exist.

I think modules being installed or not is by far the most common use case for this and I'd rather focus on that. callbacks imply dynamic logic like something based on settings, but then you have to rebuild the container if that changes. Seems more complex to support and define. Could also design the attribute in a way that allows to extend it later?

#[ConditionalRegister(moduleExists: 'config_translation')]

And then we could add a callback parameter if there is a valid use case for that later.

Adding an attribute to the class that skips it for registration if the dependency is unmet seems like a good solution to the optional constructor argument problem. If we know that the dependency needs to exist for our hooks to be registered as a service, we can safely use the dependency's services with autowiring.

This would be useful for 🐛 Deleted menus are not removed from content type config Active .

Also useful for 📌 Deprecate node_reindex_node_search() and move reindexing logic to a hook Active

Can I suggest we call the attribute something like "DependsOn"?

It might not necessarily be a hook that is "owned" by the module, maybe we're implementing a hook_ENTITY_TYPE_update for an entity type defined by that module or something like that.

Can I suggest we call the attribute something like "DependsOn"?

I actually proposed a Dependencies attribute in 📌 Use alternate parsing method for attribute-based plugin discovery to avoid errors Active to solve a similar issue for plugin discovery (with a bigger problem there being that the attribute couldn't be parsed by Reflection when necessary modules aren't installed), so I think either of those is more readable than HookBy.

I think it also makes sense for the attribute to take an array of modules as the parameter, or a string|array union, just in case there are multiple modules that need to be installed for the hook.

Did you see my updated name proposal in #5?

No strong opinion on what we name it exactly, agreed that HookBy isn't great, that was just the first thing I thought when creating the issue.

I think the new name is OK if we want to be flexible for possible new use cases. Or at least, I don't have a better suggestion.

That said, I keeping this simple might be best. Allowing for callables introduces the possibility of some developer trying to do something overly complex in their project and using code from other modules in the callable. Or someone will try to instantiate a service in the callable, even though the attribute is parsed at compile time and doing that is less than ideal.

As it is, implementing the attribute should be careful not to instantiate the module handler to check for modules being installed and try checking against the container.namespaces parameter instead. Example of that being done is in 📌 Add a fallback classloader that can handle missing traits Active .

There's also a container.modules parameter that I'd assume is available at the same time and might be easier to use? That's the pattern I've used in ServiceProvider::alter() when defining services depending on another module:

    $modules = $container->getParameter('container.modules');
    if (isset($modules['hal'])) {
    }

From \Drupal\entity_reference_revisions\EntityReferenceRevisionsServiceProvider

Also, this proposal is currently specifically for HookCollectorPass which already operates with that exact list and has it available as $this->modules.

Yes we would use that list when the remove hook drop happens too.

Oh, nice! I'd forgotten about that.

We can workshop the name.

I think we want to discuss if we follow the remove and reorder attribute pattern and require identifiers.

Pros you can add this attribute on any method and add the condition for other modules.
Cons, you are almost always adding this for your own implementation so it feels duplicate.

If we exclude adding the identifier as a parameter then it has to refer to the method it is in for all hooks using that method.

The thing I want to avoid is this attribute referencing other hook attributes.

That was the first approach we took in hook ordering and it was an absolute mess and that was before the order groups attempt.

Honestly I think identifiers is an optional argument after the modules array.

If we exclude adding the identifier as a parameter then it has to refer to the method it is in for all hooks using that method.

If the code inside the method requires modules to be installed, then I'm not sure what difference it makes which hook(s) the method is implementing. If somehow the method has conditionals where it can run without modules in a certain context/hook, but not without in others, either the code should be separated into multiple methods, or the the attribute should not be added at all.

I think the attribute should work pretty straightforwardly: if the attribute is on a method, then if the required modules are not installed, then the method is not registered as an implementation for any hook.

If the attribute is on a class (up for discussion whether this should be supported), then no method in that class should be registered as an implementation for any hook if the required modules are not installed.

Are there any use cases that this wouldn't work for?

Yes you're right, let's keep it an array of modules that are required to register the hook.

How about

HookDependsOn