🇺🇸United States @nicxvan

Thank you for the detailed review, I'm going to assign this to myself for now since I want to take care of the feedback.

Spent some time thinking about it and pulled an example from this MR and clarified a bit.

It's possible that it needs some additional examples, but I'm not sure that will make it clearer.

I think this would be good to get in sooner so we can show examples.

With OOP hooks I'm seeing more and more people including myself inadvertently add [$this, 'callback']

Is this a beta target change too?

nicxvan → changed the visibility of the branch 3544715-keyvalue-theme-with-invoke to hidden.

nicxvan → changed the visibility of the branch 3544715-keyvalue-theme-with-invoke to hidden.

This is green again, performance tests being chained took some churn since I cannot run them locally for some reason.

I'd love to get preprocess in here too, but got snagged up there.
We may need to address hook_theme in themes too as a gap.

I split out HookCollector to make it easier to integrate HookCollectorPass and ThemeHookCollectorPass with common methods.

I intentionally did not update HookCollectorPass though to reduce the number of changes.

I created a follow up for that.

There is a related issue from the extension side.

There was a duplicate here too #2863438: Allow modules to declare the list of files it ships with that should be publicly accessible →

I think these are great, I wonder if we can document these rector rules for contrib somewhere central.

I think this is ready.

I spent quite a lot of time thinking through this and I think this really is the best way.

For bc they can use the new attribute.

Deprecations are good and I think 13 is the right target.

I wonder if we should create a parent class for the attribute so we can simplify the check since we likely need many more of these.

Whoops, didn't mean too rtbc before that question was addressed.

I think this is almost ready then, one last question.

I see you are only deprecating if it's not already deprecated. At first I thought this was a nice little optimization, but can we end up in a bit of a bind.

E.g. we are removing this support in 12 someone could deprecate their template preprocess and try adding support for 12 and this would not notify them.

Without that check it's a double deprecation, but that might be preferable unless I'm missing something.

Other than that.
Took another look, all deprecations line up.

We usually don't test deprecation pathways but this is unique enough it is warranted.

Everything else is just a couple that were still in the include file or were using the deprecated fallback.

50 convinced me we don't need a test for the third pathway.

Fair enough, I'll review again without it.

I rebased.

I still have to finish consolidating the hook collector changes, registry and performance tests.

I will continue working on this.

Thanks everyone this is in directly from the parent!

I think this is almost ready, just one question on testing the last deprecation path.

I think that makes sense.

This could do with a fresh benchmark.

Issue summary was out of date so I updated it.

I created a branch from the tag before the rebase so we can see if tests pass.

I will take a look later at rebasing myself since there are likely a lot of gnarly conflicts.

I'll take a look at the failing tests.

Thank you that was the one!

There are a couple of comments on the MR.

Great idea! Can you post a screenshot or two?

This is a lot of code too, any chance we can get a couple of tests?

Have a high level overview and noted a couple things that need to be addressed.

@donquixote pulled in some changes from the spin off issue.

I reviewed them and they are good, mostly comment clarifications and static declarations.

There was an annotation in one of the tests I replaced with the group attribute and rebased again to pull in the fix for the broken HEAD test.

This broke head there is an annotation.

The reverse is also true, and the other one has a few other issues dependent on it so I really think this should be postponed on that one.

I confirmed that the feedback nod_ gave has been addressed.

I recorded the extension list and permissions list, but you can't upload webm.

@angel_devoeted that is certainly an option as well.

I've had a couple of suggestions to create a specific non runtime attribute for finding these implementations (e.g. #[Implements] or #[DynamicHook])

I have the following reservations about that approach:

1 people already are inconsistent about adding the comments
2 do we only want it for dynamic or all, cause just dynamic is two ways to grep for hooks
3 people already do the comments wrong for dynamic e.g. people will do [DynamicHook('custom_entity_name_presave')] which defeats the purpose
4 This is an extra attribute that is looped over during collection even though it's not used
5 this will create two ways to search depending on if it's dynamic or not

I think number 4 is the biggest reason not to do it this way.

The truth is you can search for dynamic hooks even using the #[Hook] attribute with some regex, maybe we just need to build that and document it for a couple of common hooks so people can easily search.

Let's postpone this on the separate hooks issue since that is now rtbc and far more important.

That will also require a reroll here.

Great idea! This is a duplicate of an rtbc issue though!

Thanks!

Postponing because I agree with 4 and 5 and in 6 years there doesn't seem to be any new use cases identified.

I'll leave this open for a bit in case I'm missing something.

Can someone confirm this is still happening?

I'm going to postpone this for the questions in 14.

Honestly I think we should close this as won't fix.

I agree this page needs work, but there are multiple issues taking about many alternative ideas.

I don't think this version makes the page more usable.

Having paragraphs come after workspaces I think is far more confusing than the current situation.

I'm not sure this is something core wants to do, it would be pretty straightforward in contrib by implementing hook_modules_installed.

