Follow-up to #2887142-62: NodeType source plugin should include comment information

The method \Drupal\node\Plugin\migrate\source\d7\NodeType::fields() contains dead code for comment data

  public function fields() {
    return [
      'type' => $this->t('Machine name of the node type.'),
...
    ];
    if ($this->moduleExists('comment')) {
      $fields += $this->getCommentFields();
    }
    return $fields;
  }

It needs to extend tests because no broken tests reports

CommentFileSizeAuthor
#8 3007436-8-complete.patch1.82 KBjofitz
#8 3007436-8-test_only.patch1.22 KBjofitz
#2 3007436-2.patch622 bytesandypost
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost created an issue. See original summary.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Active » Needs review
FileSize
3007436-2.patch622 bytes

I bet it needs backport to 8.6

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Title: FIx comment » Fix d7 comment migration

proper title

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Related issues: +#2937844: Fix 'Squiz.PHP.NonExecutableCode' coding standard

Affects #2937844-7: Fix 'Squiz.PHP.NonExecutableCode' coding standard

Status: Needs review » Needs work

The last submitted patch, 2: 3007436-2.patch, failed testing. View results

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs work » Needs review

bot flux, probably \Drupal\Tests\comment\Kernel\Migrate\d7\MigrateCommentTypeTest could be extended to cover this method

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Acro Commerce for Acro Commerce commented
Status: Needs review » Needs work

Needs tests, so back to NW.

jofitz’s picture

jofitz CreditAttribution: jofitz at jofitz commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3007436-8-test_only.patch1.22 KB
3007436-8-complete.patch1.82 KB

Added test.

The last submitted patch, 8: 3007436-8-test_only.patch, failed testing. View results

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer commented
Assigned: Unassigned » mikelutz

The last submitted patch, 8: 3007436-8-test_only.patch, failed testing. View results

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Discussed in maintainers meeting with @heddn and @quietone. While we would not generally test the return value of the fields() method on a source plugin, because it's dynamic in this case, it's appropriate to test that the comment fields appear when they are supposed to. The fix is obviously correct, and should be backported into 8.6 as a bugfix.

RTBC on the condition that it passes tests against 8.6, which it should.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer commented
Assigned: mikelutz » Unassigned
catch’s picture

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

Committed and pushed da44172f5c to 8.7.x and 7836638b08 to 8.6.x. Thanks!

  • catch committed da44172 on 8.7.x 
    Issue #3007436 by Jo Fitzgerald, andypost, mikelutz, heddn: Fix d7...

  • catch committed 7836638 on 8.6.x 
    Issue #3007436 by Jo Fitzgerald, andypost, mikelutz, heddn: Fix d7...

Status: Fixed » Closed (fixed)

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