Specify view mode to be used by comment formatter

Thanks, definitely an omission

Patch.

Small fix.

Ok, it's not straight.

This is looking great

1. +++ b/core/modules/comment/comment.install
   @@ -127,3 +127,30 @@ function comment_update_8001() {
   +      if (isset($component['type']) && ($component['type'] == 'comment_default')) {
   

   nit ===

2. +++ b/core/modules/comment/config/schema/comment.schema.yml
   @@ -4,6 +4,9 @@ field.formatter.settings.comment_default:
   +      label: 'The comment entity view mode to be used in this formatter '
   

   Trailing space

3. +++ b/core/modules/comment/src/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
   @@ -229,4 +238,22 @@ public function settingsSummary() {
   +        if ($display = EntityViewDisplay::load("comment.$bundle.$mode")) {
   

   nice

4. +++ b/core/modules/comment/src/Tests/CommentInterfaceTest.php
   @@ -291,4 +294,54 @@ public function testAutoFilledHtmlSubject() {
   +    $this->assertNoRaw('<p>' . $comment_text . '</p>');
   

   nice

5. +++ b/core/modules/comment/src/Tests/Update/CommentUpdateTest.php
   @@ -0,0 +1,47 @@
   +    $config = $this->config('core.entity_view_display.node.article.default');
   +    $this->assertNull($config->get('content.comment.settings.view_mode'));
   +
   +    // Run updates.
   +    $this->runUpdates();
   +
   

   Awesome

6. +++ b/core/profiles/standard/config/install/core.entity_view_display.node.article.default.yml
   @@ -23,13 +23,6 @@ content:
   -  comment:
   -    label: above
   -    type: comment_default
   -    weight: 20
   -    settings:
   -      pager_id: 0
   -    third_party_settings: {  }
   

   was this duplicated?

#12.1, 2: Fixed.
#12.6: Yes, it was duplicated.

Now I added one more KernelTest because I had to fix the case when a commend display, used as setting in field formatter, gets disabled. That case is not covered by calculateDependencies(). We need to act in the same way as we do when we delete de dependent entity —­ disable the formatter and log a message.

@jonathanjfshaw, thank you for review. Can you try this new patch?

OK. Fixed the crash (hopefully).

@jonathanjfshaw, if this is green, it would be nice to have another review.

Checking patch core/modules/comment/src/Tests/Update/CommentUpdateTest.php...
error: core/modules/comment/src/Tests/Update/CommentUpdateTest.php: already exists in working directory
Checking patch core/modules/comment/tests/src/Kernel/CommentIntegrationTest.php...
error: core/modules/comment/tests/src/Kernel/CommentIntegrationTest.php: already exists in working directory

Can you try pulling code once and create patch again.

The patch is OK. See that it applies in test. You should use the 8.1.x branch and apply the patch on that. But wait till we'll have a green patch, I still need to fix the code.

Let's see.

Ignore patch from #24. The patch was malformed. Interdiff is against #18.

Passing to @larowlan for a review.

+++ b/core/modules/comment/comment.module
@@ -756,3 +757,43 @@ function comment_preprocess_field(&$variables) {
+/**
+ * Implements hook_ENTITY_TYPE_presave() for entity_view_display entities.
+ */
+function comment_entity_view_display_presave(EntityViewDisplayInterface $display) {
...
+  // Disable the comment field formatter when the used view display is disabled.
...
+}

This can be rewritten in a more nicer way if #2647576: onDependencyUpdating() - React on dependency updating would be committed.

This looks really good, a couple of nits and one minor cleanup as well as some random musings that you can ignore unless you feel strongly about it.

1. +++ b/core/modules/comment/comment.install
   @@ -127,3 +127,30 @@ function comment_update_8001() {
   + * Adds the new 'view_mode' setting to all view displays using the
   

   Should we have an @addtogroup updates-8.1-0 or similar here?

2. +++ b/core/modules/comment/comment.install
   @@ -127,3 +127,30 @@ function comment_update_8001() {
   +      $display->save(TRUE);
   

   Should we collate the updated displays and show a message to the user with regards to the changes made?

3. +++ b/core/modules/comment/comment.module
   @@ -756,3 +757,43 @@ function comment_preprocess_field(&$variables) {
   +function comment_entity_view_display_presave(EntityViewDisplayInterface $display) {
   

   Random observation: we could refactor this to a domain object that could be unit tested?

   $comment_view_update_reaction = new CommentViewModeUpdateReaction(\Drupal::entityTypeManager()->getStorage('entity_view_display'), \Drupal::entityTypeManager()->getStorage('entity_view_mode'), \Drupal::logger('system'));
   $comment_view_update_reaction->react($display);
   

4. +++ b/core/modules/comment/comment.module
   @@ -756,3 +757,43 @@ function comment_preprocess_field(&$variables) {
   +  if ($display->isNew() || $display->getTargetEntityTypeId() != 'comment' || $display->status()) {
   ...
   +      if (isset($component['type']) && ($component['type'] == 'comment_default')) {
   +        if ($component['settings']['view_mode'] == $display->getMode()) {
   

   nit: === and !==

5. +++ b/core/modules/comment/comment.module
   @@ -756,3 +757,43 @@ function comment_preprocess_field(&$variables) {
   +          \Drupal::logger('system')->warning("View display '@id': Comment field formatter '@name' was disabled because is using the comment view display '@display' (@mode) that was just disabled.", $arguments);
   

   nit: 'is is using' instead of 'is using'

6. +++ b/core/modules/comment/src/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
   @@ -206,6 +208,23 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
   +    if (count($view_modes) > 1) {
   +      $element['view_mode'] = [
   +        '#type' => 'select',
   +        '#title' => $this->t('Comments view mode'),
   +        '#description' => $this->t('Select the view mode used to show the list of comments.'),
   +        '#default_value' => $this->getSetting('view_mode'),
   +        '#options' => $view_modes,
   +      ];
   +    }
   +    else {
   +      $element['view_mode'] = [
   +        '#type' => 'value',
   +        '#value' => 'default',
   +      ];
   +    }
   

   this hunk could be simplified to

   $element['view_mode'] = [
     '#type' => 'select',
     '#title' => $this->t('Comments view mode'),
     '#description' => $this->t('Select the view mode used to show the list of comments.'),
     '#default_value' => $this->getSetting('view_mode'),
     '#options' => $view_modes,
     '#access' => count($view_modes) > 1,
   ];
   

7. +++ b/core/modules/comment/src/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
   @@ -220,13 +239,41 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
   +          $dependencies[$display->getConfigDependencyKey()][] = $display->getConfigDependencyName();
   

   Sometimes I see code that calls ::addDependency and then return the parent result for ::calculateDependencies? Is there a standard for that?

8. +++ b/core/modules/comment/src/Tests/CommentTestTrait.php
   @@ -111,6 +114,7 @@ public function addDefaultCommentField($entity_type, $bundle, $field_name = 'com
   +          'settings' => array('view_mode' => $comment_view_mode),
   

   nit: I think new code is supposed to use [] instead of array()

9. +++ b/core/modules/comment/tests/src/Kernel/CommentIntegrationTest.php
   @@ -0,0 +1,142 @@
   +  public function testViewMode() {
   

   Great work

Thank you for review, @larowlan.

#30.1: That's not very clear, I guess. I asked a question here #2624318-5: [meta] 8.1.x upgrade path. For now I wrapped the function in addtogroup updates-8.1.0 and we'll see what the core committer will say after RTBC.

#30.2: OK.

#30.3: We are fully testing comment_entity_view_display_presave() in the new Kernel test \Drupal\Tests\comment\Kernel\CommentIntegrationTest. Also, as I mention in #29, I think it's better to convert it into an onDependencyUpdating(), when (and if) #2647576: onDependencyUpdating() - React on dependency updating lands. BTW, that needs a good review too but I feel that it's must in order to drop a lot of procedural code.

#30.4, 5: Done.

#30.6: Nice! Done.

#30.7:

Sometimes I see code that calls ::addDependency and then return the parent result for ::calculateDependencies? Is there a standard for that?

Yes, there's a difference between how calculateDependencies() works on config entities vs. plugins that are items in plugin collections attached to config entities. In this case this is a formatter plugin that is member of a collection part of the entity display (a formatter in entity display components plugin collection). There's no addDependency() method in plugins and the plugin calculateDependencies() (unlike the same method of config entities) should return the dependencies. Then the enclosing config entity will collect those dependencies from all plugin collections and add them to its own dependency list.

#30.8: Yes, but the enclosing statement entity_get_display(...)->setComponent($field_name, array(...)) still uses the old syntax and our code standards documents states (https://www.drupal.org/coding-standards#array) about the short syntax:

When used, try to apply it consistently to at least a whole method or function.

So I didn't wanted to increase the footstep of the patch by converting the whole method.

#30.9: Thank you!

Ouch!

Assuming bot agrees
Thanks - great work

1. +++ b/core/modules/comment/comment.install
   @@ -127,3 +129,47 @@ function comment_update_8001() {
   + * Adds the new 'view_mode' setting to all view displays using the
   + * 'comment_default' formatter.
   

   So this gets displayed to the user. It must be a single line and should start with "Add" to "Adds" (ie. imperative not indicative).

2. +++ b/core/modules/comment/src/Tests/CommentInterfaceTest.php
   @@ -291,4 +294,54 @@ public function testAutoFilledHtmlSubject() {
   +   * Tests the comment field formatter used with non-default comment entity view
   +   * mode.
   

   Should be a single line.

3. +++ b/core/modules/comment/tests/src/Kernel/CommentIntegrationTest.php
   @@ -0,0 +1,142 @@
   +      'mode' => 'default'
   

   Need a comma after 'default'

4. +++ b/core/modules/comment/tests/src/Kernel/CommentIntegrationTest.php
   @@ -0,0 +1,142 @@
   +    $logged = (bool) Database::getConnection()->select('watchdog')
   +      ->fields('watchdog', ['wid'])
   +      ->condition('type', 'system')
   +      ->condition('message', "View display '@id': Comment field formatter '@name' was disabled because is using the comment view display '@display' (@mode) that was just disabled.")
   +      ->condition('variables', serialize($arguments))
   +      ->execute()
   +      ->fetchField();
   

   $logged is not used - I guess we just need an assert.

5. +++ b/core/modules/comment/src/Tests/Update/CommentUpdateTest.php
   @@ -0,0 +1,47 @@
   +  protected function setDatabaseDumpFiles() {
   +    $this->databaseDumpFiles = [
   +      __DIR__ . '/../../../../system/tests/fixtures/update/drupal-8-rc1.bare.standard.php.gz',
   +    ];
   +  }
   

   Can we use the full update db so we can test updating multiple config entities... forum should be enabled there.

@alexpott, Implemented all from #35. @larowlan you can RTBC if you feel that it's OK.

Great review @alexpott, thanks again @claudiu.cristea

If we do this, can we later do #1938096: Convert the node comments list to a view, or does this mean we're not planning on doing that?

I think comments render is complex enough to live in formatter, so there's no need to do that with views.
I closed #1938096: Convert the node comments list to a view

1. +++ b/core/modules/comment/comment.install
   @@ -127,3 +129,46 @@ function comment_update_8001() {
   + * @addtogroup updates-8.1.0
   

   Should be 8.1.x

2. +++ b/core/modules/comment/comment.install
   @@ -127,3 +129,46 @@ function comment_update_8001() {
   +function comment_update_8101() {
   

   Not sure it matters but while 8000 was reserved for detecting 7.x databases, we don't need to do that with 8100.

3. +++ b/core/modules/comment/comment.module
   @@ -756,3 +757,43 @@ function comment_preprocess_field(&$variables) {
   +          \Drupal::logger('system')->warning("View display '@id': Comment field formatter '@name' was disabled because it is using the comment view display '@display' (@mode) that was just disabled.", $arguments);
   

   Why disable and not switch back to default?

   Or should we validate disabling the view displays instead when they're in use? This is the only bit of the patch that didn't sit right with me, might just need some more comments on why we're doing it this way, but feels a bit fragile. I realise #2647576: onDependencyUpdating() - React on dependency updating would make this visually a bit nicer.

4. +++ b/core/modules/comment/src/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
   @@ -206,6 +208,16 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
   +      '#description' => $this->t('Select the view mode used to show the list of comments.'),
   

   Are we using view mode or display in UI text?

Rerolled for 8.2.x

@catch,

#4.1: Fixed.
#40.2: OK. Starting from 8200.
#40.3: This was a long discussion on removing image styles. Should the image formatter use the original image or the entire field should be disabled. This is somehow the same. My point: we cannot assume that the site builder wants to use that view display. When he's doing such operations (deleting/disabling a display) he must assume what he's doing. We cannot substitute him. The only thing we can make is to disable and notify him. So, I'm for disabling.
#40.4: 'Mode", no? I cannot decide that but mode sounds OK for me.

thanks @claudiu.cristea

Read through the entire patch again and tested it out - this looks like a really nice feature addition for 8.2.x that makes the system work the way you think it should.

Given this is a change in behaviour we need a change record - once we have that we're good for a commit. There are both developer changes and site builder changes. Also this is not an API change because a sensible default is used which results in the existing behaviour.

@alexpott, Added the draft CR at https://www.drupal.org/node/2769201. Back to RTBC as per #45.

Committed 7f8e1c9 and pushed to 8.2.x. Thanks!

@alexpott, Seems it wasn't committed. At least I cannot see it in Git log

Great and useful improvement, thanx!