The D6 node builder (which creates multiple migrations such as d6_node__blog, d6_node__forum, etc. from the d6_node template), while obviously using the node type to set a unique ID for each migrations, does not alter the "Drupal 6 nodes" label. Thus, a UI reporting on what's happening will show several "Imported Drupal 6 nodes" lines in a row. The type should be added into the label.

Comments

mikeryan created an issue. See original summary.

mikeryan’s picture

Status: Active » Needs review
StatusFileSize
new2.43 KB

Even the test is simple...

mikeryan’s picture

StatusFileSize
new2.43 KB

Don't know what I was thinking...

quietone’s picture

Nice, really like extra information.

My only question is the format of the message. Is that the standard way, to have the additional detail in parenthesis? When I first read the message I thought why not "Imported Drupal 6 nodes - page" instead of
"Imported Drupal 6 nodes (page)".

+1 RTBC

phenaproxima’s picture

Status: Needs review » Needs work
+++ b/core/modules/node/src/Tests/Migrate/d6/MigrateNodeBuilderTest.php
@@ -23,6 +23,7 @@ public function testCreateMigrations() {
@@ -53,6 +54,17 @@ public function testCreateMigrations() {

@@ -53,6 +54,17 @@ public function testCreateMigrations() {
     $this->assertIdentical('d6_node__test_page', $migrations[8]->id());
     $this->assertIdentical('d6_node__test_planet', $migrations[9]->id());
     $this->assertIdentical('d6_node__test_story', $migrations[10]->id());
+    $this->assertIdentical('Drupal 6 nodes (article)', $migrations[0]->label());
+    $this->assertIdentical('Drupal 6 nodes (company)', $migrations[1]->label());
+    $this->assertIdentical('Drupal 6 nodes (employee)', $migrations[2]->label());
+    $this->assertIdentical('Drupal 6 nodes (event)', $migrations[3]->label());
+    $this->assertIdentical('Drupal 6 nodes (page)', $migrations[4]->label());
+    $this->assertIdentical('Drupal 6 nodes (sponsor)', $migrations[5]->label());
+    $this->assertIdentical('Drupal 6 nodes (story)', $migrations[6]->label());
+    $this->assertIdentical('Drupal 6 nodes (test_event)', $migrations[7]->label());
+    $this->assertIdentical('Drupal 6 nodes (test_page)', $migrations[8]->label());
+    $this->assertIdentical('Drupal 6 nodes (test_planet)', $migrations[9]->label());
+    $this->assertIdentical('Drupal 6 nodes (test_story)', $migrations[10]->label());

I'd rather that we used the assertEntity() pattern I established in other tests to prevent repeating code. Something like:

$this->assertMigration($migration_entity, $expected_id, $expected_label);

Looks great otherwise.

quietone’s picture

Assigned: Unassigned » quietone

If my fingers are fast enough I might be able to finish this before I have to work in the garden.

quietone’s picture

Status: Needs work » Needs review
StatusFileSize
new3.88 KB
new3.74 KB

After uploading I reread that using assertMigration was suggested, I used assertEntity.

phenaproxima’s picture

Two minor things, then it's RTBC.

  1. +++ b/core/modules/node/src/Tests/Migrate/d6/MigrateNodeBuilderTest.php
    @@ -17,12 +18,28 @@ class MigrateNodeBuilderTest extends MigrateDrupal6TestBase {
    +   * Asserts various aspects of a migration
    

    Nit: missing a period. Also, can this say "migration entity", just for clarity?

  2. +++ b/core/modules/node/src/Tests/Migrate/d6/MigrateNodeBuilderTest.php
    @@ -17,12 +18,28 @@ class MigrateNodeBuilderTest extends MigrateDrupal6TestBase {
    +   *   The username.
    

    Username? :)

quietone’s picture

StatusFileSize
new703 bytes
new3.88 KB

Both fixed.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

#7 has passed DrupalCI and if #9 doesn't, I'll eat my socks. RTBC from me.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Much better, thanks!

Committed and pushed to 8.0.x. W00t!

  • webchick committed 7f3d413 on 8.0.x
    Issue #2558839 by quietone, mikeryan, phenaproxima: Node builder should...

Status: Fixed » Closed (fixed)

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

quietone’s picture

Assigned: quietone » Unassigned