Problem/Motivation

In Drupal 8.0.x the UI allows us to create a new view mode for a comment type, but there is no way to use that view mode without writing a custom formatter. Ideally, there should be a setting on the built-in comments formatter that allows us to specify a particular view mode should be used for the comments on a particular content type.

Proposed resolution

  • Add a new Comment field formatter setting view_mode, defaulting to default.
  • Add an update path to set this setting on existing formatters.
  • Test the update path test.
  • Allow the new setting to be configured from the view display form.
  • Calculate dependencies for the new setting and add appropriate reactions on dependency removal or disable.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

jonathanjfshaw created an issue. See original summary.

jonathanshaw’s picture

Issue summary: View changes
larowlan’s picture

Thanks, definitely an omission

claudiu.cristea’s picture

Status: Active » Needs review
FileSize
2.34 KB

Patch.

Status: Needs review » Needs work

The last submitted patch, 4: 2627678-4.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
2.02 KB
713 bytes

Small fix.

Status: Needs review » Needs work

The last submitted patch, 6: 2627678-6.patch, failed testing.

claudiu.cristea’s picture

claudiu.cristea’s picture

claudiu.cristea’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -Needs upgrade path, -Needs upgrade path tests
FileSize
9.94 KB
10.42 KB

Ok, it's not straight.

Status: Needs review » Needs work

The last submitted patch, 10: 2627678-10.patch, failed testing.

larowlan’s picture

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?

claudiu.cristea’s picture

FileSize
15.68 KB
20.06 KB

#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.

claudiu.cristea’s picture

Status: Needs work » Needs review
jonathanshaw’s picture

Patch works great on simplytest.me. 1 minor issue:

If I add a new comment display mode called "Newmode" then I have 3 display modes: Default, Full comment and Newmode. At admin/structure/comment/manage/comment/display I can assign different display properties for Default, Full comment and Newmode. When got to admin/structure/types/manage/article/display and set the new comment formatter setting in this patch to "Newmode", it works and I get the Newmode comment display mode shown on Articles. But whether I select "Default" or "Full comment" in the formatter setting, I get the "Full comment" display mode either way. Perhaps something is weird about "Default" and it should be excluded from the options presented by this formatter setting?

claudiu.cristea’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update
FileSize
19.78 KB
4.25 KB

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

Status: Needs review » Needs work

The last submitted patch, 16: 2627678-16.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
21.23 KB
1.46 KB

OK. Fixed the crash (hopefully).

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

Status: Needs review » Needs work

The last submitted patch, 18: 2627678-18.patch, failed testing.

ashhishhh’s picture

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.

ashhishhh’s picture

Status: Needs work » Needs review
ashhishhh’s picture

Status: Needs review » Needs work
claudiu.cristea’s picture

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.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
21.66 KB
1.5 KB

Let's see.

Status: Needs review » Needs work

The last submitted patch, 24: 2627678-24.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
21.84 KB
1.5 KB

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

jonathanshaw’s picture

Great! Sorry for delay in replying. Latest version of the patch fixes #15. I'm not qualified to review the revised code, so leaving status at Needs review.

claudiu.cristea’s picture

Assigned: Unassigned » larowlan

Passing to @larowlan for a review.

claudiu.cristea’s picture

+++ 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.

larowlan’s picture

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

claudiu.cristea’s picture

FileSize
22.4 KB
5.51 KB

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!

Status: Needs review » Needs work

The last submitted patch, 31: 2627678-31.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
22.4 KB
747 bytes

Ouch!

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Assuming bot agrees
Thanks - great work

alexpott’s picture

Status: Reviewed & tested by the community » Needs 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.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
23.03 KB
4.45 KB

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

larowlan’s picture

Assigned: larowlan » Unassigned
Status: Needs review » Reviewed & tested by the community

Great review @alexpott, thanks again @claudiu.cristea

catch’s picture

Status: Reviewed & tested by the community » Needs review

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?

andypost’s picture

Status: Needs review » Reviewed & tested by the community

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

catch’s picture

Status: Reviewed & tested by the community » Needs review
  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?

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

claudiu.cristea’s picture

FileSize
22.93 KB
2.07 KB

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.

claudiu.cristea’s picture

Issue tags: +DevDaysMilan
jonathanshaw’s picture

Regarding #40.4, entity reference uses "view mode" in the settings ui of its rendered entity formatter.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

thanks @claudiu.cristea

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +8.2.0 release notes, +Needs change record

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.

claudiu.cristea’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -DevDaysMilan, -Needs change record

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

claudiu.cristea’s picture

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

  • alexpott committed 7f8e1c9 on 8.2.x
    Issue #2627678 by claudiu.cristea, larowlan, jonathanjfshaw, alexpott,...
andypost’s picture

Great and useful improvement, thanx!

Status: Fixed » Closed (fixed)

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