🇺🇸United States @nicxvan

nicxvan → changed the visibility of the branch 3492282-refactor-away-moduleinstallerinvokeall to hidden.

I'll update the CR again to remove the warning once this is merged.

1. should not change until this issue lands.
2. I will add that now.
3. The behavior is the same if you have converted all hooks or just some hooks, not sure how to lay that out.

Good catch on the media library comment.

I applied all of the other changes too though I'm not sure I would say they should be addressed here, there is an issue quietone created to review the hook conversions and issues like this 📌 Add Dependency Injection Active and 📌 Cleanup hook implementations in the Workspaces module Active

This issue really isn't ready for review yet, we still need the runtime BC layer, but you can see how it works in ckeditor here:

  #[Hook('form_filter_format_form_alter')]
  #[HookOrderGroup(['form_filter_format_add_form_alter', 'form_filter_format_edit_form_alter'])]
  #[HookAfter(['editor', 'media'])]

We kind of have to do this all at once, we're only replacing the one implementation in this issue though.

nicxvan → created an issue.

I looked through, I think the one for rest views is correct on capec, the one I was considering was https://capec.mitre.org/data/definitions/54.html

I did not dig through CWEs, but reading the one posted looks right.

Why not a hook? 📌 [policy, no patch] Encourage hooks over event subscribers Active

Rebased and comments addressed.

Rebase is werd I'll look tomorrow

nicxvan → created an issue.

Yeah I saw the conflict but when I went to rebase it was gone, maybe an issue was reverted I'll fix it and address your comments.

I wouldn't postpone the locale issue

Yeah that will help, we might want to split this and postpone update on that.

Though I do hope the moduleHandler add issue gets merged first! 📌 Investigate ModuleHandler::add Active

📌 Mark Locale and Update as converted Active
📌 Explore inverting checkForProceduralOnlyHooks to prevent them from being added as listeners. Active
✨ Explore adding rule to mark contrib as having been converted Active

nicxvan → created an issue.

nicxvan → created an issue.

nicxvan → created an issue.

nicxvan → made their first commit to this issue’s fork.

Well I committed it instead of suggesting so now I can't review.

Sorry I just realized there is one more, I'll make a suggestion.

It's on the main page: https://www.drupal.org/project/webform →

That was considered, I'll update the meta to add more details, but here is the gist.

We replace the $phase = 'install' portion with this issue.

We replace the $phase = 'update' with 📌 Create class to replace hook_update_requirements Active

And we replace the $phase = 'runtime' with 📌 Create hook_runtime_requirements Active

Once we have all three this covers most cases, but for more complex hook_requirements like system and package_manager with multiple phases we convert in a follow up.

Once core is converted we deprecate hook_requirements being called without LegacyHook being attached.

nicxvan → created an issue.

Why not module_handler?

You can make it a hook, they are now event listeners and give a lot of the same benefits of event subscribers, they are now classes and are autowired.

I also think there's less boilerplate needed among other benefits.

Ah from hook hook info? I thought they had to be top level.

Any other includes would have to be manually included so they would be picked up.

Forgot to mention I also installed drupal standard and umami both with and without drush.

