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.
Updated: Comment #N
Problem/Motivation
#2201693-18: Field output supporting code should move out of field.module effectively reverted most some of #1987398: field.module - Convert theme_ functions to Twig.
Proposed resolution
Let's fix it! Re-do all applicable changes from #1987398: field.module - Convert theme_ functions to Twig to the files in the new locations. For example theme_field() and theme_field_multiple_value_form() should be gone.
Remaining tasks
- Create the patch.
- Patch needs review.
User interface changes
n/a
API changes
n/a
Comment | File | Size | Author |
---|---|---|---|
#22 | 2226233-22.patch | 724 bytes | andypost |
Comments
Comment #1
star-szrThe changes to field.html.twig at least are intact, so correcting the issue summary.
Comment #2
Manuel Garcia CreditAttribution: Manuel Garcia commentedComment #3
Manuel Garcia CreditAttribution: Manuel Garcia commentedI believe most things were actualy moved into system module.
template_preprocess_field
,theme_field
, andtemplate_preprocess_field_multiple_value_form
was moved into includes/theme.incthe return array of
field_theme
was added intodrupal_common_theme
field-multiple-value-form.html.twig
andfield.html.twig
was moved into core/system/templatesThe only thing I believe stil left to do is that they left
field_theme_suggestions_field
in field.module, while they created it also on system.module assystem_theme_suggestions_field
, the attached patch removes it from field.module.Comment #4
Manuel Garcia CreditAttribution: Manuel Garcia commentedThey also removed css from field.module.css, which was moved into system.theme.css
Comment #6
alexrayu CreditAttribution: alexrayu commentedProposed patch.
Comment #7
alexrayu CreditAttribution: alexrayu commentedComment #8
alexrayu CreditAttribution: alexrayu commentedNow I see patches are identical except for indices. Re-queuing Manual's original patch for testing.
3: drupal-2226233-2.patch queued for re-testing.
Comment #9
galooph CreditAttribution: galooph commentedPatch in #6 looks ok and applies cleanly.
Comment #10
star-szrThe current patch is duplicating work being done at #2201693: Field output supporting code should move out of field.module and I'd rather leave that cleanup to that issue. I want to focus on re-removing the theme functions and making sure our preprocess changes are maintained. I went through and diffed everything before/after #2201693-18: Field output supporting code should move out of field.module and here's what I found that we need to do in this issue:
Edited to linkify screenshots.
Comment #11
star-szrAnd thank you @Manuel Garcia, @alexrayu, and @galooph for tackling this one :)
Comment #12
ckrinaStarting to work on that.
Comment #13
Manuel Garcia CreditAttribution: Manuel Garcia commentedThat makes total sense Cottser, and thanks ckrina for taking this on!
Comment #14
ckrinaAttaching the patch.
Anyway, there were still some references to theme_field() and theme_field_multiple_value_form():
"Callback for usort() within theme_field_multiple_value_form()." > "Callback for usort() within field-multiple-value-form.html.twig."
I've made the change.
:
Comment #15
galooph CreditAttribution: galooph commented_field_sort_items_value_helper() is also used for a usort inside theme_file_widget_multiple() in core/modules/file/file.field.inc. Might be worth adding that in the docblock too, eg "Callback for usort() within field-multiple-value-form.html.twig and theme_file_widget_multiple()."
Comment #16
star-szrDon't worry about those - we're tackling any docs followup in #2226863: Update stale references to theme functions that have been converted to Twig :)
Let's see what the bot says. Patch looks good to me though.
Comment #17
galooph CreditAttribution: galooph commentedThanks @Cottser, I wasn't aware there was a separate cleanup issue.
Patch looks good and applies fine.
Comment #18
star-szrThanks @ckrina :)
The patch does what it's supposed to do, there is the following extra (bonus) change but it's not hurting anyone as far as I can see (and it's a correct change):
Comment #19
star-szrMaking the problem/motivation less dramatic ;)
Comment #21
webchick$clobbering--;
The only thing that threw me was all those nice docs getting deleted. But basically they're no longer needed, since theming fields now works like theming everything else.
Committed and pushed to 8.x. Thanks!
Comment #22
andypostDo not mix field name and type on one level of template suggestions.
Faced with that in #1962846-8: Use field instance name for header of comment list, drop comment-wrapper template
That leads to
field__comment
template could be used for for any field that named "comment" for example custom entity with base field named soComment #23
star-szrThat looks entirely unrelated, please open a separate issue. Thanks!
Comment #24
andypostFiled follow-up #2229355: Field template suggestions are colliding