UI-401: Fix/imagemagick stub cleanup #179
Conversation
Seems like migrate_tools has some behaviors in its executable that widely hit on the migrate events, so let's avoid the base events to avoid its behaviours.
📝 WalkthroughWalkthroughThe changes introduce an event-dispatching wrapper around the core migrate.stub service, enabling lifecycle event hooks during stub creation. A new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Wrapper as MigrateStubEventWrapper
participant Dispatcher as EventDispatcher
participant Inner as MigrateStub (inner)
participant Subscribers as Event Subscribers
Client->>Wrapper: createStub(migration_id, source_ids, default_values)
Wrapper->>Wrapper: getMigration(migration_id)
Wrapper->>Dispatcher: dispatch(PRE_IMPORT event)
Dispatcher->>Subscribers: notify PRE_IMPORT listeners
Subscribers->>Subscribers: execute handlers
Wrapper->>Inner: createStub(migration, source_ids, default_values)
Inner->>Inner: doCreateStub with event dispatches
Inner->>Dispatcher: dispatch(PRE_SAVE event)
Dispatcher->>Subscribers: notify PRE_SAVE listeners
Inner->>Inner: perform import
Inner->>Dispatcher: dispatch(POST_SAVE event)
Dispatcher->>Subscribers: notify POST_SAVE listeners
Inner-->>Wrapper: return stub result
Wrapper->>Dispatcher: dispatch(POST_IMPORT event)
Dispatcher->>Subscribers: notify POST_IMPORT listeners
Subscribers->>Subscribers: execute handlers
Wrapper-->>Client: return stub result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/MigrateStubEventWrapper.php`:
- Around line 5-26: The constructor currently type-hints
EventDispatcherInterface from PSR-14 but the class calls dispatch($event,
$eventName) (Symfony-style); update the constructor and use statement to
type-hint Symfony's event dispatcher (e.g.
Symfony\Contracts\EventDispatcher\EventDispatcherInterface or
Symfony\Component\EventDispatcher\EventDispatcherInterface) so
MigrateStubEventWrapper::__construct(...) accepts the Symfony dispatcher used by
the dispatch($event, $eventName) calls; adjust the import and keep the parameter
name (eventDispatcher) unchanged.
🧹 Nitpick comments (1)
src/MigrateStub.php (1)
22-78: Align dispatcher type with the event-name dispatch usage here as well.This code calls
dispatch($event, $eventName), which requires Symfony's dispatcher signature. PSR-14'sEventDispatcherInterfaceonly supportsdispatch(object $event)without an event name parameter. While Drupal'sevent_dispatcherservice is Symfony-based and functionally supports this usage, the type hint should match: switch toSymfony\Component\EventDispatcher\EventDispatcherInterfacein the import, property docblock, and method return type.♻️ Proposed change
-use Psr\EventDispatcher\EventDispatcherInterface; +use Symfony\Component\EventDispatcher\EventDispatcherInterface; - * `@var` \Psr\EventDispatcher\EventDispatcherInterface|null + * `@var` \Symfony\Component\EventDispatcher\EventDispatcherInterface|null */ protected ?EventDispatcherInterface $eventDispatcher = NULL; - * `@return` \Psr\EventDispatcher\EventDispatcherInterface + * `@return` \Symfony\Component\EventDispatcher\EventDispatcherInterface * The event dispatcher service. */ protected function getEventDispatcher() : EventDispatcherInterface { return $this->eventDispatcher ??= \Drupal::service('event_dispatcher'); }
| use Drupal\Component\Plugin\Exception\PluginNotFoundException; | ||
| use Drupal\dgi_migrate\EventSubscriber\StubMigrateEvents; | ||
| use Drupal\migrate\Event\MigrateImportEvent; | ||
| use Drupal\migrate\MigrateMessageInterface; | ||
| use Drupal\migrate\MigrateStubInterface; | ||
| use Drupal\migrate\Plugin\MigrationInterface; | ||
| use Drupal\migrate\Plugin\MigrationPluginManagerInterface; | ||
| use Psr\EventDispatcher\EventDispatcherInterface; | ||
|
|
||
| /** | ||
| * Wrap import/creation of stubs to ensure events fire for them. | ||
| */ | ||
| class MigrateStubEventWrapper implements MigrateStubInterface { | ||
|
|
||
| /** | ||
| * Constructor. | ||
| */ | ||
| public function __construct( | ||
| protected MigrateStubInterface $inner, | ||
| protected MigrationPluginManagerInterface $migrationPluginManager, | ||
| protected EventDispatcherInterface $eventDispatcher, | ||
| protected MigrateMessageInterface $migrateMessage, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /repo && find . -name "MigrateStubEventWrapper.php" -type fRepository: discoverygarden/dgi_migrate
Length of output: 126
🏁 Script executed:
wc -l src/MigrateStubEventWrapper.phpRepository: discoverygarden/dgi_migrate
Length of output: 105
🏁 Script executed:
cat -n src/MigrateStubEventWrapper.phpRepository: discoverygarden/dgi_migrate
Length of output: 3282
🌐 Web query:
PSR-14 EventDispatcherInterface dispatch method signature
💡 Result:
In PSR-14, Psr\EventDispatcher\EventDispatcherInterface::dispatch() is defined as:
public function dispatch(object $event);There is no declared return type in the method signature, but the spec requires the dispatcher to return the same event object (typically documented as @return object). [1] [2]
Sources: [1] [2]
🌐 Web query:
Symfony EventDispatcherInterface dispatch method signature
💡 Result:
Current Symfony (Contracts; used by Symfony 5+ / 6 / 7 / 8) uses:
public function dispatch(object $event, ?string $eventName = null): object;(generically, it returns the same event type T that you pass in). [1][3]
Older Symfony (e.g. 4.1-era symfony/event-dispatcher) used:
public function dispatch(string $eventName, Event $event = null);[2]
Switch to Symfony's dispatcher interface because the code passes an event name parameter.
Lines 36 and 40 call dispatch($event, $eventName), which is Symfony's API. PSR-14's EventDispatcherInterface::dispatch() accepts only a single parameter: dispatch(object $event). Using the PSR-14 interface here is incorrect and would fail at runtime if a PSR-14-only dispatcher is injected.
Proposed change
-use Psr\EventDispatcher\EventDispatcherInterface;
+use Symfony\Component\EventDispatcher\EventDispatcherInterface;🤖 Prompt for AI Agents
In `@src/MigrateStubEventWrapper.php` around lines 5 - 26, The constructor
currently type-hints EventDispatcherInterface from PSR-14 but the class calls
dispatch($event, $eventName) (Symfony-style); update the constructor and use
statement to type-hint Symfony's event dispatcher (e.g.
Symfony\Contracts\EventDispatcher\EventDispatcherInterface or
Symfony\Component\EventDispatcher\EventDispatcherInterface) so
MigrateStubEventWrapper::__construct(...) accepts the Symfony dispatcher used by
the dispatch($event, $eventName) calls; adjust the import and keep the parameter
name (eventDispatcher) unchanged.
1 participant
Builds on top of #171
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.