Variable to config: search_active_modules [d7]

I think this is probably OK, though not being an expert in migrate or migrate tests, I am not setting this to RTBC. From what I can tell though, the migration looks like it is probably mostly correct.

A few concerns:
- The transform to Boolean method -- it is only getting value FALSE if the value in the array is exactly === 0. I do not know that I would rely on that -- I don't think the Search module in D7 is quite that strict -- I think in D7 the search module gets the value of search_active_modules variable and just evaluates the array value to be either TRUE or FALSE using normal PHP evaluation, rather than checking === 0. So probably the migration should use the same logic?
- There may be contrib search modules that are part of search_active_modules. Are you planning to allow for those to be migrated, or does the contrib module have to take care of that?
- A few minor docs questions/suggestions/issues:

1. +++ b/core/modules/search/src/Plugin/migrate/process/d7/SearchPageStatus.php
   @@ -0,0 +1,35 @@
   + * Transform search status to a boolean.
   

   boolean => Boolean

   Also I think I would say "search page status" not "search status".

2. +++ b/core/modules/search/src/Plugin/migrate/source/d7/SearchPageStatus.php
   @@ -0,0 +1,70 @@
   +/**
   + *
   + * @MigrateSource(
   

   This class is missing documentation.

3. +++ b/core/modules/search/src/Plugin/migrate/source/d7/SearchPageStatus.php
   @@ -0,0 +1,70 @@
   +      'status' => $this->t('The status of the search.'),
   

   I would document this as "Whether or not this module is enabled for search"

I don't really see where most of my concerns in the previous comment were addressed in the patch... and I'm not convinced all the changes in the interdiff are actually in the new patch file.

I took a careful look at the latest patch (didn't check the interdiff). There are still some issues:

1. +++ b/core/modules/search/src/Plugin/migrate/destination/d7/SearchPageStatus.php
   @@ -0,0 +1,51 @@
   +/**
   + * @MigrateDestination(
   + *   id = "d7_search_page_status"
   + * )
   + */
   +class SearchPageStatus extends DestinationBase {
   

   This class still has no documentation. It has annotation, not documentation.

2. +++ b/core/modules/search/src/Plugin/migrate/destination/d7/SearchPageStatus.php
   @@ -0,0 +1,51 @@
   +    $id = $row->getDestinationProperty('module') . '_search';
   

   Oh, I just noticed this.

   This is the right way to transform 'node' module to the default 'node_search' page config that the Node module provides in its config/install, and 'user' module to 'user_search', but it is NOT necessarily the correct thing to do for contrib modules.

   Probably what you need to do is:

   a) If it's 'node' or 'user', transform to node_search or user_search.

   b) If it's anything else, do not migrate it. Let the contrib module migrate its own search page status.

3. +++ b/core/modules/search/src/Plugin/migrate/destination/d7/SearchPageStatus.php
   @@ -0,0 +1,51 @@
   +    $search_page = SearchPage::load($id);
   

   It is quite likely that this will return NULL because the plugin ID may not actually exist.

   I mean, you're OK for node and user because they're in core, but if a site used a contrib search module you may not be OK.

4. +++ b/core/modules/search/src/Plugin/migrate/destination/d7/SearchPageStatus.php
   @@ -0,0 +1,51 @@
   +  public function getIds() {
   +    $ids['module']['type'] = 'string';
   +    return $ids;
   +  }
   

   I am unable to review this method, because I have no idea what it is supposed to do, so I have no idea if it is correct?

5. +++ b/core/modules/search/src/Plugin/migrate/process/d7/SearchPageStatus.php
   @@ -0,0 +1,35 @@
   +/**
   + * Transform search page status to a Boolean.
   + *
   + * @MigrateProcessPlugin(
   + *   id = "d7_search_page_status"
   + * )
   + */
   +class SearchPageStatus extends ProcessPluginBase {
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function transform($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
   +    list($module, $status) = $value;
   +    $transformed_status = TRUE;
   +    if (!$status) {
   +      $transformed_status = FALSE;
   +    }
   +    return $transformed_status;
   +  }
   +
   +}
   

   This whole class seems really silly to me.

   This transformer is just "if the property evaluates to TRUE it's TRUE, and if FALSE it's FALSE". ... You go to all this trouble to transform it using a class, when you could just put $status directly into the if() statement in SearchPageStatus::import() instead.

   I don't get it?

6. +++ b/core/modules/search/src/Plugin/migrate/source/d7/SearchPageStatus.php
   @@ -0,0 +1,72 @@
   +/**
   + *
   + * Iterate search_active_modules variable as an associative array.
   

   Extra blank line at the top of this doc block.

7. +++ b/core/modules/search/src/Plugin/migrate/source/d7/SearchPageStatus.php
   @@ -0,0 +1,72 @@
   +  public function getIds() {
   +    $ids['module']['type'] = 'string';
   +    return $ids;
   +  }
   

   Again, unable to review this getIds() method because I don't really know what it should be doing?

8. +++ b/core/modules/search/src/Tests/Migrate/d7/MigrateSearchPageStatusTest.php
   @@ -0,0 +1,50 @@
   +  public static $modules = array('node', 'search');
   

   Probably you also need the 'user' module?

9. +++ b/core/modules/search/tests/src/Unit/Plugin/migrate/process/d7/SearchPageStatusTest.php
   @@ -0,0 +1,53 @@
   +   * Test the transform process.
   

   Test => Tests

10. +++ b/core/modules/search/tests/src/Unit/Plugin/migrate/source/d7/SearchPageStatusTest.php
    @@ -0,0 +1,52 @@
    +  const PLUGIN_CLASS = 'Drupal\search\Plugin\migrate\source\d7\SearchPageStatus';
    

    No doc block?

11. +++ b/core/modules/search/tests/src/Unit/Plugin/migrate/source/d7/SearchPageStatusTest.php
    @@ -0,0 +1,52 @@
    +  protected $migrationConfiguration = [
    

    no doc block?

12. +++ b/core/modules/search/tests/src/Unit/Plugin/migrate/source/d7/SearchPageStatusTest.php
    @@ -0,0 +1,52 @@
    +  protected $expectedResults = [
    

    No doc block?

Regarding doc blocks... See
https://www.drupal.org/node/1354

I guess we don't have a specific section about documenting member variables or member constants, but you can look at other classes in Drupal Core for examples. One that has both constants and member variables is in
core/lib/Drupal/Core/StringTranslation/PluralTranslatableMarkup.php

So, this patch is looking better! A few more things to address:

1. +++ b/core/modules/search/src/Plugin/migrate/destination/d7/SearchPageStatus.php
   @@ -0,0 +1,52 @@
   + * Destination for search page status.
   + *
   

   I think I would add something to this documentation block saying that this is only usable for the core Node and User search pages, not for contrib modules.

   I realize it's only being called with those inputs, but that's off in some other class.

2. +++ b/core/modules/search/src/Plugin/migrate/destination/d7/SearchPageStatus.php
   @@ -0,0 +1,52 @@
   +    /** @var \Drupal\search\Entity\SearchPage $search_page */
   +    $id = $row->getDestinationProperty('module') . '_search';
   +    $search_page = SearchPage::load($id);
   

   That @var comment belongs maybe one row down?

   Also I think that I would add an in-code comment noting that this will only be certain to work for the core User and Node search page plugins, whose IDs happen to match this pattern.

   Either that or at least add some more if() statements etc. to check if $search_page actually exists after trying to load it.