I then enabled every module except deprecated ones (I don't think I ever did that before).

I saw no errors there either beyond the known umami issue for missing hook theme: 🐛 Theme hooks not found warning on Umami install Active which is fixed by 📌 Remove the automatic cron run from the installer Needs work

If you think this needs more manual testing feel free to add that tag and put it back in needs review.

All todos created, they need more detail, but there are suggestions.

nicxvan → created an issue.

nicxvan → created an issue.

nicxvan → created an issue.

nicxvan → created an issue.

nicxvan → created an issue.

nicxvan → created an issue.

I've reviewed this again thoroughly. It's been reviewed by several people and all comments have been addressed.

CR looks good.

I'm going to create issues for the remaining todos and add suggestions, I don't think that should be a blocker though.

nicxvan → created an issue.

The tests were random I reran them.

Thanks!

Ok the three phases are all ready for review.

The install time class needs review for approach.

I tested this against 3491780 which is why I found this old issue.

The issue there is that during install we don't load all theme hooks, but this is an issue during cron because items are rendered.

I think @longwave's comment has been addressed, feel free to put it back in needs work if you think it needs more discussion.

This also fixes 🐛 Theme hooks not found warning on Umami install Active

This is fixed by 📌 Remove the automatic cron run from the installer Needs work

Thanks, that title and component are much better.

I'm using this issue to help Aloyr learn the ropes of contribution using Gitlab

Yes, that is accurate, traversed is a better word.

Oh interesting both approaches kill installer tests.

I think that revert broke some tests.

Do you want me to create an issue in those modules to keep an eye on this?

The only related issue is this: 🐛 Theme hooks not found warning on Umami install Active

Oh that triggers Exception: The theme implementations may not be rendered until all modules are loaded. maybe we can use this to find out how to handle the hook_theme issues too.

So the loadAll was added in 📌 OOP hooks using attributes and event dispatcher Needs review

module_preinstall
modules_installed
module_preuninstall
modules_uninstalled
cache_flush

Previously called: $this->moduleHandler->invokeAll

They were changed to:
protected function invokeAll($hook, $args = []): void {
$this->moduleHandler->loadAll();
foreach ($this->moduleHandler->getModuleList() as $module => $extension) {
$this->invoke($module, $hook, $args);
}
}

and invoke does the function exists.

This was then consolidated to what it is today here: 📌 Fix invocation of hook_module(s)_(pre(un))install(ed) so they support OOP hooks Active and here 📌 Clean up how ModuleInstaller invokes hooks around installing other modules. Active

I bet we can go full circle as you pointed out, and remove:
ModuleInstaller::invokeAll() and just call $this->moduleHandler->invokeAll(
Directly, that should just work.

The only concern I have for the former is if they get split up, but that would be in another issue, this way it's all unified.

Why do we need to call loadAll() every time we go around the loop? Clearer to call it once at the start, with a reason.

That's actually probably worth considering.
In fact I wonder if the loadAll is needed for the uninstall ones at all.
Install maybe especially now that we are considering doing them in batches.

$this->invokeAll

Is called explicitly to call loadAll before calling ModuleInstaller

protected function invokeAll($hook, $args = []): void {
    $this->moduleHandler->loadAll();
    $this->moduleHandler->invokeAll($hook, $args);
  }

We would have to call loadAll before each instance of invokeAll in ModuleInstaller

There is no single call to $this->moduleHandler->invokeAll($hook, $args); outside of ModuleInstaller::invokeAll

$this->invokeAll('module_preinstall', [$module, $sync_status]);
$this->invokeAll('modules_installed', [$modules_installed, $sync_status]);
$this->invokeAll('module_preuninstall', [$module, $sync_status]);
$this->invokeAll('modules_uninstalled', [$module_list, $sync_status]);
$this->invokeAll('cache_flush');

Those 5 hooks are all invoked in other modules than the one being installed and this allows them to be OOP.

The thing that might be confusing is there is also:

ModuleInstaller::invoke()
This is called on the modules being installed and cannot be oop cause they are not loaded yet.

$this->invoke($module, 'update_last_removed')
$this->invoke($module, 'install', [$sync_status])
$this->invoke($module, 'uninstall', [$sync_status])
$this->invoke($module, 'schema')

This uses function_exists

protected function invoke(string $module, string $hook, array $args = []): mixed {
    $function = $module . '_' . $hook;
    return function_exists($function) ? $function(... $args) : NULL;
  }

I'd love to factor that away, but I think that should be handled elsewhere.

nicxvan → created an issue.

nicxvan → created an issue.

One minor suggestion.

I reviewed it thoroughly, I don't know the config installer well enough to sign off, but it looks right to me.

The ModuleInstaller looks right. One minor suggestion.

I think the test coverage for the new flag covers all situations.

I think the plan is to find the leak, but this is still definitely worth doing and helps in the meantime.

For credit purposes @mstrelan helped me figure out the last test failures on slack.

I think I answered your concerns, let me know if you have any further questions.

Tagged 📌 OOP hooks using attributes and event dispatcher Needs review 📌 Convert entity type discovery to PHP attributes Needs review

On my list thanks

It's complicated by the fact that we don't even check the value if it exists we skip procedural.

What about no_procedural_hooks

I vote 2