Pipeline failure exceptions should say which plugin they failed at

+++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -358,7 +358,7 @@ public function processRow(Row $row, array $process = NULL, $value = NULL) {
+            throw new MigrateException(sprintf('Pipeline failed at %s plugin for destination %s: %s got instead of an array,', $plugin->getPluginId(), $destination, $value));

While we're at it, "got" isn't very elegant English here... Maybe we can change it to "received"?

That's a nit - needs work because needs tests. Given that the tests passed, that suggests this exception is not currently tested, good opportunity to add one...

Thanks!

Added test.

This would be a big usability win for Migrate. Kicking it up a notch.

Self-assigning for review.

1. +++ b/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php
   @@ -420,6 +420,40 @@ public function testProcessRowEmptyPipeline() {
   +      $plugins['destination_id'][$key] = $this->getMock('Drupal\migrate\Plugin\MigrateProcessInterface');
   +      $plugins['destination_id'][$key]->expects($this->once())
   +        ->method('getPluginDefinition')
   +        ->will($this->returnValue(['handle_multiples' => FALSE]));
   +      $plugins['destination_id'][$key]->expects($this->atMost(1))
   +        ->method('transform')
   +        ->will($this->returnValue('transform_return_string'));
   +      $plugins['destination_id'][$key]->expects($this->atMost(1))
   +        ->method('multiple')
   +        ->will($this->returnValue(TRUE));
   +      $plugins['destination_id'][$key]->expects($this->atMost(1))
   +        ->method('getPluginId')
   +        ->will($this->returnValue('plugin_id'));
   

   Can this be changed to use Prophecy? It's a lot more readable. If we don't want to switch to Prophecy, we should have comments to improve grokkability.

2. +++ b/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php
   @@ -420,6 +420,40 @@ public function testProcessRowEmptyPipeline() {
   +    try {
   +      $this->executable->processRow($row);
   +      $this->fail('Pipeline exception should throw.');
   +    }
   +    catch (MigrateException $e) {
   +      $this->assertEquals('Pipeline failed at plugin_id plugin for destination destination_id: transform_return_string received instead of an array,', $e->getMessage());
   +    }
   

   Rather than try/catch, let's take advantage of PHPUnit's @expectedException and @expectedExceptionMessage annotations.

• Changed from using getMock() to prophesize() for improved readability.
• Used @expectedException and @expectedExceptionMessage - what an awesome, elegant code structure. Love it! @phenaproxima++

+++ b/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php
@@ -420,6 +421,31 @@ public function testProcessRowEmptyPipeline() {
+    $this->migration->expects($this->once())
+      ->method('getProcessPlugins')
+      ->with(NULL)
+      ->will($this->returnValue($plugins));

One tiny thing, for brevity's sake. This can be rewritten:

$this->migration->method('getProcessPlugin')->willReturn($plugins);

Let us change it to that, and then this is good to go! Excellent work, @Jo Fitzgerald. Thank you!

Done - much nicer!

Typo!

Glorious.

Seems to have been a testbot glitch, back to RTBC.

t3rabyt3@imt3rabyt3-LY710 /v/w/h/drupal84> git apply --check 2846002-14.patch 
t3rabyt3@imt3rabyt3-LY710 /v/w/h/drupal84> 

Patch applies cleanly, not sure why the testbot says "PHP 5.6 & MySQL 5.5 Patch Failed to Apply"

Patch did not apply for me so re-rolled.

Resetting to RTBC in expectation.

+++ b/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php
@@ -420,6 +421,28 @@ public function testProcessRowEmptyPipeline() {
+   * @expectedException \Drupal\migrate\MigrateException
+   *
+   * @expectedExceptionMessage Pipeline failed at plugin_id plugin for destination destination_id: transform_return_string received instead of an array,

We don't use annotations for this anymore. See #2822837: Replace @expectedException @expectedExceptionMessage with $this->setExpectedException.

Replaced @expectedException, @expectedExceptionMessage with $this->setExpectedException.

phenaproxima-- for misleading me :p

Hey now, this is news to me as well! :) Having just now glanced at the issue linked by @alexpott, the rationale for using setExpectedException() makes sense to me. I don't agree that it should always be used -- like if unit testing a single method that will throw an exception on failure -- but the decision has been made and I will abide by it.

Preemptively back to RTBC assuming the tests pass.

Committed and pushed 2421ca2 to 8.4.x and 0fdb7f3 to 8.3.x. Thanks!

Backported to 8.3.x as migrate is experimental and this is helpful.