Problem/Motivation

In case someone has the idea to override CommentViewBuilder, it is basically just impossible without overring everything, as CommentViewBuilder and self
hardcodes the used classed. Both references should be replaced by static, so you can easily have a CustomCommentViewBuilder extends CommentCommentViewBuilder.

Related: https://www.drupal.org/node/2342683 (it is the same case)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

estoyausente’s picture

larowlan’s picture

Status: Active » Needs review
larowlan’s picture

Two of these will fail, can't use static in string callback

tim.plunkett’s picture

Issue tags: -DX +DX (Developer Experience)
FileSize
1.81 KB

It's a little shortsighted of #post_render_cache to key by the $callback, since normally we'd use an array for this. Trying concatenating instead.

Status: Needs review » Needs work

The last submitted patch, 4: comment-2348547-4.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.7 KB

Ahh, drupal_render_cache_generate_placeholder does not support callables either, only strings.

larowlan’s picture

  1. +++ b/core/modules/comment/src/CommentViewBuilder.php
    @@ -197,7 +197,7 @@ public function buildComponents(array &$build, array $entities, array $displays,
    +    $callback = get_class() . '::renderLinks';
    

    Lets just move this to the service (CommentPostRenderCache) where we can have real DI an no \Drupal and then it can be swapped/subclassed easily

  2. +++ b/core/modules/comment/src/CommentViewBuilder.php
    @@ -209,8 +209,7 @@ public static function renderLinks(array $element, array $context) {
    +      $links['comment'] = static::buildLinks($entity, $commented_entity);
    

    +1

estoyausente’s picture

Hi, thanks for the patch correction. It's enough for me now :-)

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

8.1 can be followup

larowlan’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed bc20bd5 and pushed to 8.0.x. Thanks!

  • alexpott committed bc20bd5 on 8.0.x
    Issue #2348547 by tim.plunkett, estoyausente: Fixed CommentViewBuilder...
tstoeckler’s picture

Don't we really want get_called_class() here instead of get_class()?

tim.plunkett’s picture

Status: Fixed » Needs work

Um, yeah we do. Either get_called_class() or get_class($this)

dawehner’s picture

larowlan’s picture

Status: Needs work » Needs review
FileSize
1.13 KB

/me didn't even know of that function - thanks

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Cool

larowlan’s picture

trivial patch of the month?

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 6d5e445 and pushed to 8.0.x. Thanks!

  • alexpott committed 6d5e445 on 8.0.x
    Issue #2348547 followup by larowlan: Fixed CommentViewBuilder should use...

Status: Fixed » Closed (fixed)

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