Problem/Motivation

We're concerned here with the route comment.reply and the method \Drupal\comment\Controller\CommentController::getReplyForm().

Currently, comment module does some gymanstics to get a fully-rendered node and just a single comment on a page.

// The comment is in response to a entity.
elseif ($entity->access('view', $account)) {
  // We make sure the field value isn't set so we don't end up with a
  // redirect loop.
  $entity->{$field_name}->status = CommentItemInterface::HIDDEN;
  // Render array of the entity full view mode.
  $build['commented_entity'] = $this->entityManager()->getViewBuilder($entity->getEntityTypeId())->view($entity, 'full');
  unset($build['commented_entity']['#cache']);
  $entity->{$field_name}->status = $status;
}

Note the hiding of the comment field, rendering the node, then reseting of the comment field status. This is not ideal, especially because we're actively unsetting the cache property, so this page becomes uncached.

Proposed resolution

What we might try instead is for the Comment module to provide a node view mode that is a combo of the fully-rendered entity and the comment form. Or, in the case of a reply to a comment, the comment being replied to and a comment form -- this last case might require a view mode for comment as well.

We could then cache the entity/comment reply pages given the unique view mode.

User interface changes

None.

API changes

None.

Original report by @jessebeach

Comments

larowlan’s picture

Good idea, but it's the same issue as the compact user view mode. Comment doesn't know node exists anymore so we're going to need to make a view mode for every entity type. But where can we do this from?

Wim Leers’s picture

Title: Comment module should provide a view mode for its individual comment reply page » EntityViewBuilderInterface::view() should accept an EntityViewDisplay, not only a view mode ID
Component: comment.module » entity system
Assigned: Unassigned » Wim Leers
Issue tags: +Entity Field API, +Performance, +D8 cacheability

The code cited in the issue summary does some horrible, horrible things to render an entity minus one field. We should be able to load an EntityViewDisplay, remove a component, pass that display to the entity's view builder and that have that be used to render the entity.

IOW, we should be able to do this:

$display = EntityViewDisplay::collectRenderDisplay($entity, 'full');
$display->removeComponent($field_name);
$this->entityManager()->getViewBuilder($entity->getEntityTypeId())->view($entity, $display);

So, repurposing this issue to fix that; the cited code would be the first use case.

This blocks #2099131: Use #pre_render pattern for entity render caching.

catch’s picture

Priority: Normal » Critical
Issue tags: +beta blocker

Bumping to critical/beta blocker.

larowlan’s picture

Yes yes yes, this will fix #2114887: Maximum nesting level when attaching comment field to User. too.
Awesome idea.

andypost’s picture

Nice idea, but requiring to collect display in controller is bad idea and DX--

xjm’s picture

Assigned: Wim Leers » Unassigned

Per @Berdir and @Wim Leers, this was originally bumped to beta-blocking as a blocker for #2099131: Use #pre_render pattern for entity render caching, but that issue took a different route. In that case, does this still need to be beta-blocking? It sounds like this is still an important refactor, but it's not clear how much public API breakage there would be, so potentially work on this could continue during the beta phase.

catch’s picture

Priority: Critical » Major
Issue tags: -beta blocker

Yes there's a workaround for the other issue now, so this could be major I think.

yched’s picture

#2 looks like the right approach, but I fully agree with @andypost #5, we cant require the callers of EVB::view() to fetch the EntityDisplay themselves.

Add a separate method then ?
Or allow EVB::view to accept either a view mode string or a Display ? (we usually frown at polymorphism though)

Wim Leers’s picture

Wim Leers’s picture

yched’s picture

Not easy given the current flow :

1) EVB::view() calls EVB::viewMultiple(), so adding the feature to view() means adding it to viewMultiple(). But adding that feature for multiple entities (possibly of different bundles) makes this connected to #2466913: Allow EVD to render fields across bundles, since the EVD will be used to display several bundles.
Unless viewMultiple() receives an array of separate EVDs keyed by bundle, and leaves it to the caller to pass the right EVDs for the right bundles ?

2) Then EVB::viewMultiple() doesn't actually do the job, but simply sets a #pre_render callback for late rendering only on cache miss.

3) getBuildDefaults() makes sure the $element has all the #parameters the callback will need for the actual rendering - the #view_mode atm, but in our case here it would need to be the injected #EVD.

4) In getBuildDefaults() as well, we use the $view_mode to determine whether the render array is cacheable or not. Not sure what that would mean if we need to render an injected EVD instead of a view mode.

5) hook_entity_build_defaults() runs after that, so it will need to cater for "sometimes there will be a #view_mode, sometimes an #EVD" ?

6) Then, buildMultiple(), that does the actual rendering on cache miss, is currently really structured on "by view mode". But that part at least seems refactorable for "I have an EVD" without too many roadblocks.

So, not really trivial :-) I'm a bit scared that this will lead to two parallel code paths in each of the methods above, which won't really make the code easier to follow...

Wim Leers’s picture

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

This is now 8.1 material at best.

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.

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

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

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.