Problem/Motivation

After see a few new issues about body content not being migrated I looked at MigrateNodeTest and didn't see a test for the migration of the node body.

Proposed resolution

Add tests for D6 and D7.

Remaining tasks

If needed, add data to dump files.
Write the tests, presumably in d7/MigrateNodeTest.php and d6/MigrateNodeTest

User interface changes

N/A

API changes

N/A

Data model changes

N/A

CommentFileSizeAuthor
#12 add_tests_for_node_body-2635242-12.patch875 bytesalvar0hurtad0
#5 2635242-5.patch894 bytesquietone
#5 interdiff-2635242-2-5.txt655 bytesquietone
#2 2635242-2.patch923 bytesquietone
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quietone created an issue. See original summary.

quietone’s picture

quietone CreditAttribution: quietone commented
FileSize
2635242-2.patch923 bytes

There is a test for the node body value Drupal 6 migrations in d6/MigrateNodeTest.php. But there isn't a similar on for Drupal 7 migrations.

Drupal 7 MigrateNodeTest migrations test_content_type which does not have a body field. Adding a body field to that by the web interface caused a WSOD. Maybe time to work on #2578483: [meta] Keep dumps up-to-date and improve DX. An easier solution was to add a migration of the article type and test that.

quietone’s picture

quietone CreditAttribution: quietone commented
Status: Active » Needs review
snehi’s picture

snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/node/src/Tests/Migrate/d7/MigrateNodeTest.php
@@ -134,6 +135,10 @@ public function testNode() {
+    $node = Node::load(2);

    i don't think this is the correct way to do it. 

  2. +++ b/core/modules/node/src/Tests/Migrate/d7/MigrateNodeTest.php
@@ -134,6 +135,10 @@ public function testNode() {
+    $expected = "...is that it's the absolute best show ever. Trust me, I would know.";

    more than 80 col

quietone’s picture

quietone CreditAttribution: quietone commented
FileSize
interdiff-2635242-2-5.txt655 bytes
2635242-5.patch894 bytes

@snehi, thx for the review.

1. What do you suggest? $node = Node::load(N) is used elsewhere in the file. I've just used what is already there.

2. Yes, it is long. And, in fact, I've just changed it to be consistent with other migration tests and in doing so it is even longer. See MigrateUserMailTest for an example.

quietone’s picture

quietone CreditAttribution: quietone commented
Status: Needs work » Needs review
chx’s picture

chx CreditAttribution: chx commented
Issue tags: +SprintWeekend2016
alvar0hurtad0’s picture

alvar0hurtad0
he/him
Primary language Spanish
Location Cáceres
CreditAttribution: alvar0hurtad0 at ASPgems commented
Status: Needs review » Needs work

1. What do you suggest? $node = Node::load(N) is used elsewhere in the file. I've just used what is already there.

I think the problem is load the node directly by NID, IMHO we should get the id before and use a variable.

alvar0hurtad0’s picture

alvar0hurtad0
he/him
Primary language Spanish
Location Cáceres
CreditAttribution: alvar0hurtad0 at ASPgems commented
Assigned: Unassigned » alvar0hurtad0

Working on this.

The last submitted patch, 2: 2635242-2.patch, failed testing.

alvar0hurtad0’s picture

alvar0hurtad0
he/him
Primary language Spanish
Location Cáceres
CreditAttribution: alvar0hurtad0 at ASPgems commented
Issue tags: +Needs reroll

The patch don't apply.

Doing the reroll

alvar0hurtad0’s picture

alvar0hurtad0
he/him
Primary language Spanish
Location Cáceres
CreditAttribution: alvar0hurtad0 at ASPgems commented
Assigned: alvar0hurtad0 » Unassigned
Status: Needs work » Needs review
FileSize
add_tests_for_node_body-2635242-12.patch875 bytes

This is the reroll.

The last submitted patch, 5: 2635242-5.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 12: add_tests_for_node_body-2635242-12.patch, failed testing.

David Hernández’s picture

David Hernández CreditAttribution: David Hernández as a volunteer commented
Status: Needs work » Reviewed & tested by the community

This seems ok to me.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!

  • catch committed 376180d on 8.1.x 
    Issue #2635242 by quietone, alvar0hurtad0: Add tests for node body [d6...

  • catch committed 98410fb on 
    Issue #2635242 by quietone, alvar0hurtad0: Add tests for node body [d6...

Status: Fixed » Closed (fixed)

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