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.
Follow-up to: #891112: Replace theme_item_list()'s 'data' items with render elements
Problem
- The
'type'
theme variable of theme_item_list() clashes with#type
of render arrays.
Proposed solution
- Rename the variable to
'list_type'
.
This is blocking
- #2009008: Replace theme() with drupal_render() in field and field_ui modules
- #2005970: In renderable arrays, #type should provide a #theme suggestion, similar to how drupal_prepare_form() works
Comment | File | Size | Author |
---|---|---|---|
#41 | rename-type-variable-1828536-41.patch | 6.47 KB | pwieck |
#41 | rename-type-variable-1828536-35-41-interdiff.txt | 2.4 KB | pwieck |
#35 | rename-type-variable-1828536-35.patch | 6.45 KB | benjf |
#32 | 1828536-32.patch | 7.59 KB | benjf |
#25 | 1828536-interdiff-23-25.txt | 3.06 KB | thedavidmeister |
Comments
Comment #1
sunThis should cut it.
If this passes the bot, then this issue should be marked postponed on #891112: Replace theme_item_list()'s 'data' items with render elements, since that should go in first, since it's way harder to roll.
Comment #3
sunRe-rolled against HEAD.
Comment #4
sun#3: drupal8.theme-item-list-type-tag.3.patch queued for re-testing.
Comment #5
sunRe-rolled against HEAD.
Comment #7
sun#5: drupal8.theme-item-list-type-tag.5.patch queued for re-testing.
Comment #9
sunTagging... Please feel free to take this over, don't know when I'll get back to it.
Comment #10
hawkeye.twolfPatch re-rolled. Uploading to check for for greenlight.
Comment #11
jenlamptonI don't like the variable name 'tag' since there are actually a lot of HTML tags in the item_list Twig template. Tag really only makes sense if you are concatenating strings to generate HTML tags as we did in Drupal 7:
$output .= '<' . $tag . new Attribute($list_attributes) . '>';
.But we are removing this kind of behavior from Drupal 8, so I think it's important that we keep the names of variables closer to what they mean. In this case 'type' is indicating what type of list we were generating. I had changed the variable name to 'list_type' in the Twig template. I find that slightly more sensical than just 'tag'.
Here's an example of how the twig template would read using
tag
:The above just doesn't read nicely to me. If we used
list_type
instead it makes a little more sense when you look at it:Comment #12
jenlamptonUpdating issue title. Rerolled patch with alternate variable name.
Comment #14
jenlampton#10: 1828536-drupal8-theme-item-list-type-tag.10.patch queued for re-testing.
Comment #15
jenlamptonretesting previous patch to confirm that I broke everything :/
Comment #16
dcam CreditAttribution: dcam commentedI think I found the problem. One of the changes to "tag" wasn't changed to "list_type."
Comment #17
andymartha CreditAttribution: andymartha commentedAfter applying patch 1828536-16-list-type.patch in #16 from dcam, it appears that list items can be made/viewed with no errors using the alternate variable name. See screenshot.
Comment #18
Fabianx CreditAttribution: Fabianx commentedLooks very good to me, passes tests, had manual testing. => RTBC
Comment #19
tim.plunkettMissing changes to
core/modules/tour/lib/Drupal/tour/TourRenderController.php
core/modules/views/views.theme.inc
Comment #20
jibranRelated #1300744: Introduce #type 'links'
Comment #20.0
jibrannot tag
Comment #21
thedavidmeister CreditAttribution: thedavidmeister commentednot sure this should still be assigned to sun. I see a few others rolling patches here.
Comment #21.0
thedavidmeister CreditAttribution: thedavidmeister commentedadded blocking issue
Comment #22
thedavidmeister CreditAttribution: thedavidmeister commentedalso related #2005970: In renderable arrays, #type should provide a #theme suggestion, similar to how drupal_prepare_form() works.
Comment #23
thedavidmeister CreditAttribution: thedavidmeister commentedComment #24
star-szrI just double checked the changes, looks good. Will need a change notification, tagging.
Minor coding standards nitpick, otherwise RTBC from me:
These should all have a trailing comma per https://drupal.org/coding-standards#array.
Comment #25
thedavidmeister CreditAttribution: thedavidmeister commentedComment #27
star-szr#25: 1828536-25.patch queued for re-testing.
Comment #29
star-szr#25: 1828536-25.patch queued for re-testing.
Comment #30
star-szrLooks good to me!
Comment #31
alexpottNeeds a reroll
Comment #32
benjf CreditAttribution: benjf commentedre-rolled
Comment #33
tstoecklerThese look like merge errors. Should be simply
for each of them
Comment #34
thedavidmeister CreditAttribution: thedavidmeister commentedYeah, #25 needs a re-re-roll without re-introducing theme() everywhere.
Comment #35
benjf CreditAttribution: benjf commentedok, thanks for the quick feedback! Hopefully this one is correct.
Comment #36
tstoecklerYup, that's it. Awesome response time!!! :-) Let's do it.
Comment #38
star-szrNeeds another quick reroll after #2010672: Rename 'type' variable of theme_mark to 'status' was committed. Thanks for all the rerolls @benjf!
Comment #39
thedavidmeister CreditAttribution: thedavidmeister commentedComment #40
pwieck CreditAttribution: pwieck commentedre-rolling now
Comment #41
pwieck CreditAttribution: pwieck commentedre-rolled to latest head
Comment #42
pwieck CreditAttribution: pwieck commented#41 passed and ready for review or rework
Comment #43
thedavidmeister CreditAttribution: thedavidmeister commentedThe patch looks fine to me, but the interdiff is backwards I think.
Comment #44
alexpottI've scanned the codebase for all usages of the item_list theme and everything appears to be updated correctly. This needs a change notice...
Committed 088b191 and pushed to 8.x. Thanks!
Comment #45
aspilicious CreditAttribution: aspilicious commentedis a task
Comment #46
star-szrCreated a small change notification similar to the one for #2010672: Rename 'type' variable of theme_mark to 'status':
https://drupal.org/node/2030833
Comment #47.0
(not verified) CreditAttribution: commentedUpdated issue summary.