Modules often implement hooks of other modules they don't depend on as optional integrations. That works basically out of the box with legacy hooks and also new OOP hooks. But they are still registered and their class added to the container when we know they will never be called. For example, user module implements views hooks.
This becomes a bit more complicated if your hook also requires an injected dependency of that module. Because the class is added as a service, it will be invoked and you need to make that dependency optional even though that class and hook actually requires it. And we'll run into that more often when improving Hooks to do proper DI.
And, unlike legacy hooks, where hooks were only discovered and cached when invoked, now we add them all to the container.
If you provide a plugin for a given plugin type owned by another module that isn't installed this works as it's never discovered.
A good example are the node search integration hooks. Now all bundled in NodeSearchHooks. See MR for other core examples.
We can't figure this out automatically, we don't have enough knowledge about hooks and who owns them. Also, sometimes the hook does exist, but what it does is only needed when another module is available. A good example for this are again the NodeSearchHooks. There are comment hooks, a hook cron. A hook cron would always run, and until now, we had to add a runtime check to make sure the search module is actually installed.
A new attribute is added, HookDependsOnModule, this has a single parameter for the module name.
#[Hook('config_translation_info_alter')]
#[HookDependsOnModule('config_translation')
public function configTranslationInfoAlter(array &$info): void {
We can check for that, and if config_translation isn't in the container, we skip it. And if a service class has no hooks, we also don't add it to the container.
The attribute can be added multiple times, if any dependency is not available, it is skipped. The attribute can be added both on a specific method, and on a class. the result is the same. Hook classes are only autoregistered as a service if they have at least one active hook method.
The reason a specific attribute class that only works for modules and not something more generic is to prevent future BC issues. Adding a new property is a fatal error on any core version that does not have that. That would mean that relying on this new attribute would require contrib projects to depend on that specific core version. See https://3v4l.org/31a3W.
If we do have a new use case, we can always add a new attribute class. We have existing examples for this in the hook system, such as LegayModuleImplementsAlter which was added in 11.2 and 11.3 will hopefully add one for requirements hooks in #3490846: Deprecate hook_requirements.
| Comment | File | Size | Author |
|---|
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
Comments
Comment #2
kristiaanvandeneyndeDeclaring 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.
Comment #3
quietone commentedComment #4
nicxvan commentedThere 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.
Edit, it was the decouple block content from the block module issue.
Still can't link it because I'm on my phone.
Comment #5
berdirI 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?
And then we could add a callback parameter if there is a valid use case for that later.
Comment #6
kristiaanvandeneyndeAdding 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.
Comment #7
catchThis would be useful for #3337882: Deleted menus are not removed from content type config.
Comment #8
acbramley commentedAlso useful for #3533612: Deprecate node_reindex_node_search() and move reindexing logic to a hook
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.
Comment #9
godotislateI actually proposed a
Dependenciesattribute in #3490322: Use nickic/php-parser for attribute-based plugin discovery to avoid errors 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|arrayunion, just in case there are multiple modules that need to be installed for the hook.Comment #10
berdirDid 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.
Comment #11
godotislateI 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.namespacesparameter instead. Example of that being done is in #3502913: Add a fallback classloader that can handle missing traits for attribute discovery.Comment #12
berdirThere'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:
From \Drupal\entity_reference_revisions\EntityReferenceRevisionsServiceProvider
Comment #13
berdirAlso, this proposal is currently specifically for HookCollectorPass which already operates with that exact list and has it available as $this->modules.
Comment #14
nicxvan commentedYes we would use that list when the remove hook drop happens too
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...
And
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...
Comment #15
godotislateOh, nice! I'd forgotten about that.
Comment #16
nicxvan commentedWe 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.
Comment #17
godotislateIf 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?
Comment #18
nicxvan commentedYes you're right, let's keep it an array of modules that are required to register the hook.
How about
HookDependsOn
Comment #20
berdirStarted something here. Supports both class attributes (had to extend the file cache info a bit and methods. Had to add another loop for that as we want to check HookDependsOn first.
Added it to NodeSearchHooks, which allows to make the argument there non-optional and also NodeHelpHooks on the method, that's also sufficient to skip the class from being registered as only classes with at least one identified hook are registered
Comment #21
berdirAs mentioned in #3506930: Separate hooks from events, hook_help() is an extremely common hook that is from an optional module. It would be a little bit tedious to add this to every single help hook, I wonder if it's worth it to special cases a few hooks like this one and optimize them out by default. If we end up with more examples like NodeHelpHook then we might be able to skip dozens of services from being added to the container.
Comment #22
nicxvan commentedThank you, this one has been on my mind.
Now that I see it I have a pretty strong feeling we shouldn't do it this way.
The extra loop is not great and we're again affecting implementations from a second location that needs a loop. We ran into this with the first pass of the ordering issue if you recall.
I think we should look at adding a parameter since it will just be an array I don't think we'll hit the same bc wall we did with ordering since there is no object. (We need to verify)
That keeps the condition local to the implementor.
Which brings me to the second point:
Initially I loved this line of thought, but can we reliably do that? What if someone else invokes help? They won't be available.
If we're OK with that then it's a nice optimization, but I'm concerned it works break things.
Maybe we add a setting to ignore conditions in both cases? Then people that set it can call remove hook on the implementations they still want gone?
Either way I think maybe a parameter on the hook attribute is the way to go.
Comment #23
berdirWhat exactly do you consider the problem with the extra loop (technically two of them, but there will be very few class attributes).
I can not imagine that it is an issue in regards to performance. An extra loop with an instance of check is going to be super, super fast compared to everything else that's going on here. I did consider sorting the attributes so that depends definitions are always first, but I think it's not worth it.
Adding it to Hook would not allow to put it on the class as I did on NodeSearchHooks, so we'd need to duplicate it on every single hook there. Also, I'm pretty sure even without a class, this would be a BC problem. See https://3v4l.org/31a3W, using a named parameter that doesn't exist is a fatal error.
Comment #24
nicxvan commentedThank you for confirming my recollections were wrong, in that case the way you did it is correct.
My concern with the loops btw is what happened with the original ordering approach that it's just an extra loop you have to keep in your mental model.
I'm also not a fan of having these on classes though since hooks can be there we need to support that, I kind of wish we didn't add that in the original hook, but that is very much just personal preference cause then the method would always be defined and tightly coupled.
Let me review this again now that you've validated the approach is correct again.
Comment #25
nicxvan commentedA couple of questions in the mr.
Comment #26
berdir> Initially I loved this line of thought, but can we reliably do that? What if someone else incomes help? They won't be available.
Yes, I was wondering about that too, what if someone does a better_help module in contrib that wants to use those hooks. The NodeHelpHooks change was more there to test the system. I think we want to add dedicated test coverage about this instead in a test module. Setting to needs work for that.
We could do a setting, that seems like an interesting idea, but also not sure it's actually worth it. A tiny amount sites will bother with that level of micro-optimization. Id rather work on my other ideas to move the metadata/hook list "services" out of the container and experiment with more efficient caching strategies, that would benefit everyone without custom settings.
Comment #27
berdirAdded tests, the one remaining question for me is the class name and if we should add Module to that since adding more parameters would be a pain for BC anyway.
Comment #28
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #29
nicxvan commentedNot sure what the bot is complaining about. I think we do rename the attribute.
We can always add the skip conditions setting later.
Comment #30
godotislateLooks like an info.yml file got changed to 755 in the MR ("File mode changed from 100644 to 100755"):
check failed: file core/modules/user/tests/modules/user_hooks_test/user_hooks_test.info.yml should not be executableComment #31
nicxvan commentedAh I already had a comment on the mr about that, I didn't see that in the bot file thanks.
Comment #32
nicxvan commentedWhy does that file permission change? I have a recollection of this happening for me too but always just one file.
Comment #33
ghost of drupal pastThese days I dislike commenting on core issues and I dislike controversy even more. I read the issue but I absolutely can't find why a constructor argument was not considered. I might have missed something -- even the original issue summary (yes, I read that too) wanted a separate attribute but why? Of course it will work but I do not see how it's a separate thing when HookBy can't appear without Hook. To me that says they shouldn't be separate.
Of course, what do I know.
Edit: ncixvan told me it's because of named parameters and backwards compatibility -- calling with a nonexistent named parameter is a fatal error. Major bummer! But, separate attribute it is.
Comment #34
catchThere are possible workarounds:
1. Calling the module (if not project) 'help'
2. Shipping an empty 'help' module along with the main module to active the hooks.
Both of these are not exactly clean.
Comment #35
berdirThe file permissions change because the HookCollectorPassTest writes that file. I did run that locally to test my new method there.
@catch: Yeah, either way, it seems fairly controversial. It was a spontaneous idea, but I think there are more worthwhile things to explore for now, specifically my ideas in #3506930: Separate hooks from events. If we extend that first first step with a basic cache collector then we will just never add help into the data loaded on every request, leaves just the services. And there it only makes a difference for hook classes that only have this method.
Comment #36
catchShould we postpone it on that issue? I'll try to review the blocker for that one too.
Comment #37
berdirI don't think that's necessary (postponing). The interesting part here is the NodeSearchHooks use case. An explicit declaration of module A implementing hooks for module B, which are only needed when module C is installed. For example, node module does things with comments (and nodes) which is only needed when the search module is enabled. That's useful right now and does not require anything else.
While it helps to reduce the size of the hook info and autodiscovered services a little bit, it's mostly useful to have clearer and easier dependencies. NodeSearchHooks currently has an optional injected DI for SearchIndexInterface. It's only optional in case the search module isn't enabled, but if it is, we expect it's there or the functionality wouldn't work.
We can make it required now because the service only exists when search is enabled.
It's also a tiny runtime improvement in this case, because unlike help and config translation situations, these are hooks that actually are invoked and then just don't really do anything.
Which reminds me that we can simplify \Drupal\node\Hook\NodeSearchHooks::cron a bit and remove the module check, because we can rely on it only being called if search is enabled now.
I'll try to update the issue summary a bit as well and also explain those use cases and why you'd want to do it in the CR.
Comment #38
berdirI knew there was another case that we had but I misrembered it as date/views related, it's actually file/views related, core/modules/file/src/Hook/FileViewsHooks.php and maybe other such views hooks can add a dependency on the views module so we can make the dependency there non-optional.
also the prviously mentioned \Drupal\menu_ui\Hook\MenuUiHooks::menuDelete, we can drop the node_type check there.
Both are pretty theoretical and mostly for tests, not many sites don't have node or views installed I guess.
Comment #39
nicxvan commentedhonestly even help is pretty theoretical, most sites use standard and in my experience leave it installed, this could be a reason to start uninstalling it on most sites though.
Comment #40
berdirRenamed the class, added some more usages, created a CR.
Comment #41
nicxvan commentedTwo minor doc updates and a parameter rename I'm not super sold on, but might be nice to match other hook attributes.
Comment #42
nicxvan commentedI think this is ready then.
If someone else wants the parameter renamed I left suggestions for all of the usages, alternatively, they can kick it back and we can do it locally with the ide.
The only remaining concern is if we should allow a way to bypass the skip.
However, the more I think about it the more I think we shouldn't a skip for HookDependsOnModule.
The implementor knows their code and should have the final say.
In some cases it might be safe like help where another module like advanced help is just invoking the hook.
I'm others like node search hooks it would still break because a dependency is missing entirely.
Comment #43
astonvictor commentedseems like the pipeline has some errors to resolve
Comment #44
berdirThose are random fails.
Comment #45
astonvictor commentedok, thanks for the clarification
Comment #46
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #47
berdirRebased, tests still pass, so should be OK.
Comment #48
eric_a commented#[HookDependsOnModule()]is a wonderful feature, though for a second the attribute name made me wonder if the intent was to stop the module from being uninstalled, which is how dependency management works in some other Drupal subsystems.But what else? Could now only come up with:
#[HookForModule()]#[HookActiveIfModule()]Comment #49
kristiaanvandeneyndeI feel like we might be shooting ourselves in the foot here with the naming. If we are deliberately going forward with the HookDependsOnModule name, we are ruling out that anything else may specify a hook in the future, e.g. a theme.
I'd rather see a more agnostic name such as HookWithDependency, where we keep the param called "name". If at any point in the future we decide to support hooks anywhere other than modules, we can keep the attributes as they are, perhaps introducing a second "type" parameter which defaults to "module" for BC reasons, and all we'd need to do is change the collector pass to check for more than just "$this->modules".
Either way, just my 2c. I've learned the hard way that naming things too specifically tends to bite you in the ass at some point.
Comment #50
berdirWe discussed this. We decided specifically for this for BC and a new feature would be a new attribute. Having a non-existing attribute class is just fine. Providing a named parameter to an existing attribute class that doesn't exist is a fatal error (this will be fun on plugin attributes too).
Comment #51
godotislateI think there have been some struggles with PHPStan in contrib projects trying to support multiple versions, where adding an ignore a non-existing attribute class for the older version causes PHPStan to complain about an unnecessary ignore in the newer version, so it's not completely painless. But yes, that's nothing compared to fatal errors.
Off-topic, but speaking of plugin attributes, I wonder if being forward compatible with future named parameters is a good reason to go forward with allowing arbitrary parameters as a solution for #3443882: Allow attribute-based plugins to discover supplemental attributes to set additional properties.
Comment #52
kristiaanvandeneyndeRe #50, I saw #23 and https://3v4l.org/31a3W.
What I had in mind was this: https://3v4l.org/egZQ2
We start with "name" and add "type" with a default value of "module" later if we ever get to that. No fatal error, even if people are already using the attribute.
Edit: So if we never get to supporting hooks outside of modules, worst case scenario we're left with HookWithDependency as a name. But if we do start supporting other sources for hooks, then our attribute name still makes sense. If at that point for some technical limitation we'd need to manually specify the source, all we'd need to do is introduce the "type" parameter and we'd be golden. Although we might even find a way to avoid that if we put our minds to it.
But at least we wouldn't have an attribute indicating modules that actually also supports, say, themes.
Comment #53
nicxvan commentedThe issue is that we are still guessing at possible future parameters and those fatals are problematic. I'm on the fence here, but I think we really do want to avoid adding potential parameters to just handle possible future cases.
The thing is we can always add a HookDependsOnTheme, then we can add a base attribute to simplify discovery. It makes it easier to implement too
Comment #54
kristiaanvandeneyndeBut I'm not saying we should add any new params? And what I'm suggesting won't cause any fatals.
All I'm saying is let's not marry ourselves to the scope of modules in the name of the new attribute. If we don't widen the scope, so be it, if we do then we don't have to rename the attribute.
Essentially:
#[HookWithDependency(name: 'some_module')]If we never widen the scope then that's all it will ever be. If we do widen the scope, then we can have:
#[HookWithDependency(name: 'some_module')]#[HookWithDependency(name: 'some_theme', type: 'theme')]#[HookWithDependency(name: 'some_module', type: 'module')]The first one still working and the last one not being necessary because the default value of "type" would be "module". So it would be fully BC. Again, if we ever even get to that.
But at least we won't have:
#[HookDependsOnModule(name: 'some_theme', type: 'theme')]then.Comment #55
nicxvan commentedI was talking about
typeit is a parameter we don't have, and we're speculatively adding it here.I'm not sure we should add something just in case we need it since it won't do anything.
Comment #56
berdir> But at least we won't have: #[HookDependsOnModule(name: 'some_theme', type: 'theme')] then.
No, but we don't need to. We can always add HookDependsOnTheme. Or HookDependsOnCallback, or HookDependsOnFullMoon or whatever other use case that we didn't foresee where name/type doesn't fit. We can have multiple purpose-built attribute classes, it doesn't need to be just one.
Comment #57
berdirAlso, note that HookDependsOnTheme does not and will never make sense for how this is implemented. This check is made during discovery, these hooks do not exist then at all, including their Hook "service". While we will know if a theme is *installed* during container build once #3544715: Add oop support to hooks currently supported by themes lands, whether or not a theme is active is a runtime thing. And I think installed or not doesn't really matter for a theme, it's whether it's active or not, so you need to do a runtime check for that.
Comment #58
kristiaanvandeneyndeThat is not what I am saying? Nowhere did I write we should already add "type" now. I even said there's a good chance we will never add it at all.
I also considered that, but that just feels wrong? You'd have multiple attributes with the same meaning aside from a small implementation detail.
We don't go and add EntityType, EntityTypeWithOwner, EntityTypeWithOwnerAndPublished, EntityTypeWithOwnerAndPublishedAndRevision, etc. to core either. In that example we do have two attributes (ConfigEntityType and ContentEntityType), but they actually are wildly different behavior and somewhat different attribute parameters.
Either way, I've said my peace. The way core is evolving nowadays, I don't think it's unimaginable that one day something other than a module can call a hook. I'd hate to be stuck with an attribute that is named too narrowly when that day comes. I've learned the hard way that naming things too specifically because you never thought you'd support more than what is currently the case can really bite you in the ass.
MR looks fine otherwise, my only concern is the name of the attribute.
Comment #59
berdirI updated the issue summary to better explain this, it was very updated. If we don't add type now, it will be very hard to add it in the future, see the code snippet I linked. Unlike services, method calls and so on, attributes can't be conditional.
We already have multiple attributes like this in the hook system, And we do have in fact ConfigEntityType and ContentEntityType for entity types. And are considering to support it for plugins too to avoid very large array structures and possibly BC issues like this.
Comment #60
kristiaanvandeneyndeLook, I'm fine with keeping the name as is. I've raised my objections and so be it.
But you absolutely can add new named parameters to existing attributes, I've added my own https://3v4l.org/egZQ2 to prove this. In a world where people were already using the attribute with "name", you absolutely can add an optional "type" without any BC issues later on.
In what world would someone download Drupal 1x.x, see that there's now a "type" named argument, use said argument and not bump the minimum required version for their module to whatever Drupal version they just saw the new named argument in? That does not make sense.
And any old code using only the "name" named argument would still work on said newer version of Drupal.
I really don't get where this insistence comes from that we cannot add named arguments to existing attributes. You absolutely can, provided you make them optional.
Edit: I know it's not intentional, but I'm starting to feel gaslighted here 😅.
Comment #61
catchAnother use-case for this one: #3562753: History module triggers ServiceNotFoundException for comment.manager when Comment module is not enabled. Could be done in a follow-up. Still haven't reviewed the MR here.
Comment #62
berdirRe #60: It is not my intent to gaslight you and I don't understand why we so completely disagree on this one:
But that's exactly the issue. For two reasons:
a) There is a good chance that you are not aware that type only exists since for example 11.4, and the class exists since 11.1. The docs will likely not tell you that, because I've never seen a constructor argument document in which version it was introduced. And with autocomplete, code generation and what not, you'll likely never read the docs even if that would be there.
b) For contrib, you can't just write code that works in the most recent core version. That's fine for custom, but for contrib, the core LTS approach now means that you either have to support 3 major versions (10, 11 and 12) in one code base or maintain multiple minor/major versions. That's hard. See https://www.drupal.org/node/3493962, you're welcome to explain to me how to have a weighted order hook in a contrib module using that order attribute that works in D10, 11.1, 11.2 and 12.0. Because I honestly don't. In this case, there are some alternatives, such the alternative attributes or a version constraint like
^10.4 || ^11.2 || ^12.0, that's doable for this case, but in general, if it's for a version you still want to support (lets say D10.4 would have added Hook but order is only in D11) it's just straight impossible.Re #61: I updated HistoryTokenHooks to use the new attribute and also added DI. Happy to show off more examples how handy this is.
Comment #63
berdirI removed HistoryTokensHooksTest for now, it's no longer possible to test it like that, because CommentManager is now a required constructor argument for that class and it's now skipped entirely. Without the attribute, that is a hard error on the missing service.
One option would be to convert it to a kernel test.
Comment #64
nicxvan commentedAlso for posterity, it's not about adding a named argument.
It's using a named argument where the type is an object and that object class does not exist.
As berdir pointed out it does happen with ordering, and doing it this way allows us to enable a contrib module to support versions older than 11.2 with order parameters.
Comment #65
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #67
acbramley commentedRebased with main, dropping the last 2 commits since history module has been deprecated.
Also renamed the parameter to
$moduleNamewhich I think is clearer than $module or $name. Nothing wrong with being a bit verbose.Comment #68
nicxvan commentedThank you for dusting this off!
I hate to expand scope a tad bit, but I wonder if we should allow themes to also skip a hook if a module doesn't exist. Themes skipping hook implementations if a module isn't enabled seems very useful.
The changes to themeCollectorPass should be almost identical, but we would need to get the module list from the container too.
Also I think the parameter name really should be module to be consistent with #[Hook] HookCollectorPass looks at attribute->module for the hook attribute, I think this should be consistent.
Comment #69
acbramley commentedThat does sound really useful, but perfect for a follow-up?
Didn't see that, makes sense.
Comment #70
berdirWe discussed themes above, that can not work. This is done at build time, we only know at runtime if a specific request has a certain theme active or not.
Comment #71
nicxvan commented@berdir I'm talking about a hook in a theme skipping registration when a module is not available.
That is not runtime.
Comment #72
berdirI see. that would be possible, but I'm not sure if it's worth it? Themes also don't support other extra attributes such as order and complex interdependencies where themes want to alter or preprocess something depending on another module (which is not the hook or render thing of said module) seem quite rare.
I'd wait until we we think there are enough use cases that quantify adding that?
Comment #73
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #74
acbramley commentedComment #75
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. The merge request has merge conflicts and cannot be merged. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #76
acbramley commentedComment #77
nicxvan commentedI think this is ready again and would be good to get in even if just for help hooks.