Ink going to postpone this for a bit before closing in case there's a case in not thinking of where contrib would not work.

I'd love ideas for how to optimize this.

When testing an issue I output the number of scans for a single kernel test and it was dozens.

This needs some updates for what is mentioned in 9.

What is mentioned in the issue summary no longer describes the code flow.

I'm wondering if we close this and open a new issue targeting what you mentioned @mile23

Is this a duplicate? I can't find the other issue at the moment.

We can likely close this when separate hooks from event dispatcher lands.

Adding related issue that can be closed when this lands.

Yes, sorry, I meant are there other test modules that define the dependency incorrectly.

For example: core/modules/system/tests/themes/test_theme_mixed_module_dependencies/test_theme_mixed_module_dependencies.info.yml
doesn't have drupal:help

That being said I did a quick audit and here are the ones we need to update still:

• core/modules/system/tests/themes/test_theme_mixed_module_dependencies/test_theme_mixed_module_dependencies.info.yml
• core/modules/system/tests/themes/test_theme_depending_on_nonexisting_module/test_theme_depending_on_nonexisting_module.info.yml
• core/modules/system/tests/themes/test_theme_depending_on_modules/test_theme_depending_on_modules.info.yml
• core/modules/system/tests/themes/test_theme_depending_on_constrained_modules/test_theme_depending_on_constrained_modules.info.yml
• core/modules/package_manager/tests/modules/package_manager_test_validation/package_manager_test_validation.info.yml
• core/modules/package_manager/tests/modules/package_manager_test_event_logger/package_manager_test_event_logger.info.yml
• core/modules/package_manager/tests/modules/package_manager_test_api/package_manager_test_api.info.yml
• core/modules/package_manager/tests/modules/package_manager_bypass/package_manager_bypass.info.yml
• core/modules/navigation/modules/navigation_top_bar/navigation_top_bar.info.yml

I think with those comments addressed I think this is ready!

I think that was a good change with the optimization pass.

See #3506930-76: Separate hooks from events → for a description of the contributions I've made, mostly updating performance tests.

Excited to get this one in so we can get the theme hook one built on top!

I think we can close this, the related issue reduces it to a stub only used for targeting with remove and reorder.

It's no longer registered in the container once that lands.

I think it's safe to close this now, we really don't want the added complexity and if a particular use case warrants it a specific issue will also be warranted.

I'm going to postpone this for now.

The protect browser initiative agreed with my assessment that is mostly recipes.

I think eclipsegc had a great take, but that was also 14 years ago and I'm not sure how we turn that into an actionable plan.

Postponing this for 18.

But also I tend to agree with 1.

One issue is if we check primary module hook requirements before installing the dependents we can easily get modules that are uninstallable.

If the module's installed requirements checks for something in the dependent module, that would necessarily fail unless the user installs it manually first.

But then you just get back to the scenario in the issue summary if another requirement fails where the dependent module is installed but not the one you want.

I think this should be closed as works as designed unless a clean solution to the scenario I raised is proposed.

I'm wondering if we close this and open a targeted issue.

The original issue and most of the discussion is around adding the machine name and allowing search by machine name, both exist in core.

There has been a lot of back and forth about the location, but I really don't think the ui is the place for it and while the issue summary seems to have been updated for that the title has not and I do not see a strong consensus asking for it, even the title seems to imply it's not necessary.

I'm going to mark this as postponed for now to give a chance for response.

If there is a strong desire for just the location I think a new issue is warranted and related here.

There is a new runtime requirements hook this would be moved to.

This needs a recollection but would probably be nice to get in.

Alex in slack for the two groups in 1.

This needs an mr with the concerns 2, 3 , and 4 addressed.

So, by defaulting to whatever is first in the array, are we perhaps overwriting a good label format with a bad one?

Possibly, but it's better than multiple groups.
Simple title or sentence carrying was ruled out.

Should we have additional functionality to Title case the label if there are multiple versions that differ only by casing?

I feel strongly we should not, that would potentially be a lot of shuffling and guessing.

And what if the labels differ by more than their casing? What if they legitimately belong in different groups and it's the package key that's completely wrong?

I'm not sure how that could arise or if it's a concern to worry about. If you have an examples that would help.

I think I'll mark this as won't fix for now, if someone has a use case we can reopen.

I'm wondering if this is related: 🌱 Formalize a structure for static assets in modules and themes Active

Did you check other test modules?

nicxvan → created an issue.

nicxvan → created an issue.

Rebase was clean, I had to update the new HCP test to match the new hook information format.

nicxvan → changed the visibility of the branch 3544715-convert-theme-hooks-keyvalue to active.

nicxvan → changed the visibility of the branch 3544715-convert-theme-hooks-keyvalue to hidden.

I can take care of that!

I'm not sure how test only is passing, it's a service... maybe the bot set up the test wrong?

Fair enough!

For some reason I thought there were more services.

Good point @eric_a!

That only affects dynamic hooks though.

