Problem/Motivation

Steps to reproduce:

  1. Install standard profile
  2. Create article content
  3. Add comment
  4. Edit comment type (admin/structure/comment/manage/comment) and change the comment target entity type to something other than content.
  5. Visit article node again ... kaboom!
Fatal error: Call to a member function getSetting() on a non-object in /Volumes/devdisk/dev/sites/drupal8alt.dev/core/modules/comment/src/CommentViewBuilder.php on line 293

Proposed resolution

Prevent changing once field using comment type has been created?

Remaining tasks

Write patch.

User interface changes

Disable field if this can not be changed?

API changes

Not sure.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Yep should be removed if entity isn't new, patch coming

larowlan’s picture

Hoping for Red/Green

The last submitted patch, 2: comment-type-entity-type-change-snafoo-2303521.fail_.patch, failed testing.

Berdir’s picture

  1. +++ b/core/modules/comment/src/CommentTypeForm.php
    @@ -87,18 +87,31 @@ public function form(array $form, array &$form_state) {
    +      $form['target_entity_type_id'] = array(
    +        '#type' => 'value',
    +        '#value' => $comment_type->getTargetEntityTypeId(),
    +      );
    

    Is this really necessary?

    The entity building should be intelligent enough to only update existing values, but I only know for sure that's true for content entities.

  2. +++ b/core/modules/comment/src/Tests/CommentTypeTest.php
    @@ -76,6 +76,11 @@ public function testCommentTypeCreation() {
    +    // Edit the comment-type and attempt to change the entity-type.
    +    $this->drupalGet('admin/structure/comment/manage/foo');
    +    // The entity-type should not be editable on an existing comment-type.
    +    $this->assertNoField('target_entity_type_id', 'Entity type file not present');
    

    The first comment seems a bit strange, we're not really attempting to change it, as there's no way to do it (with the fix).

    I think it should just say that we verify that it can not be changed and is a read-only information? The second comment is enough IMHO for both lines of code. Possibly add an assertion for the label?

larowlan’s picture

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, no need to hold this up further.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Do we not need to tell the user that this can only be set when creating the comment type?

swentel’s picture

Status: Needs work » Needs review
FileSize
2.95 KB
617 bytes

Something like this ?

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Thanks

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

  • webchick committed 2d3c7f1 on 8.0.x
    Issue #2303521 by swentel, larowlan | alexpott: Fixed CommentTypeForm...

Status: Fixed » Closed (fixed)

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