Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Following from #1938930: Convert theme_system_modules_details() to #type table and #2010982-8: Replace theme() with drupal_render() in system module for system_status()
Problem/Motivation
Currently theme_status_report()
uses hard-coded table render
Now tables cell could get classes #1948374: #type 'table' allow attributes on table cells
Proposed resolution
Proposed way is to implement a template
Comment | File | Size | Author |
---|---|---|---|
#18 | remove-type-1938314-18.patch | 2.88 KB | pplantinga |
#17 | theme_status_report-remove-type-1938314-17.patch | 5.96 KB | pplantinga |
#12 | theme_status_report-1938314-12.patch | 6.05 KB | pplantinga |
#12 | theme_status_report-remove-type-1938314-12.patch | 5.95 KB | pplantinga |
#8 | theme_status_report-1938314-8.patch | 6.05 KB | pplantinga |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedTaking this one. My first patch so please needs review.
Comment #2
andypostNice patch! Please set to 'needs review' when when new patch added
useless netbeans file should be removed from patch
suppose, severities should be defined in preprocess
Comment #4
Anonymous (not verified) CreditAttribution: Anonymous commentedReroll the patch as per #2 suggestions.
Comment #6
andypostGreat, now needs to fix tests
please add a new line after end
Comment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #8
pplantinga CreditAttribution: pplantinga commentedHow about this?
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commented#5 looks good.
Comment #10
Eric_A CreditAttribution: Eric_A commentedMisspelled severities (twice)
type is the wrong property.
Comment #11
thedavidmeister CreditAttribution: thedavidmeister commentedAs part of this simplification can we try to remove checking the #type of requirements completely?
If it is checking for #type as a test for an array being a render element - this is a very flawed test as there's currently no requirement for render arrays to have a #type set and so a more robust test should be implemented.
If it is checking some kind of #type that has nothing to do with #type as defined for element_info(), this is super confusing and should be renamed or at the least documented way better.
If it is neither of these two things, can we please figure out what it is and remove it or document why it is required?
Comment #12
pplantinga CreditAttribution: pplantinga commentedI'm not sure fixing the #type check fits within the scope of this issue, but here's my opinion anyway. I think we should remove the check altogether and just make the calling function format $requirements appropriately.
Here's two patches, one that fixes the spelling issues, and one that fixes the spelling and removes the #type check.
Comment #13
pplantinga CreditAttribution: pplantinga commentedComment #14
thedavidmeister CreditAttribution: thedavidmeister commentedWhy would it be out of scope? This issue is specifically for simplifying the render process for theme_status_report().
Thanks for the patch :)
Comment #15
pplantinga CreditAttribution: pplantinga commentedFair enough. I somehow had it in my head that it was a straight theme() to twig conversion.
Comment #16
Eric_A CreditAttribution: Eric_A commentedThere are dedicated Twig conversion issues already (#1987410: [meta] system.module - Convert theme_ functions to Twig / #1757550: [Meta] Convert core theme functions to Twig templates), so probably better to focus on just the simplification here?
Comment #17
pplantinga CreditAttribution: pplantinga commentedI think this issue is much more specific than #1987410: [meta] system.module - Convert theme_ functions to Twig and so can override it.
Here's a re-rolled patch.
Comment #18
pplantinga CreditAttribution: pplantinga commentedActually, #16 is right. Here's a patch that just does the simplification.
Comment #19
joelpittet@pplantinga re: #18
If I read this patch right, the only significant change is the removal of the $requirements['#type'] check inside the loop?
Is this all that's needed for this issue to simplify? If so, it's RTBC:)
Comment #20
pplantinga CreditAttribution: pplantinga commentedSee #11 and #12. Removing #type check seems to be consensus.
Comment #21
alexpottYep this seems totally reasonable since https://api.drupal.org/api/drupal/core!modules!system!system.api.php/fun... makes no mention of a #type key.
Committed 0e100f3 and pushed to 8.x. Thanks!
git diff -w gives the following output