I wonder if we should work on the policy to allow excluding comments on hook implementations, or do we want to add comments that actually explain what the hook implementation is doing. I have always felt like we should be explaining the point of them.

I think we can mark this now. Looks good to me and @alexpott signed off too.

I've updated credit.

I think I've address all of your feedback except one comment I think should be in a follow up.

Test only fails as expected, thanks for the test case!

I also reviewed this and it looks good too.

I'm no longer sure what to do with this, I've certainly seen this working with modules without an upgrade, just switching branches.

I think this might be an APC issue, and maybe the new container key handles it better, I am also not able to reproduce it from a fresh install upgraded from 10.5 to 11.2.

I think I'll close this and can't reproduce.

If someone else can come up with steps to reproduce we can reopen this and explore further.

Either way the solution to wrap the check in file_exists is not the answer since that is called many times, we want to clear the defunct .module file.

Thanks for the push to try reproducing this myself, I missed the fact that you had tried too, that's three tests to attempt to reproduce that can't so I think we can close this for now.

We can remove that todo for 🐛 ModuleHandler::add() can prevent existing implementations from being added Active , I self applied a suggestion for that as well as one more comment update for a property you renamed.

I looked through all instances of listener and I think the remaining ones look fine beyond the comment I updated.

I think the BC break is ok and called out in the CR (wonderful improvements there) if we want to look at attempting BC we can, but I think this has too many changes. I've added a note to the remaining tasks.

I think the bot is confused, I'm not going to add the tag yet, but I'll set this back to needs review. I did rebase, but it's up to date.

I also updated the theme hook approach to use this approach so I have a good understanding.

I have contributed to this issue but I think I can still mark this RTBC since my contributions have been limited to the following.
1. The two comments mentioned here
2. A few test updates for performance tests
3. A couple of tests that needed the new parameter added
4. A strict comparison == -> ===
5. Two test cases for an edge case I was worried would be dropped

I've been reviewing this deeply in both iterations.

I think this is ready, but since we just got this settled I will leave this open for other reviews for a bit and to confirm the bot.

I think pmni is still accurate

I've seen it, I want to see if I can reproduce.

It's actually all constructors that don't need comments, not just ones on hooks: https://www.drupal.org/docs/develop/standards/php/api-documentation-and-... →

Took a quick pass at a CR, it definitely needs work but I wanted high level bullet points to start from.

nicxvan → changed the visibility of the branch 3544715-convert-theme-hooks to hidden.

Got it back to green, gotta handle BC properly and preprocess suggestions for themes, but that might be better done when the other issue lands.

I'm going to leave this in needs work for that, but it can be reviewed by interested parties for the approach.

Moving back to needs work since the event dispatcher took a different direction, I pushed up a very rough and untested version following that pattern.

I still may need to extract the include bit and work on the preprocess for suggestions bit.

I'm going to open an MR to see what we're missing, but I expect it to fail.

Created the follow up here: 📌 Explore deprecating hooks in .inc files Active

nicxvan → created an issue.

I think this is very close, there is one open question on BC I think we might be able to solve.

The other bit is a follow up for the include files, I'll create that in a bit.

I created a tentative CR and we can link the old one to the new one once this gets in.

So SDC's are available at all times if a theme is enabled, even if it's not in the active theme context?

Conceptually this makes sense if someone wants to start working on an MR.

I think this is ready, I had some concerns about the file usage test and token changes, but the current test results were replicated to the new tests and align with the discussions here and in slack.

I wonder about the utility of the file usage test as it stands now, but I can see why resolving that would be out of scope here and the current test will prevent any regressions beyond. We probably need a follow up to untangle the file usage and token tests once this is all settled. I'll ask for them to be created.

nicxvan → created an issue. See original summary → .

Looks right to me, thanks!

Yeah I only just found that hook is procedural only recently the merge to exclude it from conversion hasn't been merged yet.

Those numbers look definitive for the new direction.

I'm going to hide the other branch for now.

I also reviewed the packed order changes and those line up too.

I think there are still a few minor cleanup bits, but this is actually smaller now.
I really don't think it's worth breaking this up further either, while it's a tad large it's scoped for making the container smaller and extracting the event dispatcher.

nicxvan → changed the visibility of the branch 3506930-hookcollectorpass-registers-event to hidden.

Took a pass at updating the issue summary with the new approach.

I'm considering hiding the original approach, but might leave it up for a bit.

Rebased this, I'll give it another review in a bit

nicxvan → changed the visibility of the branch 11.x to hidden.

I think this is good, @fathershawn pointed out I missed this was already added to FormBase

I figured it out!

When we copied the hook class we forgot to clear out the implementations from the test module.

That means we added this implementation:

  /**
   * Implements hook_markdown_easy_environment_modify().
   */
  #[Hook('markdown_easy_environment_modify')]
  public function markdownEasyEnvironmentModify(Environment &$environment): void {
    $environment->addExtension(new StrikethroughExtension());
  }

Removing that hook and associated use statement made the tests green again.