3. +++ b/core/modules/search/src/Plugin/migrate/destination/d7/SearchPageStatus.php
   @@ -0,0 +1,52 @@
   +  }
   +  /**
   

   Nitpick: There should be a blank line between these two methods.

4. +++ b/core/modules/search/src/Plugin/migrate/process/d7/SearchPageStatus.php
   @@ -0,0 +1,35 @@
   +/**
   + * Transform search page status to a Boolean.
   + *
   + * @MigrateProcessPlugin(
   + *   id = "d7_search_page_status"
   + * )
   + */
   +class SearchPageStatus extends ProcessPluginBase {
   

   I thought you were getting rid of this class? Also I think there is a test for it lower down in the patch, which would also need to go away.

5. +++ b/core/modules/search/src/Plugin/migrate/source/d7/SearchPageStatus.php
   @@ -0,0 +1,73 @@
   +        ->condition('name', ['search_active_modules'], 'IN');
   

   Why is this condition using an array and IN? It should just be using a string an = I think, for efficiency?

6. +++ b/core/modules/search/src/Tests/Migrate/d7/MigrateSearchPageStatusTest.php
   @@ -0,0 +1,50 @@
   +  /**
   +   * The modules to be enabled during the test.
   +   *
   +   * {@inheritdoc}
   +   */
   +  public static $modules = array('node', 'search');
   

   Either use @inheritdoc or document the variable. Not both.

7. +++ b/core/modules/search/src/Tests/Migrate/d7/MigrateSearchPageStatusTest.php
   @@ -0,0 +1,50 @@
   +   * Tests Drupal 7 search status to Drupal 8 search status.
   

   Tests => Tests migrating

Thanks! Looking better.

1. +++ b/core/modules/search/src/Plugin/migrate/destination/d7/SearchPageStatus.php
   @@ -0,0 +1,57 @@
   + * The source plugin passes search status for core modules that use search,
   + * which are node and user.
   

   This documentation doesn't make sense to me.

   My suggestion would be:

   This plugin only works for the search page status of the core User and Node module plugins.

2. +++ b/core/modules/search/src/Plugin/migrate/destination/d7/SearchPageStatus.php
   @@ -0,0 +1,57 @@
   +    /** @var \Drupal\search\Entity\SearchPage $search_page */
   +    $id = $row->getDestinationProperty('module') . '_search';
   +    $search_page = SearchPage::load($id);
   +    if ($row->getDestinationProperty('status')) {
   

   Just a reminder of my earlier review of this section:

   a) Move the @var down one line.

   b) Verify that $search_page was not NULL after the load.

   c) Put in a code comment noting that this method of saying $id is equal to the module with _search appended is probably only valid for the search page plugins from core Node and User modules.

3. Also you mentioned you need to add one more test, for the destination plugin.

Note: I'm taking a vacation and will be away from Drupal-land for the next 3 weeks after today. Sorry for the inconvenience... I won't be reviewing patches on this issue again after today for a while.

Looking good! I'm back from my trip...

We're down to coding standards nitpicks now. :)

1. +++ b/core/modules/search/src/Plugin/migrate/destination/d7/SearchPageStatus.php
   @@ -0,0 +1,64 @@
   +    /*
   +     * Config name is constructed as <module_name>_search, which probably
   +     * only works for the Node and User module.
   +     */
   

   Coding standards: for in-code comments, use // not /* even for multi-line comments, in the Drupal project.

2. +++ b/core/modules/search/src/Plugin/migrate/destination/d7/SearchPageStatus.php
   @@ -0,0 +1,64 @@
   +    /** @var \Drupal\search\Entity\SearchPage $search_page */
   

   Again, this @var shoudl go next to the variable it is documenting, not 3 lines up.

Thanks!

+++ b/core/modules/search/src/Plugin/migrate/destination/d7/SearchPageStatus.php
@@ -0,0 +1,64 @@
+    //
+    // Config name is constructed as <module_name>_search, which probably
+    // only works for the Node and User module.
+    //

Almost! Please check out the comment standards at

https://www.drupal.org/node/1354#inline

- Comments should immediately precede the code they are documenting.
- No empty // lines

Yeah, we may not have written down a "no empty comment lines" standard, but if you look in Core, you won't see any. ;) It's pretty hard to have all of the conventions written down.

Anyway, this is looking good! It has tests, and is definitely doing the right thing regarding migrating D7 variables to D8 for Search module & associated Core modules.

Let's just set it back to RTBC and see if that clears up.

This seems to be assuming that the node module is enabled on the target site - it might not be...

Hm. I think this is right... but the tests are a bit strange now.

core/modules/search/tests/src/Unit/Plugin/migrate/source/d7/SearchPageStatusTest.php
This unit test I think verifies what the Source plugin does: that it correctly checks to see if the modules are turned on, and correctly checks to see if the D7 search_active_modules variable indicates the search page should be on or off.

core/modules/search/src/Tests/Migrate/d7/MigrateSearchPageStatusTest.php
This test is the same as it was before: it turns on both node/user modules, and it verifies that if the D7 search_active_modules variable says node search should be on and user search should be off, that the D8 plugin status ends up correct.

