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.
Comments
Comment #1
larowlanGood 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?
Comment #2
Wim LeersThe 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:
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.
Comment #3
catchBumping to critical/beta blocker.
Comment #4
larowlanYes yes yes, this will fix #2114887: Maximum nesting level when attaching comment field to User. too.
Awesome idea.
Comment #5
andypostNice idea, but requiring to collect display in controller is bad idea and DX--
Comment #6
xjmPer @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.
Comment #7
catchYes there's a workaround for the other issue now, so this could be major I think.
Comment #8
yched CreditAttribution: yched commented#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)
Comment #9
Wim LeersComment #10
Wim LeersOops.
Comment #11
yched CreditAttribution: yched commentedNot 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...
Comment #12
Wim LeersThis is now 8.1 material at best.