Problem/Motivation

Comment field settings are nested in a 'comments' details element.
The field-schema does not reflect this, so the values/settings aren't saved

Proposed resolution

Remove the 'comments' wrapper nesting from the form field names

Remaining tasks

Review

User interface changes

None

API changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

The last submitted patch, comment-settings-busted.fail_.patch, failed testing.

yched’s picture

Agreed on the fix, a bit sad that there isn't a simpler way ?

dawehner’s picture

+++ b/core/modules/comment/src/Plugin/Field/FieldType/CommentItem.php
@@ -210,4 +212,17 @@ public function storageSettingsForm(array &$form, FormStateInterface $form_state
 
+  /**
+   * Form builder process callback.
+   */
+  public static function processCommentSettings(&$element) {
+    $parents = $element['#parents'];
+    // Remove the 'comment' nesting.
+    array_pop($parents);
+    foreach (Element::children($element) as $key) {
+      $element[$key]['#parents'] = array_merge($parents, array($key));
+    }
+    return $element;
+  }
+

it sounds like a generic functionality for me. Can't we provide some kind of process callback in FAPI somewhere? Note: I have seen a similar thing in SystemMenuBlock

yched’s picture

So the case is "I don't know which form structure I'm embedded into, but I want my values sub-array to be structured that way".

Our current tools (#tree, #parents) do not allow this, but ideally, such things could be expressed directly in the $element sub form without having to resort to process callbacks ?

larowlan’s picture

So perhaps we allow #tree => FALSE on details elements to auto-implement this functionality? Even when #tree is already TRUE.

But to be honest, this sounds like a follow up, I'd rather we didn't block fixing this issue on adding a new API.

Thoughts?

larowlan’s picture

Also, I'm completely ok with removing the details element if that helps with velocity.
I think it was a carry over from the old details tab on the node-type form

larowlan’s picture

So just removed the details element in the interest of velocity.
Doesn't make much difference - no other fields have a details wrapper on their settings.
Screenshot

Status: Needs review » Needs work

The last submitted patch, 7: comment-settings-busted.7.patch, failed testing.

yched’s picture

Status: Needs work » Reviewed & tested by the community

Looks good to me

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/comment/src/Plugin/Field/FieldType/CommentItem.php
@@ -106,21 +107,13 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state) {
-      '#attached' => array(
-        'library' => array('comment/drupal.comment'),
-      ),

Is this not used?

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Ya that was used to update the vertical tab summary when these fields were on the node-type edit form.
So, nope - not needed.
The library is still used to update a vertical tab summary on the node edit form, so we need it, just not here

grendzy’s picture

Status: Reviewed & tested by the community » Needs review

I think I found one other place which looks up this variable using the wrong key:

http://cgit.drupalcode.org/drupal/tree/core/modules/comment/src/CommentA...

$anonymous_contact = $commented_entity->get($entity->getFieldName())->getFieldDefinition()->getSetting('anonymous_contact');
larowlan’s picture

Status: Needs review » Reviewed & tested by the community
grendzy’s picture

Thanks, good work!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs usability review

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 125424b and pushed to 8.0.x. Thanks!

  • alexpott committed 125424b on 8.0.x
    Issue #2405675 by larowlan: Comment field settings don't save
    

Status: Fixed » Closed (fixed)

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