But we don't have a test that verifies that if the D7 search_active_modules variable said ON but the Node module is not installed, the plugin doesn't get turned on. It seems like we should?

Looks great! One very minor nitpick:

+++ b/core/modules/search/src/Plugin/migrate/destination/d7/SearchPageStatus.php
@@ -0,0 +1,65 @@
+    ¶

There seems to be some extra space on this line.

I think the rest is ready to go. Great work!

Looks good then, thanks!

testbot glitch

+++ b/core/modules/search/src/Plugin/migrate/destination/d7/SearchPageStatus.php
@@ -0,0 +1,64 @@
+    $id = $row->getDestinationProperty('module') . '_search';
+    /** @var \Drupal\search\Entity\SearchPage $search_page */
+    $search_page = SearchPage::load($id);

This looks a bit fragile... what happens if the search page has no equivalent in D8?

That won't happen. This only runs for modules 'node' and 'user', and the whole class is documented only to work for those.

I think it can happen. If for some reason node is not enabled on the target site but is on the source site then it is going to try to load it and it won't exist.

OK, let's see what happens in this process... As far as I can tell:

a) \Drupal\search\Plugin\migrate\source\d7\SearchPageStatus::initializeIterator()
==> Sets up node/user search pages for migration, if they are enabled on the source site (which means the modules and the search pages are both active in source site).

b) \Drupal\search\Plugin\migrate\source\d7\SearchPageStatus::prepareRow()
==> Runs a moduleExists check on the source site for node/user modules

c) \Drupal\search\Plugin\migrate\destination\d7\SearchPageStatus::import()

+    // Config name is constructed as <module_name>_search, which probably
+    // only works for the Node and User module.
+    $id = $row->getDestinationProperty('module') . '_search';
+    /** @var \Drupal\search\Entity\SearchPage $search_page */
+    $search_page = SearchPage::load($id);
+    if (!$row->getDestinationProperty('module_exists')) {
+      $search_page->delete();
+    }
+    else {
+      if ($row->getDestinationProperty('status')) {
+        $search_page->enable()->save();
+      }
+      else {
+        $search_page->disable()->save();
+      }
+    }
+  }

@alexpott is right -- this code is not quite correct.

If module node/user is not installed on the destination site, then the config item wouldn't exist either, SearchPage::load($id) would come up with a NULL, and $search_page->delete() would give you a PHP error. So probably just before $search_page->delete() it should verify that $search_page exists?

Another possibility is that the node/user module could be installed, but the user might have manually deleted the search page object before they ran the migrate. In this case... well I don't know what the standard response for that case would be for Migrate? Either you'd want to tell the person they've messed up the site and their search page config can't be migrated, or you'd want to create a new SearchPage with the default config (assuming that the page was enabled on the source site in the first place).

Anyway, agreed it still needs work.

OK... Reviewing the interdiff:

1. +++ b/core/modules/search/src/Plugin/migrate/destination/d7/SearchPageStatus.php
   @@ -22,22 +25,57 @@
   +   * Constructs a BlockedIP object.
   

   Oops. Copy/paste...
   BlockedIP => SearchPageStatus

2. +++ b/core/modules/search/src/Plugin/migrate/destination/d7/SearchPageStatus.php
   @@ -22,22 +25,57 @@
   +   * @param array $configuration
   +   *  Plugin configuration.
   

   Nitpick: All of these @param docs lines need one more space of indentation (the docs lines following @param lines that is).

3. +++ b/core/modules/search/src/Plugin/migrate/destination/d7/SearchPageStatus.php
   @@ -22,22 +25,57 @@
   +      $search_page = SearchPage::load($id);
          if ($row->getDestinationProperty('status')) {
            $search_page->enable()->save();
          }
   

   Again, I think here you also need to check that $search_page is not NULL. Just because the node/user module exists does not mean that the search page plugin that the module installed by default still exists. Admins can delete search pages.

   [And do we also need to verify that the Search module exists? You can't even create search plugins if not. Oh, yeah, duh, this migrate plugin is under the search module, so I guess it will only exist if Search is installed. OK, don't worry about that. :)]

   So I think what needs to be done here is something like:

   $search_page = SearchPage::load($id);
   if (!$search_page) {
      // create a new search page of type NodeSearch or UserSearch,
     // depending on which module you are doing
   }
   

   Then you can proceed to enable/disable this page. Although probably there isn't much point in creating the search page if you're just going to disable it, so I guess maybe it would be better to do this:

   // load it like before
   if ( ... status ) {
     if (doesn't exist ) {
        // create it
     }
     // enable it
   }
   else {
      if (exists) {
          // disable it
      }
   }
   

   Right?

OK, fair enough.

I cannot review this patch...

1. I got hung up on a number of files that I have no idea about, so I can't really review them at all:

   +++ b/core/modules/migrate_drupal/tests/fixtures/drupal7.php
   
   +++ b/core/modules/search/config/schema/search.source.schema.yml
   
   +++ b/core/modules/search/migration_templates/d6_search_page.yml
   
   +++ b/core/modules/search/migration_templates/d7_search_page.yml
   
   

2. And then I got here and saw this had gone back to what it was before:

   +++ b/core/modules/search/src/Plugin/migrate/destination/EntitySearchPage.php
   @@ -19,6 +20,26 @@
   +      $search_page = SearchPage::load($id);
   +      if ($row->getDestinationProperty('status')) {
   +        $search_page->enable()->save();
   +      }
   +      else {
   +        $search_page->disable()->save();
   +      }
   +      return [$id];
   +    }
   

   Once again....

   You really CANNOT assume that SearchPage::load() returned a non-NULL value. You need to check the return value. What happened to the code in previous patches that verified this?????

I stopped reviewing there. What happened to the patch?

I have no idea about the parent method, but in *this* method, it is trying to load the entity given only the ID, and it is definitely possible that this particular search page doesn't exist.

I think it would be very easy to make a failing test that would crash due to this: just delete, for instance, the UserSearch page before running the migrate.

I don't know what to tell you. How is this working? If you do a load on an ID that doesn't exist, you get a NULL result. Then if you try to access methods on the null object, how can it possibly not crash? I don't get it.

OK. Back to #52 item 2.

This method is calling SearchPage::load(). If the requested search page ID doesn't exist, it definitely returns NULL, or at least that is what the documentation of Entity::load() says, which is what SearchPage::load() is.

