The getMessageIterator() method does not return an instance of Iterator

@clepson:

Thanks for taking this on!

When you add a patch to an issue, you should change the status to "Needs review" (NR) to let people know that the issue is ready for the next step. I will do that for you, and I will give a review in a day or two unless someone else does it first.

The patch in #2 does what the issue suggests. It also does a little more, adjusting the description to match. (Kudos!)

Now we have to decide whether this is the right thing to do. This part is not a novice task, so I am removing that tag.

As far as I can tell, there are two classes that implement this interface, both in the Drupal\migrate\Plugin\migrate\id_map namespace: Sql and NullIdMap.

Sql::getMessageIterator() returns an object of class Drupal\Core\Database\Statement, which implements StatementInterface, which extends \Traversable: so far, so good.

NullIdMap::getMessageIterator() returns new \ArrayIterator([]). That class implements \SeekableIterator, which extends \Iterator, which extends \Traversable.

I think the current approach is the minimal fix to get this working.

While working on #2714529: Add source and destination IDs to the data returned by getMessageIterator() , I also noticed that some of the tests (see MigrateSkipRowTest) assume that Sql::getMessageIterator() returns a Statement object. (They call the fetchAll() method on the result.) I think it is out of scope for this issue, but we could either

1. Rewrite the test to avoid using fetchAll().
2. Declare that Sql::getMessageIterator() implements Drupal\Core\Database\StatementInterface

I think that (1) is a terrible idea, but I bring it up for the sake of discussion. I am not sure that (2) is allowed in PHP, but I think it is; if so, then I think it is a good idea. I might even be convinced that it is in scope for this issue.

I guess the problem is that this solution violates the backwards compatibility (BC) policy: https://www.drupal.org/core/d8-bc-policy#interfaces.

I will add a new proposed resolution to the issue summary. If I do a good job of that, then I think I can restore the Novice tag.

The BC policy mentions that this change should be mentioned in the release notes, so I am adding a section to the issue summary for a snippet.

@clepson:

I would use just the interface name and the method in the title of the change record (CR): something like "In MigrateMessageInterface, getMessageIterator() is deprecated in favor of getMessages()". The body of the CR should contain the fully qualified name.

I am not sure whether there are any guidelines on titles for CRs, but when I browse the recently published ones, I see only a few that use fully qualified names. Some, like "\Drupal\Core\Validation\TranslatorInterface no longer extends \Symfony\Component\Translation\TranslatorInterface", would be hard to understand without the full name. I do not think that MigrateMessageInterface has much potential for confusion.

Can you also add some before and after code snippets to the CR? You can use something like

-    $messages = $migration->getIdMap()->getMessageIterator()->fetchAll();
+    $messages = $migration->getIdMap()->getMessage()->fetchAll();

as the source of the example.

I do not have time for a full review now, but while looking for that example, I noticed that the new method is getMessage() instead of getMessages(), as suggested in #5.

I searched core for getMessageIterator and only found it in the expected places: two new deprecation tests, two IdMap plugins (where it is a wrapper for getMessages()), and the interface. So far, so good!

My only complaint are that the deprecation tests are more complicated than they have to be:

1. +++ b/core/modules/migrate/tests/src/Unit/MigrateNullIdMapTest.php
   @@ -0,0 +1,26 @@
   +  public function testGetMessageIterator() {
   +    $id_map = new NullIdMap([], 'null', NULL);
   +    $expected = new \ArrayIterator([]);
   +    $actual = $id_map->getMessageIterator();
   +    $this->assertEquals($expected, $actual);
   +  }
       

   The point of this test is to check that we get a deprecation message when we call getMessageIterator(), so I think the first and third lines are all we need. Unless you already tried that and I am wrong.

   Also, the test class extends MigrateTestCase. I did not try it, but I think that $this->getIdMap()->getMessageIterator() should work.

