Most of the time in core, MigrateExecutable is constructed directly with a new MigrateMessage object. This leads to a little bit of cruft and slightly degraded DX; MigrateExecutable's constructor could easily accept a MigrateMessageInterface object, or create a new MigrateMessage if one was not passed.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

phenaproxima created an issue. See original summary.

phenaproxima’s picture

Status: Active » Needs review
FileSize
11.59 KB

Status: Needs review » Needs work

The last submitted patch, 2: 2859246-2.patch, failed testing.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
3.08 KB
13.14 KB

Apologies for sticking my nose in if this isn't the kinda solution you're looking for, but this will at least get the tests passing.

I'm a little uncomfortable with mixing getMock() and prophesize(), but that's what I went with.

heddn’s picture

Status: Needs review » Needs work

Let's add a comment why we are passing in the event_dispatcher, or at least add a test of it in use. I think I know why, but it isn't blatantly obvious either.

heddn’s picture

Status: Needs work » Reviewed & tested by the community

I don't know what I was thinking in #5. RTBC. MigrateMessage is no longer required. And it is BC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/migrate/src/MigrateExecutable.php
    @@ -96,17 +96,17 @@ class MigrateExecutable implements MigrateExecutableInterface {
    +    $this->eventDispatcher = $event_dispatcher ?: \Drupal::service('event_dispatcher');
    

    This code makes some of

      /**
       * Gets the event dispatcher.
       *
       * @return \Symfony\Component\EventDispatcher\EventDispatcherInterface
       */
      protected function getEventDispatcher() {
        if (!$this->eventDispatcher) {
          $this->eventDispatcher = \Drupal::service('event_dispatcher');
        }
        return $this->eventDispatcher;
      }
    

    Unnecessary. But I don't think that making this change is necessary here. Why is it being done?

  2. +++ b/core/modules/migrate/tests/src/Unit/MigrateExecutableMemoryExceededTest.php
    @@ -53,8 +54,9 @@ protected function setUp() {
    +    $this->executable = new TestMigrateExecutable($this->migration, $this->message, $event_dispatcher);
    
    +++ b/core/modules/migrate/tests/src/Unit/process/UrlEncodeTest.php
    @@ -56,7 +56,8 @@ public function testUrls($input, $output) {
    +    $executable = new MigrateExecutable($this->getMigration(), NULL, $event_dispatcher);
    

    Why is it necessary to inject the event dispatcher here suddenly? It's not obvious at all to me.

phenaproxima’s picture

Regarding the event dispatcher...I don't exactly remember, but it might have been me trying to kill two birds with one stone. In the name of getting this extremely reasonable fix in, I'll revert that change and fix the event dispatcher in a follow-up.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
11.39 KB

Okay, here it is without any changes to the event dispatcher injection code.

I'm guessing this won't need any new test coverage, but it will need a change record.

phenaproxima’s picture

Status: Needs review » Needs work

The last submitted patch, 9: 2859246-9.patch, failed testing.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Jo Fitzgerald’s picture

Patch in #9 no longer applies. Re-rolled.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
832 bytes
11.49 KB

Avoid calling setMessage() with a NULL parameter.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

This looks good again.

The last submitted patch, 13: 2859246-13.patch, failed testing. View results

xjm’s picture

Straightforward DX improvement. The number of places we're no longer constructing an empty message object shows that this cleanup is worthwhile, and it's totally BC. Nice work!

Committed and pushed to 8.5.x. I didn't backport it because it's an API cleanup. I also published the CR (after retitling it a bit and bumping the branch to 8.5.x).

xjm’s picture

Status: Reviewed & tested by the community » Fixed

Reverted and recommitted to fix (a) the branch and (b) a crediting issue.

  • xjm committed 2257e63 on 8.5.x
    Issue #2859246 by Jo Fitzgerald, phenaproxima, alexpott, xjm: Allow...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.