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.

Files: 
CommentFileSizeAuthor
#5 theme-use-preprocess-600658-5.patch4.28 KBeffulgentsia
Passed: 13484 passes, 0 fails, 0 exceptions View
#4 theme-use-preprocess-600658-4.patch7.62 KBeffulgentsia
Passed: 13481 passes, 0 fails, 0 exceptions View
#1 theme-use-preprocess-600658-1.patch9.1 KBeffulgentsia
Failed: 13639 passes, 0 fails, 4 exceptions View

Comments

effulgentsia’s picture

Status: Active » Needs review
FileSize
9.1 KB
Failed: 13639 passes, 0 fails, 4 exceptions View

Might as well bite this off in small chunks. This patch refactors:

  • theme_locale_languages_overview_form
  • theme_menu_local_tasks
  • theme_tablesort_indicator

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Please leave out theme_menu_local_tasks() in here. That function serves a special purpose.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
7.62 KB
Passed: 13481 passes, 0 fails, 0 exceptions View

Ok. 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.

effulgentsia’s picture

FileSize
4.28 KB
Passed: 13484 passes, 0 fails, 0 exceptions View

Removed 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.

effulgentsia’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work

I 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.

sun’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Needs review

If it boils down to 2 theme functions, then I see no reason to defer this clean-up.

effulgentsia’s picture

Thanks. 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.

sun’s picture

Regarding 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. :)

effulgentsia’s picture

Funny. 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!).

sun’s picture

ugh, 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.... :-/

effulgentsia’s picture

Title: Use preprocess functions to separate data from presentation in function-implemented theme hooks » Refactor core module theme functions to separate data from presentation
Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work

Sadly, 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.

effulgentsia’s picture

Title: Refactor core module theme functions to separate data from presentation » Refactor core module theme functions to separate data logic from markup decisions, enabling modules to alter the former

title tweak

moshe weitzman’s picture

Stumbled on this one.

Locale has been worked over in D8 so I don't know if most recent patch is still valid.

steveoliver’s picture

Very closely related to, if not a duplicate of, #1757550: [Meta] Convert core theme functions to Twig templates.

Cottser’s picture

Title: Refactor core module theme functions to separate data logic from markup decisions, enabling modules to alter the former » Refactor core module theme functions (templates) to separate data logic from markup decisions, enabling modules to alter the former
Issue summary: View changes
Issue tags: +theme system cleanup, +Twig

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.