Refactor to decorate ModuleHandler instead of HMIA

Moving some description here, from duplicate issue.

In CheckCircularReferencesPass.php line 81:

  Circular reference detected for service "migrate_boost.module_handler", path: "migrate_boost.module_handler -> config.factory -> config.typed -> migrate_boost.module_handler".

Current code produces this error. I tried uninstalling and pre patch, and installing with patch with the same result.

What version of drupal?

Same in the title, drupal 10 and drush 13

Sorry, I applied it against 1.x, disregard my comment

I saw the same problem in 2.x

The MR still has this alter() static method call which doesn't exist any more:

function migrate_boost_module_implements_alter(&$implementations, $hook) {
  MigrateBoost::alter($implementations, $hook);
}

What's the plan for this?

MigrateBoost::isBoostActive() seems to be always not set. And the drush command doesn't seem to be available.
I think the command is missing a services yaml like:

  migrate_boost.commands:
    class: Drupal\migrate_boost\Commands\MigrateBoostCommands
    tags:
      - { name: drush.command }

and I think the namespace for the command should be
namespace Drupal\migrate_boost\Commands; instead of namespace Drupal\migrate_boost\Drush\Commands;

Thoughts?

RE #11, this is the error, I didn't make a commit because I don't have an idea what is supposed to happen with that, I suspect remove migrate_boost_module_implements_alter?

Error: Call to undefined method Drupal\migrate_boost\MigrateBoost::alter() in /var/www/html/public/modules/contrib/migrate_boost/migrate_boost.module on line 14 #0 /var/www/html/public/core/lib/Drupal/Core/Extension/ModuleHandler.php(552): migrate_boost_module_implements_alter()

Re #12, based on the Issue Summary goal of “let’s also remove the dependency on Drush,” I’m assuming we can remove the command entirely.

Since we are decorating the module handler, we also don’t need migrate_boost_module_implements_alter().

I’ll go ahead and remove them as I’m just trying to get this working. Let me know if I’ve overstepped in my haste!

No worries! HMIA does need to be removed, I don't recall if we need Drush still, I think it hooks into only affect migrations run with Drush.

Happy to have someone reviewing, I only work on this for a personal project so I've not had time to dig in further.

Thanks @nicxvan, I am not sure what HMIA is, I am guessing it's related to inheritance or something but google/ChatGPT didn't turn up good results.

hook_module_implements_alter == HMIA

Ah a Drupal acronym I've never heard of, thanks!

Ok this is working for me now. My last commit got rid of isHookDisabled() calls without the new $module arg because it wasn't available without implementing getImplementationInfo() and buildImplementationInfo() and verifyImplementations() which isn't the end of the world but getting pretty complicated and likely not needed.

The alter methods are tricky, because they call some of the decorated methods inside themselves. It’s almost better to just leave them alone.

This looks great!

Gonna test it some now.

I think I can get behind dropping the ability to kill alter, but I think we need to handle invokeAllWith still

What do you think of that?

Thanks for all of your help!

Thanks for the review!

I’ll look at it tomorrow when I am in.

@nicxvan I had a look, trying it out IRL right now. The code looks good to me, going to mark this RTBC from my end as it does what is on the Tin ;)

Providing static patch for the time being.

Seems to work great! Currently, in the midst of migrating a large Drupal 6 site. If anything goes wrong related to this module, I'll comment here again!

Note, that the introduction of the new Decorator leads to an error when migrating d6 nodes. But that is unrelated to this module but rather a drupal core issue. I just created an issue + fix here: 🐛 "core/modules/node/src/Plugin/migrate/source/d6/Node.php" "__construct" method uses variable of type "ModuleHandler" instead of "ModuleHandlerInterface" Active .

Otherwise, RTBC +1!

Confirming #26!

FYI, 🐛 "core/modules/node/src/Plugin/migrate/source/d6/Node.php" "__construct" method uses variable of type "ModuleHandler" instead of "ModuleHandlerInterface" Active was merged into core! So there is nothing holding back this issue!

Thanks everyone, this is on my list!