Refactor run() method on Migrate UI batch and remove the $operation parameter

I've been working on this and should be able to upload a patch in a day or two.

Took a bit longer than expected.

Simple reroll using git diff -M, so it sees MigrateUpgradeImportBatch.php as a rename of MigrateUpgradeRunBatch.php (thus doesn't need to match the entire contents of MigrateUpgradeRunBatch.php to apply). I don't think I lose my RTBC privileges for that...

Tested locally and works fine - I'll RTBC once the tests pass.

D'oh! Didn't wait for the full migration to finish running, this is regressing the EntityFile change to source_base_path...

OK, fixed up source_base_path.

Test failures look unrelated, so retesting.

I kinda feel my last patch here might be a little too much for me to retain RTBC privileges - someone else probably should take a look-see (phenaproxima, benjy, ...).

#12 fails to apply on core/modules/migrate_drupal_ui/src/MigrateUpgradeRunBatch.php against 8.2.x and 8.2.1

@spesic, thanks. Here is a reroll.

Assigning to myself to review this.

1. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   --- /dev/null
   +++ b/core/modules/migrate_drupal_ui/src/MigrateUpgradeBatchBase.php
   
   +++ b/core/modules/migrate_drupal_ui/src/MigrateUpgradeBatchBase.php
   --- /dev/null
   +++ b/core/modules/migrate_drupal_ui/src/MigrateUpgradeImportBatch.php
   

   I'd like to see these moved into a Batch namespace. This is more important with my next comment.

2. +++ b/core/modules/migrate_drupal_ui/src/MigrateUpgradeBatchBase.php
   @@ -0,0 +1,116 @@
   +abstract class MigrateUpgradeBatchBase {
   

   This class is not necessary if we don't implement a single base (abstract) class with two different implementations. One for running import and one for running finish. I think that approach makes more sense, have the abstract base class and two concrete classes for Import and one for Finish, each having a run method.

3. +++ b/core/modules/migrate_drupal_ui/src/MigrateUpgradeBatchBase.php
   @@ -0,0 +1,116 @@
   +  const MESSAGE_LENGTH = 20;
   

   Const name seems odd. But fixing that doesn't seem in scope. It has been that way for a while.

4. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   --- /dev/null
   +++ b/core/modules/migrate_drupal_ui/src/MigrateUpgradeBatchBase.php
   

   Could we move the new batch classes into a new Batch namespace? I don't like root level classes.

Took a few readings of #1, 2 and 4 to grasp your meaning. The attached patch moves MigrateUpgradeBatchBase, MigrateUpgradeImportBatch and MigrateMessageCapture to a new batch namespace. And moves the finish method from MigrateUpgradeImportBatch to a new file MigrateUpgradeImportFinish in the new batch namespace.

And for #3, yes MESSAGE_LENGTH is probably better named something like NUMBER_OF_MESSAGES. The place to change that may be #2569785: Placement of batch progress messages.

Looks good now. I added a comment over in #2569785: Placement of batch progress messages for #22-3. All other feedback is addressed.

+++ b/core/modules/migrate_drupal_ui/src/Batch/MigrateUpgradeImportFinish.php
@@ -0,0 +1,44 @@
+   * Callback executed when the Migrate Upgrade Import batch process completes.

Having this in a separate class doesn't seem useful - they'll never be used independently.

Why not rename the method to 'finished' and have it in the same class?

@catch, that makes sense to me. (Actually, I wondered about that to but did it anyway).

This patch makes the change mentioned in #28, and adds documentation to the finished method.

+++ b/core/modules/migrate_drupal_ui/src/Batch/MigrateUpgradeImportBatch.php
@@ -8,76 +8,31 @@
+class MigrateUpgradeImportBatch extends MigrateUpgradeBatchBase {

With this new approach, I think we can combine/squash the abstract base class, no?

I left it as preparation for rollback. Here it is merged.

Ah, silly fingers named the interdiff incorrectly. Let's try again.

Shouldn't this also remove the base class?

Well, can only say that this is embarrassing.

And more errors! There must be a 'moral of the story here'. Don't post patches for a few days after a big earthquake? Don't submit a patch before taking son to the airport in the wee hours?

And even the bot was having trouble.

Tests passing setting to NR.

Mostly small stuff. Really, all small stuff. But the first point should be addressed as it violates coding standards.

1. +++ b/core/modules/migrate_drupal_ui/src/Batch/MigrateUpgradeImportBatch.php
   @@ -30,7 +30,7 @@ class MigrateUpgradeRunBatch {
   +  static $numProcessed = 0;
   

   Me thinks this should still be protected. Or at least implicitly public.

2. +++ b/core/modules/migrate_drupal_ui/src/Batch/MigrateUpgradeImportBatch.php
   @@ -51,33 +51,28 @@ class MigrateUpgradeRunBatch {
   +   * Runs a single migrate import batch.
   

   Nit: It seems more natural english to me to say 'batch import' vs 'import batch'.

3. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -1109,13 +1109,12 @@ public function submitConfirmForm(array &$form, FormStateInterface $form_state)
          'operations' => [
   

   Not sure if it is in scope, but would renaming 'operations' => 'run' or 'run_batch' make sense? Use your discretion here. I'd be worried about if we would need to wire in a cache clear or something to an update and re-naming to me isn't that important.

1. Done. I grepped core and $numProcessed is only used in this file.
2. Changed.
3. No discretion here, 'operations' is a required parameter in the array passed to batch_set. https://api.drupal.org/api/drupal/core!includes!form.inc/function/batch_...

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!