If you have a NULL you cannot then call methods on it.

Why is this logic incorrect? #53 is not relevant to the code I am talking about.

#51 needs re-roll

curl https://www.drupal.org/files/issues/2587063-51.patch | patch -p1
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 26010  100 26010    0     0  30276      0 --:--:-- --:--:-- --:--:-- 30279
patching file core/modules/migrate_drupal/tests/fixtures/drupal7.php
Hunk #1 succeeded at 41915 (offset 360 lines).
patching file core/modules/search/config/schema/search.source.schema.yml
patching file core/modules/search/migration_templates/d6_search_page.yml
patching file core/modules/search/migration_templates/d7_search_page.yml
patching file core/modules/search/migration_templates/search_page.yml
patching file core/modules/search/src/Plugin/migrate/destination/EntitySearchPage.php
Hunk #1 succeeded at 5 (offset -5 lines).
Hunk #2 succeeded at 15 (offset -5 lines).
Hunk #3 succeeded at 44 (offset -5 lines).
patching file core/modules/search/src/Plugin/migrate/process/SearchConfigurationRankings.php
Hunk #1 succeeded at 21 (offset -5 lines).
patching file core/modules/search/src/Plugin/migrate/process/d6/SearchConfigurationRankings.php
Reversed (or previously applied) patch detected!

Re-roll.

Correct parameter types.

Update $moduleUpgradePaths array.

Setting back to "Needs review" - appears to have been a test-bot glitch. Now passing all tests.

1. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   --- /dev/null
   +++ b/core/modules/search/config/schema/search.source.schema.yml
   

   Let's take this obsolete file out, it's leftover from the days when migrations in core were configuration entities.

2. +++ b/core/modules/search/tests/src/Kernel/Migrate/d6/MigrateSearchPageTest.php
   @@ -2,17 +2,20 @@
   +  use SchemaCheckTestTrait;
   +
   

   Also can be removed.

Changes in response to code review.

