MigrateIdMapInterface has saveMessage(), to save per-row notice/warning/error messages to the message table associated with a migration. There is no method on the interface for retrieving them (the closest thing is messageTableName() on the SQL implementation). So, let's add getMessages()...

CommentFileSizeAuthor
#5 interdiff.txt2.61 KBmikeryan
#5 need_an_api_for-2560435-5.patch5.94 KBmikeryan
#3 need_an_api_for-2560435-3.patch5.37 KBmikeryan

Comments

mikeryan created an issue. See original summary.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Issue summary: View changes
mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Active » Needs review
StatusFileSize
new need_an_api_for-2560435-3.patch5.37 KB

The main purpose here would be for tools to provide views of all the messages associated with a migration, but we also (especially for tests) want to be able to retrieve the messages corresponding to a single item (note there can be multiple messages per item). So, I added getMessageIterator($source_id_values = []), which retrieves all messages when the argument is empty.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

Looks pretty good to me.

  1. +++ b/core/modules/migrate/src/Plugin/MigrateIdMapInterface.php
@@ -66,6 +66,18 @@ public function saveIdMapping(Row $row, array $destination_id_values, $status =
+   * @param array $source_id_values
+   *   The source identifier values of a specific record to retrieve. If empty,
+   *   all messages are retrieved.

    Should be specified as (optional) 

  2. +++ b/core/modules/migrate/src/Plugin/MigrateIdMapInterface.php
@@ -66,6 +66,18 @@ public function saveIdMapping(Row $row, array $destination_id_values, $status =
+   * @return object[]
+   *   Retrieves a list of message rows.

    \Traversable is the proper type hint here. 

  3. +++ b/core/modules/migrate/src/Plugin/MigrateIdMapInterface.php
@@ -66,6 +66,18 @@ public function saveIdMapping(Row $row, array $destination_id_values, $status =
+  public function getMessageIterator(array $source_id_values = []);

    getMessageIterator() is a strange method name, can it be getMessages()?

  4. It might be nice to have the ability to retrieve messages by level as well. Maybe an additional, optional parameter to getMessageIterator()?
mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
StatusFileSize
new need_an_api_for-2560435-5.patch5.94 KB
new interdiff.txt2.61 KB

Done.

\Traversable is the proper type hint here.

Actually, I think \Iterator is more on-point.

getMessageIterator() is a strange method name, can it be getMessages()?

I think this is consistent with MigrateSourceInterface::getIterator() (specifying getMessageIterator here since the IdMap is an iterator itself over the map table). It's returning an iterator (in practice, SQL query results) rather than the messages directly.

Maybe an additional, optional parameter to getMessageIterator()?

OK, done.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

It freaking prints money!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

So this one I'm a bit less gung-ho about:

1) I don't understand why we have a totally separate logging system in Migrate vs. just using the existing \Drupal::logger(). In IRC, Mike explained this is because you want to store additional properties (such as Migration Source) for easy filtering/debugging purposes.

2) Because it's a separate system, we also don't print these messages anywhere in core, particularly in admin/reports/dblog which is where site builders expect to find status/error messages. (Migrate Upgrade does log messages there, but that's not yet in core.)

3) Awhile back, we also changed watchdog()'s implementation to be PSR-3 friendly. https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-3-logg... The proposed API for this separate logging interface OTOH is a "Drupalism," not inline with those standards.

So 1-2 are basically a "pre-existing condition," so not really a reason to hold up the patch. And I'm not sure if 3 is either, since "by design" we're not building a general-purpose messaging system like watchdog()/\Drupal::logger() are intending to be; this is specific to Migrate (see #1 ;)).

I know this is holding up other things, so I think it's safe to go in, but we should maybe revisit some of these items in follow-ups, esp. with the guidance of a Framework Manager.

For now, committed and pushed to 8.0.x. Thanks!

  • webchick committed e53688f on 8.0.x 
    Issue #2560435 by mikeryan, phenaproxima: Need an API for retrieving...
mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Related issues: +#2561455: Implement drush migrate-messages command for D8 migrate

This is now being used by migrate_plus: #2561455: Implement drush migrate-messages command for D8 migrate.

Status: Fixed » Closed (fixed)

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