Make MigrateExecutable statistics publicly available

Also, defaulted the stats to 0 (rather than make any user-facing code have to check for NULL and throw in a 0).

Added the methods to the interface per @benjy's suggestion.

Also improved the docs.

And, added a $reset parameter to the *SinceFeedback methods - when being retrieved for feedback purposes, passing TRUE will zero the fields.

I'm not a huge fan of the $reset parameter to getters to reset the values, wouldn't a reset method be better?

It seems simpler for clients to make the one method call rather than two (get and reset) - it would be rare to reset the count without reading the previous count. But, no biggie - anyone else want to weigh in?

Also, I was thinking, maybe we should move the statistics out of the executable into their own object? The executable could just return a MigrationStatistics object or such. Thoughts?

Interesting. That simplifies the MigrateExecutable API to just the one method, and we wouldn't have to extend it (just the MigrationStatistics class) to add more stats...

On the other hand, where the MigrateExecutable currently simply increments its count members, the MigrationStatistics class would have to (in addition to getters and resetters) need to have incrementers for each stat... - it'll be a lot of methods on that API to just manage a few counters.

Here it is with the additional class.

Latest version is not yet rtbc.

Updated per code review.

But now, much as I hate to bikeshed my own patch, I was wondering if we should make it more general and allow counters to be added dynamically. Something like

  public getCounter($counter_name);
  public incrementCounter($counter_name);
  public resetCounter($counter_name);

So, replace $this->statistics->incrementSuccesses() with $this->statistics->incrementCounter('successes'). As for handling the feedback case, it could be done by also tracking an explicit 'successes_since_feedback' counter, but that would require making two increment calls instead of one at each point. Alternatively we could automatically track two counters per counter name, add a getCounterSinceFeedback($counter_name), and rename resetCounter to resetFeedback.

Use case - when developing and testing migration code I'm very frequently counting stuff - how many times we find a matching term instead of inserting a new one, how often a given field has 0, 1, or multiple values, etc. This would make it very easy to throw a counter into any plugin - the drush migrate-import command could have a --dump-counters option or somesuch. The disadvantage is static discovery of available statistics, although we could maintain getTotalSuccesses etc. as wrappers around the general API for the statistics used in core.

And now thinking more about that "feedback"... In D7, progress messages were being generated within the migration class itself - at the end of the migration process by default, and periodically during it with the "feedback" option enabled. And it was only outputting the feedback counters, not the total counters - looking closely at the code, the total counters were only used for two things: computing overall throughput (983 items/second), and populating the migrate_log table, neither of which we're doing in the core module now.

So, I think we should forget about tracking the "feedback" separately - we just have simple counters with get/increment/reset methods. The web and drush interfaces can easily accumulate overall totals for themselves if they need them.

That being said, how will an external tool get periodic feedback during a migration process? #2309695: Add query batching to SqlBase might be the answer there, so I'm not inclined to worry about it here...

No interdiff, since nearly every line changed. This removes the extra "feedback" counters and provides general-purpose counters rather than hard-coded successes/processed counters. At this point, I'm not sure the new class really adds value over building the counters into MigrateExecutable, but consider this a POC...

Ugh, let me regenerate that patch file....

Haven't had time to generate an iteration with the explicit successes/processed methods... But re: counters - no, all the execution stats we're tracking are counters, and the API with its increment(Counter) method is assuming counters. I felt the more specific name would be clearer, but I'm not wedded to it - we can go back to Statistics if we think some other kind of statistics might be useful.

Here it is with specific methods for success/processed. Keeping in mind that I also want to add error/ignored counters, though, I fear method explosion...

Like so?

At this point #2443089: Track additional statistics when migrating is so simple I folded it in here, but I can back it out if requested...

edit: linked to wrong issue

Uploaded wrong interdiff...

1. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -81,6 +58,13 @@ class MigrateExecutable implements MigrateExecutableInterface {
   +   * An object for tracking statistics.
   

   No longer an object.

2. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -604,6 +584,49 @@ protected function getTimeElapsed() {
   +  public function resetCounter($name) {
   +    $this->counters[$name] = 0;
   +  }
   +
   +  public function incrementCounter($name) {
   +    if (!isset($this->counters[$name])) {
   +      $this->counters[$name] = 0;
   +    }
   +    $this->counters[$name]++;
   +  }
   +
   +  public function getCount($name) {
   

   I would prefer 'resetCount' and 'incrementCount'.

   These functions need docs.

simplified++

Thanks!

Made the changes myself (other than the name changes - writing the doc comments, I decided the original names were right). Also, today I ran across the source plugin counting statistics that nobody uses, I think this is the right patch to remove those.

Well... The source plugin might need access to incrementIgnored(), depending on how we resolve #2443617: Find a way for SourcePluginBase to get MigrateExecutable. But without the helpers, it can always use incrementCounter() anyway...

I'm feeling more now that we've moved this all back into MigrateExecutable that the helpers don't really add much - if they're not public (and I don't see a use case for overriding them) they don't add anything useful to the API. Maybe, if want the available counters to be discoverable, have

public function counterList() {
  return array_keys($this->counters);
}

Definite yes to adding the basic counter APIs to the interface.

Removing the specialized methods, adding the public methods to the interface, adding getCounterList().

Stepping back a bit - this issue exists purely to support contrib UIs, which don't have the information necessary to be able to determine on its own the details of what's been imported/failed/ignored. So, for something completely different - instead of the core API dealing with statistics on its own, how about it dispatches events, enabling contrib to track the stats (and who knows what else)? This patch rips the stat tracking out and replaces it with having the ID map dispatch events when an ID mapping is saved or deleted. Note that it would obviate the need for #2443089: Track additional statistics when migrating as well.

I should admit, this is a foot in the door, I think an event-driven approach will help with other issues including

• #2501731: Migrate pre/post Import/Rollback methods not invoked
• #2488836: Refactor prepareRow() to add events in addition to hook
• #2443617: Find a way for SourcePluginBase to get MigrateExecutable
• #2429089: Track the last imported time for migrations

Yep, fixed up the unit tests to reflect my changes, forgot the simpletests...

OK, this time both unit tests and simpletest pass locally.

I'll post a patch at #2432993: Implement reporting of results on migrate-import using the events to count imported/ignored/failed items.

I'd like to go beyond this, emitting events at various key points where contrib can do various things with them - it seemed simplest to me to have the migration hold the event dispatcher, which could propagate to the various plugins (e.g., so the destination plugin can implement #2501731: Migrate pre/post Import/Rollback methods not invoked with events).

Perhaps it would be best to postpone this issue and open a separate issue for adding all the event dispatching I'm imagining? And once that's in, this and a few other issues could be close as dupe/won't fix...

At the moment, the plan is for this to be accomplished by #2535458: Dispatch events at key points during migration.

Contrib can do this itself now: https://www.drupal.org/node/2544874