+++ b/core/modules/search/src/Plugin/migrate/destination/EntitySearchPage.php
@@ -23,7 +44,10 @@ class EntitySearchPage extends EntityConfigBase {
+    // The user_search plugin does not have a setConfiguraion method.

Nit: "setConfiguraion" (can be fixed on commit).

This looks good to me from a migration standpoint - I'd like someone more conversant with the search module to look it over, though, and give the RTBC if it looks good to them.

Stealing from @quietone for review.

1. +++ b/core/modules/search/src/Plugin/migrate/destination/EntitySearchPage.php
   @@ -14,6 +15,26 @@
   +    if (\Drupal::moduleHandler()->moduleExists($row->getDestinationProperty('module'))) {
   

   If possible, I'd rather this used ContainerFactoryPluginInterface to inject this service.

   Alternatively, could the source property module_exists be of any use here, rather than bother with the module handler?

2. +++ b/core/modules/search/src/Plugin/migrate/destination/EntitySearchPage.php
   @@ -14,6 +15,26 @@
   +      $search_page = SearchPage::load($id);
   

   Same here -- let's use the entity type manager (injected) to load the search page.

3. +++ b/core/modules/search/src/Plugin/migrate/destination/EntitySearchPage.php
   @@ -23,7 +44,10 @@ class EntitySearchPage extends EntityConfigBase {
   +    // The user_search plugin does not have a setConfiguraion method.
   

   Should be setConfiguration().

4. +++ b/core/modules/search/src/Plugin/migrate/process/SearchConfigurationRankings.php
   @@ -21,7 +21,7 @@ class SearchConfigurationRankings extends ProcessPluginBase {
   diff --git a/core/modules/search/src/Plugin/migrate/process/d6/SearchConfigurationRankings.php b/core/modules/search/src/Plugin/migrate/process/d6/SearchConfigurationRankings.php
   
   diff --git a/core/modules/search/src/Plugin/migrate/process/d6/SearchConfigurationRankings.php b/core/modules/search/src/Plugin/migrate/process/d6/SearchConfigurationRankings.php
   deleted file mode 100644
   

   Why did this get deleted? Is it just git being weird and mistaking a rename for a delete?

5. +++ b/core/modules/search/src/Plugin/migrate/source/d6/SearchPage.php
   @@ -0,0 +1,94 @@
   + * @MigrateSource(
   + *   id = "d6_search_page"
   + * )
   

   Why doesn't this define a source_provider?

6. +++ b/core/modules/search/src/Plugin/migrate/source/d6/SearchPage.php
   @@ -0,0 +1,94 @@
   +class SearchPage extends DrupalSqlBase {
   

   Would it be useful to extend the Variable source plugin instead of sorta-reinventing it here?

7. +++ b/core/modules/search/src/Plugin/migrate/source/d7/SearchPage.php
   @@ -0,0 +1,119 @@
   +class SearchPage extends DrupalSqlBase {
   

   Again, can this extend Variable?

8. +++ b/core/modules/search/src/Plugin/migrate/source/d7/SearchPage.php
   @@ -0,0 +1,119 @@
   +      // Create an ID field so we can record migration in the map table.
   

   Nit: Not indented properly.

9. +++ b/core/modules/search/src/Plugin/migrate/source/d7/SearchPage.php
   @@ -0,0 +1,119 @@
   +        $tmp = [
   +          'module' => $module,
   +          'status' => $status,
   +        ];
   

   If this is an ID, why isn't the status column mentioned in getIds()?

10. +++ b/core/modules/search/src/Plugin/migrate/source/d7/SearchPage.php
    @@ -0,0 +1,119 @@
    +        if ($module === 'node') {
    +          $tmp = array_merge($tmp, $rankings);
    +        }
    

    Why is this only happening for node? This definitely needs a comment.

11. +++ b/core/modules/search/tests/src/Kernel/Migrate/d7/MigrateSearchPageTest.php
    @@ -7,7 +7,7 @@
    + * Migrate search page status and settings.
    

    Should be "Tests migration of search page status and settings."

12. +++ b/core/modules/search/tests/src/Kernel/Migrate/d7/MigrateSearchPageTest.php
    @@ -18,34 +18,66 @@ class MigrateSearchPageTest extends MigrateDrupal7TestBase {
    +   * @param boolean $present
    +   *   True if the entity exists.
    

    Why would this be necessary? An assertEntity() method should *always* unconditionally assert that the entity exists.

13. +++ b/core/modules/search/tests/src/Kernel/Migrate/d7/MigrateSearchPageTest.php
    @@ -18,34 +18,66 @@ class MigrateSearchPageTest extends MigrateDrupal7TestBase {
    +   * @param string $path
    +   *   The expected image style label.
    

    Image style?

14. +++ b/core/modules/search/tests/src/Kernel/Migrate/d7/MigrateSearchPageTest.php
    @@ -18,34 +18,66 @@ class MigrateSearchPageTest extends MigrateDrupal7TestBase {
    +  protected function assertEntity($id, $present, $path, $status, array $expected_config = NULL) {
    

    The $path parameter isn't documented.

15. +++ b/core/modules/search/tests/src/Kernel/Migrate/d7/MigrateSearchPageTest.php
    @@ -18,34 +18,66 @@ class MigrateSearchPageTest extends MigrateDrupal7TestBase {
    +      $this->assertIdentical($id, $search_page->id());
    

    Use assertSame().

16. +++ b/core/modules/search/tests/src/Kernel/Migrate/d7/MigrateSearchPageTest.php
    @@ -18,34 +18,66 @@ class MigrateSearchPageTest extends MigrateDrupal7TestBase {
    +      if ($expected_config) {
    

    Should be isset($expected_config), since this will be FALSE for an empty array, which may well be a valid configuration.

17. +++ b/core/modules/search/tests/src/Unit/Plugin/migrate/source/d6/SearchPageTest.php
    @@ -0,0 +1,68 @@
    +class SearchPageTest extends MigrateSqlSourceTestCase {
    

    This should be rewritten on top of MigrateSqlSourceTestBase.

18. +++ b/core/modules/search/tests/src/Unit/Plugin/migrate/source/d7/SearchPageTest.php
    @@ -0,0 +1,84 @@
    +class SearchPageTest extends MigrateSqlSourceTestCase {
    

    Ditto.

1. Service injected (property module_exists could not be used because it sometimes contained different values).
2. Service injected.
3. () added.
4. Not sure why that was deleted - can someone else address this, please?
5. Source provider has not been defined because it should be 'search', but the search module is not enabled in the D6 fixture so the test fails to run. How could this be fixed?
6. Extended the Variable plugin.
7. Extended the Variable plugin.
8. Indentation corrected.
9. 'status' added to getIds().
10. I'm not sure about this - can someone else address this, please?
11. Comment corrected.
12. assertEntity() no longer used to check for absent entities, code refactored accordingly.
13. Documentation corrected.
14. Documentation corrected (see 13).
15. assertIdentical() replaced with assertSame().
16. isset() used as recommended.
17. Now extends MigrateSqlSourceTestBase.
18. Now extends MigrateSqlSourceTestBase.

So just 4. & 10. remaining to address.

Much better. Thanks, @Jo Fitzgerald! Two small things:

1. +++ b/core/modules/search/src/Plugin/migrate/destination/EntitySearchPage.php
   @@ -15,14 +18,67 @@
   +  /**
   +   * The search page.
   +   *
   +   * @var \Drupal\search\Entity\SearchPage
   +   */
   +  protected $searchPage;
   

   Forgive me if I'm not seeing it, but where is this used? Also, the type hint needs to be an interface, if one exists.

2. +++ b/core/modules/search/src/Plugin/migrate/destination/EntitySearchPage.php
   @@ -15,14 +18,67 @@
   +   * Construct a new entity.
   

   This is kind of a misleading comment. Should be "Constructs a new EntitySearchPage".

Kicking back to NW for #79.4 and #79.10.

79.4 The same MigrateProcessPlugin is used for migrations from D6 & D7 so d6/SearchConfigurationRankings.php is redundant - it probably should have been deleted as part of #2569245: [D7] Migrate search pages.

79.10 Comment added to clarify why rankings are only added for 'node' (it's because the variables are node_rank_* which are only relevant to node). Also tweaked the code to improve performance.

81.1 Removed redundant $searchPage.

81.2 Corrected comment.

Oh, we are very nearly there. I like it. Nice work!

+++ b/core/modules/search/src/Plugin/migrate/source/d7/SearchPage.php
@@ -34,13 +34,15 @@ protected function initializeIterator() {
     $rankings = array_map('unserialize', $this->prepareQuery()->execute()->fetchAllKeyed());

Correct me if I'm wrong, but couldn't we do $rankings = array_map('unserialize', parent::values()) here?

We couldn't do $rankings = array_map('unserialize', parent::values()), but $rankings = parent::values(); seems to work!

Great!

Committed and pushed 0947fae to 8.4.x and 08e8437 to 8.3.x. Thanks!

diff --git a/core/modules/search/src/Plugin/migrate/destination/EntitySearchPage.php b/core/modules/search/src/Plugin/migrate/destination/EntitySearchPage.php
index 99d9ef2..a3e3d8e 100644
--- a/core/modules/search/src/Plugin/migrate/destination/EntitySearchPage.php
+++ b/core/modules/search/src/Plugin/migrate/destination/EntitySearchPage.php
@@ -93,7 +93,7 @@ public function import(Row $row, array $old_destination_id_values = array()) {
    */
   protected function updateEntity(EntityInterface $entity, Row $row) {
     $entity->setPlugin($row->getDestinationProperty('plugin'));
-    // The user_search plugin does not have a setConfiguraion() method.
+    // The user_search plugin does not have a setConfiguration() method.
     if (method_exists($entity->getPlugin(), 'setConfiguration')) {
       $entity->getPlugin()->setConfiguration($row->getDestinationProperty('configuration'));
     }

diff --git a/core/modules/search/src/Plugin/migrate/source/d7/SearchPage.php b/core/modules/search/src/Plugin/migrate/source/d7/SearchPage.php
index c31cd84..ebfb744 100644
--- a/core/modules/search/src/Plugin/migrate/source/d7/SearchPage.php
+++ b/core/modules/search/src/Plugin/migrate/source/d7/SearchPage.php
@@ -13,7 +13,7 @@
  *   source_provider = "search"
  * )
  */
-class SearchPage extends Variable  {
+class SearchPage extends Variable {
 
   /**
    * {@inheritdoc}
diff --git a/core/modules/search/tests/src/Kernel/Migrate/d6/MigrateSearchPageTest.php b/core/modules/search/tests/src/Kernel/Migrate/d6/MigrateSearchPageTest.php
index 113d4d9..ddf41ef 100644
--- a/core/modules/search/tests/src/Kernel/Migrate/d6/MigrateSearchPageTest.php
+++ b/core/modules/search/tests/src/Kernel/Migrate/d6/MigrateSearchPageTest.php
@@ -2,7 +2,6 @@
 
 namespace Drupal\Tests\search\Kernel\Migrate\d6;
 
-use Drupal\config\Tests\SchemaCheckTestTrait;
 use Drupal\Core\Database\Database;
 use Drupal\Tests\migrate_drupal\Kernel\d6\MigrateDrupal6TestBase;
 use Drupal\search\Entity\SearchPage;
diff --git a/core/modules/search/tests/src/Kernel/Migrate/d7/MigrateSearchPageTest.php b/core/modules/search/tests/src/Kernel/Migrate/d7/MigrateSearchPageTest.php
index 6010059..5356a2d 100644
--- a/core/modules/search/tests/src/Kernel/Migrate/d7/MigrateSearchPageTest.php
+++ b/core/modules/search/tests/src/Kernel/Migrate/d7/MigrateSearchPageTest.php
@@ -34,7 +34,7 @@ protected function setUp() {
    *   The expected search page ID.
    * @param string $path
    *   The expected path of the search page.
-   * @param boolean $status
+   * @param bool $status
    *   The expected status of the search page.
    * @param array $expected_config
    *   An array of expected configuration for the search page.
@@ -113,4 +113,5 @@ public function testUserSearchCreate() {
 
     $this->assertEntity('user_search', 'user');
   }
+
 }

Coding standards fixed on commit.

Unfortunately, this broke HEAD. According to @tim.plunkett, it's due to conflicts with #2225717: Add config translation support to migrations and implement for Drupal 6 user profile fields which was also committed today. That issue seems way more valuable, so reverted this one.

Thanks!

The other issue expanded \Drupal\migrate\Plugin\migrate\destination\EntityConfigBase::__construct() to require LanguageManagerInterface $language_manager, ConfigFactoryInterface $config_factory, see failures here:
https://www.drupal.org/pift-ci-job/596762

Added missing dependency injection.

Looks like the constructor is fixed and tests are happy, so...re-RTBC.

Committed and pushed 8deb904 to 8.4.x and aa06720 to 8.3.x. Thanks!

Committed to 8.3.x as migrate is experimental.

diff --git a/core/modules/search/src/Plugin/migrate/destination/EntitySearchPage.php b/core/modules/search/src/Plugin/migrate/destination/EntitySearchPage.php
index 24cf4fe..52402e6 100644
--- a/core/modules/search/src/Plugin/migrate/destination/EntitySearchPage.php
+++ b/core/modules/search/src/Plugin/migrate/destination/EntitySearchPage.php
@@ -101,7 +101,7 @@ public function import(Row $row, array $old_destination_id_values = array()) {
    */
   protected function updateEntity(EntityInterface $entity, Row $row) {
     $entity->setPlugin($row->getDestinationProperty('plugin'));
-    // The user_search plugin does not have a setConfiguraion() method.
+    // The user_search plugin does not have a setConfiguration() method.
     if (method_exists($entity->getPlugin(), 'setConfiguration')) {
       $entity->getPlugin()->setConfiguration($row->getDestinationProperty('configuration'));
     }
diff --git a/core/modules/search/src/Plugin/migrate/source/d7/SearchPage.php b/core/modules/search/src/Plugin/migrate/source/d7/SearchPage.php
index c31cd84..ebfb744 100644
--- a/core/modules/search/src/Plugin/migrate/source/d7/SearchPage.php
+++ b/core/modules/search/src/Plugin/migrate/source/d7/SearchPage.php
@@ -13,7 +13,7 @@
  *   source_provider = "search"
  * )
  */
-class SearchPage extends Variable  {
+class SearchPage extends Variable {
 
   /**
    * {@inheritdoc}
diff --git a/core/modules/search/tests/src/Kernel/Migrate/d6/MigrateSearchPageTest.php b/core/modules/search/tests/src/Kernel/Migrate/d6/MigrateSearchPageTest.php
index 113d4d9..ddf41ef 100644
--- a/core/modules/search/tests/src/Kernel/Migrate/d6/MigrateSearchPageTest.php
+++ b/core/modules/search/tests/src/Kernel/Migrate/d6/MigrateSearchPageTest.php
@@ -2,7 +2,6 @@
 
 namespace Drupal\Tests\search\Kernel\Migrate\d6;
 
-use Drupal\config\Tests\SchemaCheckTestTrait;
 use Drupal\Core\Database\Database;
 use Drupal\Tests\migrate_drupal\Kernel\d6\MigrateDrupal6TestBase;
 use Drupal\search\Entity\SearchPage;
diff --git a/core/modules/search/tests/src/Kernel/Migrate/d7/MigrateSearchPageTest.php b/core/modules/search/tests/src/Kernel/Migrate/d7/MigrateSearchPageTest.php
index 6010059..5356a2d 100644
--- a/core/modules/search/tests/src/Kernel/Migrate/d7/MigrateSearchPageTest.php
+++ b/core/modules/search/tests/src/Kernel/Migrate/d7/MigrateSearchPageTest.php
@@ -34,7 +34,7 @@ protected function setUp() {
    *   The expected search page ID.
    * @param string $path
    *   The expected path of the search page.
-   * @param boolean $status
+   * @param bool $status
    *   The expected status of the search page.
    * @param array $expected_config
    *   An array of expected configuration for the search page.
@@ -113,4 +113,5 @@ public function testUserSearchCreate() {
 
     $this->assertEntity('user_search', 'user');
   }
+
 }

Fixes on commit. Would have been nice if these had been rolled into #94.

This issue cause a test fail due to sorting in PHP7 - array sorts can be non-deterministic. https://www.drupal.org/pift-ci-job/607300

The random fail is happening because the order of the ranking configuration is not enforced by the NodeSearch plugin. We should file a separate issue to address this. In an ideal world we could just leverage #2361539: Config export key order is not predictable for sequences, add orderby property to config schema but that's not part of core yet.

#2361539: Config export key order is not predictable for sequences, add orderby property to config schema is now in so ... @alexpott how can we make use of it?

Re-rolled before addressing sorting issue.

Needs reroll.

rerolled :)

Re-rolled #110.

Corrected the test failure and coding standards errors.

1. +++ b/core/modules/search/migrations/d7_search_page.yml
   @@ -0,0 +1,47 @@
   diff --git a/core/modules/search/src/Plugin/migrate/destination/EntitySearchPage.php b/core/modules/search/src/Plugin/migrate/destination/EntitySearchPage.php
   
   --- a/core/modules/search/src/Plugin/migrate/destination/EntitySearchPage.php
   +++ b/core/modules/search/src/Plugin/migrate/destination/EntitySearchPage.php
   

   I don't see we need this destination at all. Let's map the data directly to pluginCollection, instead of having the technical debt of a new destination plugin.

2. +++ b/core/modules/search/src/Plugin/migrate/destination/EntitySearchPage.php
   @@ -23,7 +103,10 @@ class EntitySearchPage extends EntityConfigBase {
   +    // The user_search plugin does not have a setConfiguration() method.
   +    if (method_exists($entity->getPlugin(), 'setConfiguration')) {
   +      $entity->getPlugin()->setConfiguration($row->getDestinationProperty('configuration'));
   +    }
   

   Do we need to set the config without the setter then?

3. +++ b/core/modules/search/src/Plugin/migrate/process/SearchConfigurationRankings.php
   @@ -21,7 +21,7 @@ class SearchConfigurationRankings extends ProcessPluginBase {
   diff --git a/core/modules/search/src/Plugin/migrate/process/d6/SearchConfigurationRankings.php b/core/modules/search/src/Plugin/migrate/process/d6/SearchConfigurationRankings.php
   
   diff --git a/core/modules/search/src/Plugin/migrate/process/d6/SearchConfigurationRankings.php b/core/modules/search/src/Plugin/migrate/process/d6/SearchConfigurationRankings.php
   deleted file mode 100644
   
   +++ /dev/null
   @@ -1,33 +0,0 @@
   -class SearchConfigurationRankings extends ProcessPluginBase {
   

   We cannot delete things without leaving a BC layer, deprecations and CR.

4. +++ b/core/modules/migrate_drupal/tests/fixtures/drupal7.php
   @@ -47399,7 +47399,7 @@
   +  'value' => 'a:2:{s:4:"node";s:4:"node";s:4:"user";i:0;}',
   

   Does this mean we don't test migrating users as an active search? Seems odd we'd want to drop test coverage.

I'll check what's left to be done here.

Assigning to myself for a re-review.

Still needs a CR.

1. +++ b/core/modules/search/src/Plugin/migrate/destination/EntitySearchPage.php
   @@ -14,6 +22,78 @@
   +  public function import(Row $row, array $old_destination_id_values = []) {
   

   This entire import seems un-necessary. Parent should be able to load the previous, no?

2. +++ b/core/modules/search/src/Plugin/migrate/destination/EntitySearchPage.php
   @@ -14,6 +22,78 @@
   +    if ($this->moduleHandler->moduleExists($row->getDestinationProperty('module'))) {
   

   Should we move the search migration ymls and split them into user and node module so we can use use normal requirements checks and not have this down in the destination plugin? Hmm, no. I don't think we can, because then the destination plugin might be missing and that would cause a fatal. And we don't want to check the destination from the source, so perhaps this is the best way to handle this. Could use some comments though, why.

3. +++ b/core/modules/search/src/Plugin/migrate/destination/EntitySearchPage.php
   @@ -23,7 +103,10 @@ class EntitySearchPage extends EntityConfigBase {
   +    if (method_exists($entity->getPlugin(), 'setConfiguration')) {
   

   Do a check if instance of ConfigurableSearchPluginBase instead?

4. +++ b/core/modules/search/src/Plugin/migrate/destination/EntitySearchPage.php
   @@ -14,6 +22,78 @@
   +      if ($row->getDestinationProperty('status')) {
   +        $search_page->enable()->save();
   +      }
   +      else {
   +        $search_page->disable()->save();
   +      }
   

   This should be do-able without this php code. Normal process mapping should suffice, no?

5. +++ b/core/modules/search/src/Plugin/migrate/source/d6/SearchPage.php
   @@ -0,0 +1,46 @@
   +    return array_merge(['module' => 'node'], parent::values());
   

   On d6, do we always have node? Maybe a comment why/what we are doing here.

6. +++ b/core/modules/search/src/Plugin/migrate/source/d7/SearchPage.php
   @@ -0,0 +1,89 @@
   +  protected function initializeIterator() {
   +    return new \ArrayIterator($this->values());
   

   Why \ArrayIterator here but not in D6?

7. +++ b/core/modules/search/src/Plugin/migrate/source/d7/SearchPage.php
   @@ -0,0 +1,89 @@
   +    $ids['module']['type'] = 'string';
   +    $ids['status']['type'] = 'string';
   

   I don't think we need both of these values for the id, do we? Just the module seems enough.

125.1 you can safely ignore. I was ignoring the destination module check in there. c.f:

if ($this->moduleHandler->moduleExists($row->getDestinationProperty('module'))) {

If this comes back green, then I think we can safely RTBC this thing.

1. +++ /dev/null
   @@ -1,32 +0,0 @@
   - * @deprecated in Drupal 8.7.x and will be removed in Drupal 9.0.x.
   - * Use \Drupal\migrate_drupal\Plugin\migrate\source\d6\VariableTranslation.
   - *
   ...
   - *
   

   I don't think we want to remove this? It just got committed.

2. +++ b/core/modules/search/src/Plugin/migrate/process/d6/SearchConfigurationRankings.php
   @@ -2,9 +2,10 @@
   +@trigger_error('SearchConfigurationRankings is deprecated in
   

   This should go into a CR and be linked to here.

Doing all that extra work in the import method was bugging me. I tracked it down and here's an updated patch. That was the only blocker from me for RTBC.

1. +++ b/core/modules/migrate_drupal/tests/fixtures/drupal7.php
   similarity index 87%
   rename from core/modules/search/migrations/search_page.yml
   
   rename from core/modules/search/migrations/search_page.yml
   rename to core/modules/search/migrations/d6_search_page.yml
   

   So technically, this is a backwards compatibility break because we are changing a plugin ID ('search_page' to 'd6_search_page'). We might need to add an alter hook which aliases search_page to d6_search_page.

2. +++ b/core/modules/search/migrations/d7_search_page.yml
   @@ -0,0 +1,47 @@
   +  status:
   +    -
   +      plugin: static_map
   +      source: status
   +      map:
   +        node: true
   +        user: true
   +        0: false
   

   Seems to me like _anything_ except 'node', and 'user' should be false, no?

3. +++ b/core/modules/search/src/Plugin/migrate/destination/EntitySearchPage.php
   @@ -2,17 +2,90 @@
   +    if ($this->moduleHandler->moduleExists($row->getDestinationProperty('module'))) {
   +      return parent::import($row, $old_destination_id_values);
   +    }
   +    return [];
   

   If we return an empty array, will the row be correctly recorded as having been skipped/invalid? Or do we need to throw MigrateSkipRowException instead?

4. +++ b/core/modules/search/src/Plugin/migrate/destination/EntitySearchPage.php
   @@ -22,8 +95,12 @@ class EntitySearchPage extends EntityConfigBase {
   +    if (is_a($entity->getPlugin(), ConfigurableSearchPluginBase::class)) {
   

   I think instanceof would be clearer here. Also, I'm not a fan of repeated calls to $entity->getPlugin(). Can we just call it one and reuse the return value?

5. +++ b/core/modules/search/src/Plugin/migrate/process/d6/SearchConfigurationRankings.php
   @@ -2,32 +2,28 @@
   +use Drupal\search\Plugin\migrate\process\SearchConfigurationRankings as CommonSearchConfigurationRankings;
   

   I think "Common" is a weird word to use here. How about "BaseSearchConfigurationRankings" instead?

6. +++ b/core/modules/search/src/Plugin/migrate/process/d6/SearchConfigurationRankings.php
   @@ -2,32 +2,28 @@
   +    @trigger_error('SearchConfigurationRankings is deprecated in Drupal 8.7.x and will be removed before Drupal 9.0.0. Use Drupal\search\Plugin\migrate\process\SearchConfigurationRankings instead.', E_USER_DEPRECATED);
   

   I think we need a link to the change record.

7. +++ b/core/modules/search/src/Plugin/migrate/source/d6/SearchPage.php
   @@ -0,0 +1,47 @@
   +   *   Add a module key to identify the source search provider, node. This value
   +   *   is used in the EntitySearchPage destination plugin.
   

   It's not clear why 'node' is hard-coded.

8. +++ b/core/modules/search/src/Plugin/migrate/source/d6/SearchPage.php
   @@ -0,0 +1,47 @@
   +      'module' => $this->t('The search module.'),
   

   This is a really ambiguous description. How about "The module providing a search page"?

9. +++ b/core/modules/search/src/Plugin/migrate/source/d7/SearchPage.php
   @@ -0,0 +1,88 @@
   +    foreach (['node', 'user'] as $module) {
   

   Not a huge fan of hard-coding this (would rather it was part of the plugin configuration), but we don't have to fix that right now.

re 140.1: this isn't a BC break, because core only supports discovery via migrate_drupal_ui. And contrib uses things like migrate_upgrade or migrate_run, which doen't care about the source name. And if you make changes or depend on changes in the source plugin w/ migrate_upgrade you've grabbed a place in time copy of that yaml. In which case, you have a copy. And any changes to the name of the source plugin still don't matter.

TLDR; changing the name of a source plugin doesn't break BC. Nothing references it.

Back to NW for the todos in 142.

+++ b/core/modules/search/migrations/d7_search_page.yml
@@ -28,6 +28,7 @@ process:
         0: false
+      default_value: false

Nit: this zero option isn't needed then.

But otherwise, this is ready.

Thanks, @quietone.

Looks good. Only found these two, the second one is very minor :)

1. rename from core/modules/search/migrations/search_page.yml
   rename to core/modules/search/migrations/d6_search_page.yml
   @@ -1,11 +1,10 @@
   -id: search_page
   +id: d6_search_page
   

   Is a named migration part of our API surface? If so, we should not be renaming it but add a new one and deprecate the old one.

2. +++ b/core/modules/search/src/Plugin/migrate/source/d7/SearchPage.php
   @@ -0,0 +1,83 @@
   +    foreach (['node', 'user'] as $module) {
   +      if (isset($search_active_modules[$module])) {
   +        // Add a module key to identify the source search provider, node. This
   +        // value is used in the EntitySearchPage destination plugin.
   

   Not just for node. (In the comment).

So looks like #148/1 was raised by @phenaproxima as well.

#1 we discussed back in #141. It isn't part of the API. Do we need to make it more explicit somewhere?

#2 addressed here.

@heddn: what you are referring to is the change of the plugin ID, not the change of the migration name. The migration name is used in dependency declaration situations for example, and therefore any migrations that depended on this migration will then have a missing dependency, no?

All of the dependencies are also built at the same time as the rest of everything. Its like taking template and cloning it. However, mikelutz had a good point in Slack, we now let incremental migrations via the web UI and who knows what would happen for them. So NW and change back to the old name for the d6 migration is probably the easiest path forward.

The next step here is to remove the rename and keep its old name.

We're good again now.

Committed 566dce2 and pushed to 8.7.x. Thanks!

This looks ready now. Third time lucky.

This introduced PostgreSQL failure:
- https://www.drupal.org/pift-ci-job/1147872
- https://www.drupal.org/pift-ci-job/1147909

Drupal\Tests\search\Kernel\Migrate\d7\MigrateSearchPageTest::testSearchPage
Failed asserting that Array &0 (
    'rankings' => Array &1 (
        'promote' => 0
        'relevance' => 2
        'sticky' => 0
        'views' => 0
        'comments' => 4
    )
) is identical to Array &0 (
    'rankings' => Array &1 (
        'comments' => 4
        'promote' => 0
        'relevance' => 2
        'sticky' => 0
        'views' => 0
    )
).

/var/www/html/core/modules/search/tests/src/Kernel/Migrate/d7/MigrateSearchPageTest.php:43
/var/www/html/core/modules/search/tests/src/Kernel/Migrate/d7/MigrateSearchPageTest.php:79

Since this has been committed and reverted several times and there's nothing functionally wrong I'd be in favour of opening a follow-up issue and fixing this there.

Opened #3021838: [PostgresSQL] Variable to config: search_active_modules.