Support from Acquia helps fund testing for Drupal Acquia logo

Comments

oadaeh’s picture

Issue tags: +Needs tests
FileSize
804 bytes

Attached is the YAML for this issue, from the patch in #2382117: Migration Files for Drupal 7 Variables.
Test(s) (and maybe a dump file) still need to be written.

oadaeh’s picture

Status: Active » Needs work
miguelc303’s picture

Added organization support to Anexus IT

jcost’s picture

Project: IMP » Drupal core
Version: » 8.0.x-dev
Component: Code » migration system

Will need to be submitted again to Core since moving from sandbox.

phenaproxima’s picture

phenaproxima’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3.07 KB

Updated, with a test.

benjy’s picture

Status: Needs review » Reviewed & tested by the community

Looks fine to me, just a missing comment on testMigration if someone wants to fix that.

The last submitted patch, 1: menu_settings-2353763-1.patch, failed testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
phenaproxima’s picture

Status: Needs work » Needs review
FileSize
2.96 KB
1.4 KB

Re-rolled.

Status: Needs review » Needs work

The last submitted patch, 10: 2353763-10.patch, failed testing.

phenaproxima queued 10: 2353763-10.patch for re-testing.

The last submitted patch, 10: 2353763-10.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
2.96 KB

Changed test group.

phenaproxima’s picture

Re-rolling. I can't create an interdiff because of conflicts between the two patches (due to the unmergeability of the {variable} table dump).

phenaproxima’s picture

Re-rolled again. No interdiff for the same reason I mentioned in #15.

phenaproxima’s picture

And yet again.

phenaproxima’s picture

EDIT: Um. Clicked Save on an empty comment. That was durmb.

mikeryan’s picture

FileSize
2.73 KB

Here's a diff between the D6 and D7 menu_settings migrations.

mikeryan’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/menu_ui/migration_templates/d7_menu_settings.yml
    @@ -1,16 +1,12 @@
    -    - menu_primary_links_source
    -    - menu_secondary_links_source
    

    I assume these variables existed in D6 but not D7?

  2. +++ b/core/modules/menu_ui/src/Tests/Migrate/d7/MigrateMenuSettingsTest.php
    @@ -2,46 +2,34 @@
    + * Contains \Drupal\menu_ui\Tests\d7\Migrate\MigrateMenuSettingsTest.
    

    Migrate <=> d7

  3. +++ b/core/modules/menu_ui/src/Tests/Migrate/d7/MigrateMenuSettingsTest.php
    @@ -2,46 +2,34 @@
    -class MigrateMenuConfigsTest extends MigrateDrupal6TestBase {
    +class MigrateMenuSettingsTest extends MigrateDrupal7TestBase {
    

    Why the different class name?

  4. +++ b/core/modules/menu_ui/src/Tests/Migrate/d7/MigrateMenuSettingsTest.php
    @@ -2,46 +2,34 @@
    +    $this->installConfig(['menu_ui']);
    

    Why is this necessary for D7 but not D6?

  5. +++ b/core/modules/menu_ui/src/Tests/Migrate/d7/MigrateMenuSettingsTest.php
    @@ -2,46 +2,34 @@
    -  public function testMenuSettings() {
    -    $config = $this->config('menu_ui.settings');
    -    $this->assertIdentical(FALSE, $config->get('override_parent_selector'));
    -    $this->assertConfigSchema(\Drupal::service('config.typed'), 'menu_ui.settings', $config->get());
    +  public function testMigration() {
    +    $this->assertTrue(\Drupal::config('menu_ui.settings')->get('override_parent_selector'));
    

    Why the test function name change (testMenuSettings -> testMigration)?

    The D6 test is looking for a FALSE value, but the D7 test is looking for a TRUE value?

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
4.17 KB
3.96 KB

The menu_primary_links_source and menu_secondary_links_source variables existed in both D6 and D7. However, they have no counterpart in D8. The D6 migration was mistaken. Once I removed those variables from the D6 migration, though, I realized that the d6_menu_settings and d7_menu_settings migrations were identical. So this patch condenses them into a single migration tagged for both Drupal 6 and Drupal 7.

  1. Removed, as explained above.
  2. Fixed -- there's now only one test, in Drupal\menu_ui\Tests\Migrate.
  3. Because it follows the name of the destination config object. This is consistent with what the System module's several variable-to-config migrations do, and I'm in favor of adopting that pattern going forward.
  4. I don't know, but it seems like the smart thing to do. Before migrating configuration, the destination config object should be made available, no?
  5. The D6 and D7 migrations were looking for different values because of the content of the {variable} table dump. It now looks only for TRUE, and the one test is based on the Drupal 7 dump data. The name of the method changed for no particular reason; I had probably originally copy-and-pasted the test from another place (not related to menu_ui), and changed the method name as a matter of course.
chx’s picture

Status: Needs review » Reviewed & tested by the community

discussed with phenaproxima and he added those two to the known issues. if there's nothing to migrate them to, well.

  • webchick committed 41b6af5 on 8.0.x
    Issue #2353763 by phenaproxima, oadaeh: Variable to config: menu....
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Hm, right. Those are just straight-up blocks now. Yay for less one-off crap!

Committed and pushed to 8.0.x. Thanks!

Status: Fixed » Closed (fixed)

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

-enzo-’s picture

@webchick I dont if is possible to amend the attribution in this commit, because the original patch created by @miguelc303 and myself but splited by @oadaeh from issue #2382117: Migration Files for Drupal 7 Variables