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.We now have the ability to have (sane and meaningful) preprocess (and process) functions for all theme hooks: #572618: All theme functions should take a single argument to make preprocess sane and meaningful. I don't think there are many, but there are some theme functions that combine way too much data and logic along with presentation decisions. This makes it unnecessarily difficult for modules to adjust the data and logic decisions. Patch coming in a follow-up comment.
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | theme-use-preprocess-600658-5.patch | 4.28 KB | effulgentsia |
| #4 | theme-use-preprocess-600658-4.patch | 7.62 KB | effulgentsia |
| #1 | theme-use-preprocess-600658-1.patch | 9.1 KB | effulgentsia |
Comments
Comment #1
effulgentsia CreditAttribution: effulgentsia commentedMight as well bite this off in small chunks. This patch refactors:
I didn't attempt anything with theme_pager_*: happy to leave those to someone who has more experience with pagers -- I don't.
I also didn't attempt anything for theme functions that take a renderable element as their only parameter, since those are already sufficiently generic.
I think that might be it for theme functions implemented in the "includes" folder. If people's reactions to this is positive, I'll work on the ones in the "modules" folder.
Comment #3
sunPlease leave out theme_menu_local_tasks() in here. That function serves a special purpose.
Comment #4
effulgentsia CreditAttribution: effulgentsia commentedOk. This one fixes the test failure in locale and removes making any changes to theme_menu_local_tasks(). I guess this patch just deals with two theme functions then. IMO, the change to theme_locale_languages_overview_form() is obvious. Correct me if I'm missing why we wouldn't want this. What about the one to theme_tablesort_indicator()? Is it appropriate to think of the image information (path, alt, etc.) as data, desirable for modules to be able to change? Or is that legitimately within the purview of the theme function and should be left as it is in HEAD, in which case, I'll reduce this patch to just theme_locale_languages_overview_form()? I'd like to know the answer to this before sweeping the module-specific theme functions.
Comment #5
effulgentsia CreditAttribution: effulgentsia commentedRemoved theme_tablesort_indicator() change. After all, modules have the ability to intercept this at hook_preprocess_image(). I'll look through module-specific theme functions, but leaving this as "needs review", cause I'd still like feedback about this one.
Comment #6
effulgentsia CreditAttribution: effulgentsia commentedI won't have time to do a full sweep of this properly before 10/15, so changing to goal for D8. If someone else wants to take this on, please do and change the version back to D7 if you have a workable patch before 10/15.
Comment #7
sunIf it boils down to 2 theme functions, then I see no reason to defer this clean-up.
Comment #8
effulgentsia CreditAttribution: effulgentsia commentedThanks. I'm glad there's interest. Patch #5 just has one, and at the very least that one should land if there's support for it. So, I'm surprisingly impressed with the functions defined in the files within "includes". But the theme hooks from files in the "modules" folder are still a mess. And regarding #5, I'm noticing a lot of inconsistency in how operations links are rendered across the various places that have operations links. Is there any issue(s) already in the queue that are trying to bring consistency to that? That might be a way to approach this task in a manageable way.
Comment #9
sunRegarding those operation links, and in general, links in forms and renderable arrays, I already thought multiple times of a new #type = 'link', i.e. theme_link() function. This function would basically be just a wrapper around l(). All of those links are unalterable, because they are strings already. Not only the Overlay patch currently has the problem that it conditionally needs to form_alter() the content management overview form to add a URL query parameter to some of the links in the table.
I started an initial attempt by hi-jacking another issue: #283723-34: Make menu_tree_output() return renderable output
I would really love you if we'd be able to create an issue + patch for doing that. :)
Comment #10
effulgentsia CreditAttribution: effulgentsia commentedFunny. I'm trying to push through the reverse: make l() a wrapper to theme_link(): #318636: Make l() themable. What do you think of that one?
But what I'm referring to with respect to operations links is the ability to add new ones. For example, module x decides it wants to show "edit" and "delete" links for one of its pages. But module y wants to add "SOME OTHER OP" in addition to it. But every theme function seems to handle its ops links differently (some use colspan=X, ugh!).
Comment #11
sunugh, I went ahead too fast then :P Feel free to mark #602522: Links in renderable arrays and forms (e.g. "Operations") are not alterable as duplicate.... :-/
Comment #13
effulgentsia CreditAttribution: effulgentsia commentedSadly, as I feared, I wasn't able to get this done for D7, and must now make it a goal for D8. However, at least #602522: Links in renderable arrays and forms (e.g. "Operations") are not alterable made it to RTBC in time, and I think there's other issues in the D7 RTBC queue that make incremental progress as well.
Comment #14
effulgentsia CreditAttribution: effulgentsia commentedtitle tweak
Comment #15
moshe weitzman CreditAttribution: moshe weitzman commentedStumbled on this one.
Locale has been worked over in D8 so I don't know if most recent patch is still valid.
Comment #16
steveoliver CreditAttribution: steveoliver commentedVery closely related to, if not a duplicate of, #1757550: [Meta] Convert core theme functions to Twig templates.
Comment #17
star-szr