Refactor to decorate ModuleHandler instead of HMIA

Created on 13 January 2025, 2 months ago

Problem/Motivation

Dynamic removal of hooks will not be supported for long, let's also remove the dependency on drush.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

πŸ“Œ Task
Status

Active

Version

1.0

Component

Code

Created by

πŸ‡ΊπŸ‡ΈUnited States nicxvan

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @nicxvan
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • Merge request !8Resolve #3499285 "Refactor to decorate" β†’ (Open) created by nicxvan
  • πŸ‡©πŸ‡°Denmark ressa Copenhagen
  • πŸ‡©πŸ‡°Denmark ressa Copenhagen

    Moving some description here, from duplicate issue.

  • πŸ‡¨πŸ‡¦Canada joelpittet Vancouver
    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.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    What version of drupal?

  • πŸ‡¨πŸ‡¦Canada joelpittet Vancouver

    Same in the title, drupal 10 and drush 13

  • πŸ‡¨πŸ‡¦Canada joelpittet Vancouver

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

  • Pipeline finished with Failed
    13 days ago
    Total: 135s
    #440424
  • πŸ‡¨πŸ‡¦Canada joelpittet Vancouver

    I saw the same problem in 2.x

  • Pipeline finished with Failed
    13 days ago
    Total: 227s
    #440427
  • πŸ‡¨πŸ‡¦Canada joelpittet Vancouver

    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?

  • Pipeline finished with Canceled
    13 days ago
    Total: 200s
    #440451
  • Pipeline finished with Failed
    13 days ago
    Total: 133s
    #440452
  • πŸ‡¨πŸ‡¦Canada joelpittet Vancouver

    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?

  • πŸ‡¨πŸ‡¦Canada joelpittet Vancouver

    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()

  • πŸ‡¨πŸ‡¦Canada joelpittet Vancouver

    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!

  • Pipeline finished with Failed
    12 days ago
    Total: 155s
    #441171
  • Pipeline finished with Failed
    12 days ago
    Total: 158s
    #441199
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    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.

  • πŸ‡¨πŸ‡¦Canada joelpittet Vancouver

    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.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    hook_module_implements_alter == HMIA

  • πŸ‡¨πŸ‡¦Canada joelpittet Vancouver

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

  • Pipeline finished with Failed
    12 days ago
    Total: 130s
    #441289
  • Pipeline finished with Failed
    12 days ago
    Total: 131s
    #441294
  • Pipeline finished with Failed
    12 days ago
    Total: 155s
    #441304
  • Pipeline finished with Failed
    12 days ago
    Total: 160s
    #441330
  • Pipeline finished with Failed
    12 days ago
    Total: 141s
    #441385
  • Pipeline finished with Failed
    12 days ago
    Total: 192s
    #441387
  • Pipeline finished with Failed
    12 days ago
    Total: 250s
    #441392
  • Pipeline finished with Success
    12 days ago
    Total: 137s
    #441431
  • Pipeline finished with Canceled
    12 days ago
    Total: 91s
    #441497
  • Pipeline finished with Failed
    12 days ago
    Total: 129s
    #441498
  • Pipeline finished with Failed
    12 days ago
    Total: 230s
    #441501
  • Pipeline finished with Failed
    12 days ago
    Total: 150s
    #441504
  • Pipeline finished with Failed
    12 days ago
    Total: 132s
    #441571
  • Pipeline finished with Failed
    12 days ago
    Total: 128s
    #441589
  • πŸ‡¨πŸ‡¦Canada joelpittet Vancouver

    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.

  • Pipeline finished with Success
    12 days ago
    Total: 296s
    #441593
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    This looks great!

    Gonna test it some now.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

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

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    What do you think of that?

    Thanks for all of your help!

  • πŸ‡¨πŸ‡¦Canada joelpittet Vancouver

    Thanks for the review!

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

  • πŸ‡¨πŸ‡¦Canada joelpittet Vancouver

    @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 ;)

  • πŸ‡©πŸ‡ͺGermany Grevil

    Providing static patch for the time being.

  • πŸ‡©πŸ‡ͺGermany Grevil

    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!

  • πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

    Confirming #26!

Production build 0.71.5 2024