Follow-up to #2422353: Comment module should check that comment body field exists

Problem/Motivation

CommentAdminOverview uses deprecated drupal_render() in controller code but should return alterable render array. This causes breakage of caching.

Proposed resolution

Return a render array instead of rendering comment author.

Remaining tasks

patch/commit

User interface changes

no

API changes

no

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because uses rendering in controller code
Issue priority Normal because it's leftover code from 7.x
Disruption Non-disruptive for core/contributed and custom modules/themes
CommentFileSizeAuthor
comment-admin-user.patch1.29 KBandypost
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Nice find

Status: Reviewed & tested by the community » Needs work

The last submitted patch, comment-admin-user.patch, failed testing.

Status: Needs work » Needs review

andypost queued comment-admin-user.patch for re-testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost at Skilld commented
Status: Needs review » Reviewed & tested by the community

was a bot lux

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: +D8 cacheability, +Quickfix
Related issues: +#2450993: Rendered Cache Metadata created during the main controller request gets lost

Hurray, this helps make #2450993: Rendered Cache Metadata created during the main controller request gets lost a tad easier/smaller :)

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer commented

RTBC + 1

Status: Reviewed & tested by the community » Needs work

The last submitted patch, comment-admin-user.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Reviewed & tested by the community

Suddenly two fails? Weird. Both in browser tests. Seems like unrelated failures, I don't see how this patch could cause that. Back to RTBC, and re-testing.

Wim Leers queued comment-admin-user.patch for re-testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Nice find indeed - we probably should have a meta trying to remove as many calls (or document them) to drupal_render.*() as possible.

Committed bd13994 and pushed to 8.0.x. Thanks!

  • alexpott committed bd13994 on 8.0.x 
    Issue #2505835 by andypost: Optimize CommentAdminOverview
martin107’s picture

martin107 CreditAttribution: martin107 commented

Created a meta issue.

#2509534: [Meta] document or refactor calls to drupal_render()

Status: Fixed » Closed (fixed)

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