2. +++ b/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php
   @@ -340,6 +340,28 @@ public function testMessageSave() {
   +  public function testGetMessageIterator() {
   +    $message = 'Hello world.';
   +    $original_values = [
   +      5 => ['message' => $message, 'level' => MigrationInterface::MESSAGE_INFORMATIONAL],
   +      6 => ['message' => $message, 'level' => MigrationInterface::MESSAGE_ERROR],
   +    ];
   +    $id_map = $this->getIdMap();
   +
   +    foreach ($original_values as $key => $original_value) {
   +      $id_map->saveMessage(['source_id_property' => $key], $message, $original_value['level']);
   +    }
   +
   +    foreach ($id_map->getMessageIterator() as $message_row) {
   +      $this->assertNotEmpty($message_row);
   +    }
   +  }
       

   Same idea here. I think all we need is $messages = $this->getIdMap()->getMessageIterator();. The rest of this test more or less duplicates testMessageSave().

I edited the change record a bit.

I am updating the issue summary and un-assigning the issue. (You should un-assign yourself at the same time that you mark the issue NR.)

@clepson:

This looks perfect. Nice work!

I checked https://www.drupal.org/pift-ci-job/1320859, showed all (passing) tests, and verified that the two testGetMessageIterator tests are listed.

I updated the issue summary and added a snippet for the release notes. Back to RTBC.

This issue needs a reroll because #2714529: Add source and destination IDs to the data returned by getMessageIterator() (the issue I was working on when I created this one---see the issue summary) was fixed.

Doing the reroll is a novice task, so I am adding that label again.

@shubham.prakash:

Thanks for taking this on! I compared your patch to the one in #18.

When I added the Novice tag, I should have added to the issue summary a link to the instructions for rerolling a patch. I will do that now.

The main problem with your patch (probably the reason it fails automated tests) is that you remove the getMessageIterator() method entirely, instead of making it a wrapper for the new getMessages() method. There are some other, more minor problems:

1. +++ b/core/modules/migrate/src/Plugin/MigrateIdMapInterface.php
   @@ -62,6 +62,21 @@ public function saveIdMapping(Row $row, array $destination_id_values, $status =
       */
      public function saveMessage(array $source_id_values, $message, $level = MigrationInterface::MESSAGE_ERROR);
    
   +    /**
   

   This introduces an indentation error.

2. @@ -74,7 +89,14 @@ public function saveMessage(array $source_id_values, $message, $level = Migratio
       *
       * @return \Iterator
       *   Retrieves an iterator over the message rows.
   +   *
   +   *
   

   We only need one "blank" line here.

3. @@ -340,6 +340,16 @@ public function testMessageSave() {
        $this->assertEquals($count, 1);
      }
    
   +  /**
   +   * Tests the SQL ID map get message iterator method.
   +   * @group legacy
   +   * @expectedDeprecation getMessageIterator() is deprecated in drupal:8.8.0 and will be removed from drupal:9.0.0. Use getMessages() instead. See https://www.drupal.org/node/3060969
   +   */
   +  public function testGetMessageIterator() {
   +    $this->getIdMap()->getMessageIterator();
   +  }
   +
   

   This adds an unneeded blank line.

Since this issue deprecates an existing method, I think we should add the @deprecated tag.

@init90:

Thanks for the link to #3024461: Adopt consistent deprecation format for core and contrib deprecation messages. I was not aware of that standard.

Good catch on updating the test added in #2714529: Add source and destination IDs to the data returned by getMessageIterator() . That is the issue that made the patch in #18 fail.

Interdiffs do not work well for rerolls, so I looked at a raw diff between the patches in #18 and #41. (More precisely, I compared them using diff mode in Vim.) +1 for RTBC.

I am updating the "Remaining tasks" section of the issue summary and removing two tags now that the reroll is done.

Tip: use [# 3024461] (without the space character) to generate links to other issues.

It seems to me that the last two patches have expanded the scope of this issue.

Earlier patches aimed to make the minimal changes in order to get an honest @return comment. Given that this issue already involves a new API function and a deprecation, my inclination is to follow that minimalist approach.

The last two patches additionally update existing tests so that they no longer call fetchAll(). Since we are testing an implementation of the interface that actually returns an object of Drupal\Core\Database\Statement, it is not clear to me that we have to do that at all, let alone in this issue. In fact, it seems odd to me to avoid using fetchAll(), but then to assume that the Traversable items are objects with a message property. Let's face it, PHP is not a strongly typed language.

If we are going to expand the scope to removing calls to fetchAll(), then we should say so in the issue summary. If we are not going to expand the scope, then we could go back to the patch in #41, which was already RTBC.