Problem/Motivation

Too many theme functions and theme templates that duplicate the same resulting markup structure.

Proposed resolution

  1. Consolidate theme functions and templates into single implementations
  2. Leverage theme hook suggestions to allow targeted theme/template overrides and variable preprocessing.

Procedure

  1. Go through the list and identify similar things and group them together. See Twig conversion/refactoring/clean-up issues
  2. Find existing issues or create a new to consolidate groups of similar theme functions, using theme suggestions and preprocess to handle minor differences (or just skipping the differences if they're superfluous).

Details

Remaining tasks

Blockers

Not actually blockers for consolidating theme functions and templates, but without resolving these issues, modules and themes will not be able to easily target/override a specialized theme hook, which means a critical regression compared to D7:

Issues Pt 1 (theme functions that are grouped together)

links

tables

item lists

containers

Issues Pt 2 (other theme functions that need attention)

User interface changes

None.

API changes

None.

Files: 
CommentFileSizeAuthor
#3 1804614-hook-theme-info-csv.diff.txt2.36 KBjwilson3

Comments

Note that we agreed that the Twig conversion will do this so I am not 100% this needs to be a separate issue.

Issue tags:-d8dtx, -Twig, -theme system cleanup
StatusFileSize
new2.36 KB

I've created a spreadsheet listing all of the D8 theme functions that I think will be useful to anyone working on this, or the Twig stuff.

Its based on a small script (attached) that converts all the information from module_invoke_all('theme') into a comma-separated list.

In doing this, I've found a couple interesting observations:

  1. The script used to generate the spreadsheet uses function_exists after a full core bootstrap via hook_init() on the Modules List page, to test if the theme function is available. There are a number of functions do not inform hook_theme which file needs to be loaded and are *not* present (via function_exists) at time of exectution of full drupal bootstrap. Thus, these are marked "unusable", if a template/function was found. However, there are other cases where neither a file, function, or template exists (marked "unclear" in the spreadsheet). This might be a red-herring; obviously, core works, and the proper files are loaded when they are called... the cause could be that these functions are only being called from files in special parts of the site where the file is guaranteed to be loaded.
  2. The "render element" from hook_theme() seems really inconsistently used. This column, in theory, could be used to identify similar functions, however in a lot of cases, there are theme functions that are simple wrappers for theme_table() but instead of labeling the render element a table, it's just labeled the more generic and less informative element. It is also possible I've missed the point entirely for this, because, the documentation about "render element" is totally unclear about what this is used for.

Issue summary:View changes

Distinguish between unclear and unusable

The functions that don't provide a "file" and are also not loaded after full Drupal bootstrap, creating conditions where trying to use the theme function without knowing which file its in and loading that file by hand, or through some other mechanism:

theme_comment_preview
theme_image_style_list
theme_image_style_preview
theme_image_resize_summary
theme_image_scale_summary
theme_image_crop_summary
theme_image_rotate_summary
theme_node_admin_overview
theme_help
theme_update_page
theme_install_page
theme_task_list
theme_authorize_message
theme_authorize_report
theme_update_report
theme_update_version
theme_update_status_label

In analyzing the data in the spreadsheet I also stumbled across and filed this gem: #1805342: Fix inconsistent use of underscores in template file names

Wow, that's some impressive analysis work, James! Nice! :D

I saw a spreadsheet somewhere which had a list of Twig conversions - this was in irc and I didn't save the link. If someone knows where that is (probably Jen Lampton or FabianX do), it'd be good to cross-reference against that one.

Issue summary:View changes

Link to a google spreadsheet listing all theme functions.

Issue summary:View changes

Added reference to the other google doc spreadsheet.

I've added spreadsheet in #10 to issue summary.

Issue summary:View changes

Put the spreadsheet links in list format.

Issue summary:View changes

Expand upon what "unclear" means.

Keep an eye on #1804488: [meta] Introduce a Theme Component Library too, to make sure we don't duplicate/invalidate work.

Looks like #1484720: [Meta] Reduce the number of theme functions/templates is a duplicate meta issue, no need to have both?

Issue tags:+d8dtx, +Twig, +theme system cleanup

It does look like a dupe, but I'll let others decide which to close. Probably would be good to get them all under the same tags too.

Issue summary:View changes

Added link to theme_filter_tips refactoring issue

Status:Active» Closed (duplicate)

This is the newer issue. Can we move important information from here to the other issue?

Status:Closed (duplicate)» Active

I'm re-opening this issue, because the outlined summary is much more actionable than the discussion in #1804488: [meta] Introduce a Theme Component Library.

Marked #1484720: [Meta] Reduce the number of theme functions/templates as absolute duplicate, since it essentially asked for the identical actions to happen.

This issue is about doing something. You may follow #1804488: [meta] Introduce a Theme Component Library for discussion.

Speaking of, while most of the consolidation may happen already, doing so will leave us in a regression state, since "subpatternized" theme functions à la '[theme_]links__operations' cannot really be overridden by a theme yet, nor properly preprocessed by modules. This has to be fixed in:
#939462: Specific preprocess functions for theme hook suggestions are not invoked

Issue summary:View changes

Added link to #54898

Issue summary:View changes

Added Sun's comment about sub-patterns #939462: Specific preprocess functions for theme hook suggestions are not invoked to issue summary.

Issue summary:View changes

Added description on how to create a list of themables using the patch in comment #3.

Issue summary:View changes

Minor update to explain where my google doc came from.

I've revamped the issue summary. Unfortunately, there was a conflict while editing, but I think/hope I've re-incorporated the last edit(s) correctly.

#2 said all I had on this, I won't repeat myself but I am unfollowing, right now.

Oops, sorry, guess we were editing it at the same time without knowing... I was just adding your feedback in #16, but thanks for the revamp.

I'd also like to point out, I've added a column to the spreadsheet called "What is it really?" which we could use for identifying what kind of html is produced. So far, I'm using things like:

* list
* list of links (ie a menu)
* list item (link)
* text wrapper (span)
* text wrapper (em)
* block-level wrapper
* block-level wrapper w/ title

chx's comment notwithstanding, AFAICT, Twig is only still in its infancy phase of one-to-one conversion of theme functions and tpl.php templates to html.twig templates and with preprocess functions. I think there is high-level work that can be done in parallel to think about how we can consolidate theme functions. I dont see this standing *outside* of twig, but working directly with it.

Issue summary:View changes

Revamped issue summary. [sun]

Issue tags:+d8dtx, +Twig, +theme system cleanup

I've finished categorizing all the Drupal 8 theme functions and have sorted the spreadsheet so they are ordered by similarity. Couple observations:

  • Not sure why my spreadsheet report 186 themables, while the spreadsheet being used by the twig conversion only has 135.
  • There is an extraordinary amount of themables that amount to nothing more than an html block-level wrapper element around a title element and a body element, and in a few casess some secondary content (eg, description text, or meta data, or footer section etc.). Would having one single themable function theme_container, that accepts options similar to item_list make sense? It would default to a div for the block wrapper, but could pass in any block level element in the variables. The title and wrapper element would also have sensible defaults but be customizable as well. We would then also have a couple preset alternate configurations using theme suggestions, for example: theme_container__aside (change the title wrapper to an h3), theme_container__article (change the wrapper to 'article', change the title wrapper to an h2). This would reduce the twig conversions from lots of very similar templates to very simple preprocess functions
  • there are two feed templates in the aggregator module that could really be generalized into something like theme_feed or theme_feed__rss theme_feed__opml so that contrib / services / etc could take advantage of them. We have theme_html, why no drop in templates for other standard output formats?
  • More to come...

Issue summary:View changes

It's hook_theme

There are 22 themables in core that each implement what amounts to a form table. There are some minor variations, eg, some have rows that are draggable, some have a column of operations, some Have a pager. It seems like there could be a way to pull out some of the common code into a new themable, and provide options for the variations above to be able to use one single theme template, and only implement preprocess functions.

Very supportive of a generic wrapper template. I suggested something similar in http://drupal.org/node/1804488#comment-6586708, a "wrapper--{the full file name of the template you want to wrap}.tpl, using either #theme_wrappers or a new function." Something standardized across all theme functions so that you don't need to fork a template just to add <div class="wrap"></div> for one context.

Not sure on the block-level wrapper consolidation idea. Replacing some of them with article (e.g., article__comment) might make sense, because the semantics for articles are shared.

However, a generic "container" for all of these would be too much consolidation, IMHO. I don't think the goal is to reduce Drupal to 5 theme functions, right? ;)

What definitely makes more sense is to attempt to consolidate the text wrappers. We have a couple of issues for those already (see OP). For those that are wrapping the markup with a DIV, the existing #type/#theme container might be sufficient already. (Apparently, theme_container was misclassified as form wrapper -- but it's a generic wrapper.)

@chx it's my understanding that the Twig sandbox is focusing on a straight conversion from .tpl.php to .twig. Only once that's done will the focus move to consolidating templates (someone can correct me if I'm wrong). Given that, it makes sense for those who care about consolidating templates to try to figure out some of that now where it's possible, since it's not duplicating any work happening elsewhere.

Would like to put:

http://drupal.org/project/fences

into the mix.

Looks like I didn't read closely enough when I posted my first comment. My thoughts on the wrappers was more to remove all nonsemantic wrapping elements (anything outside the primary container, e.g., elements with a "*-wrap" class) in core templates and then use a standardized separate template file to add them back in, whereas jwilson is calling the containing block level element the wrapper. I'm actually with sun on this -- sorry for the static.

Re: Fences, there was a large discussion moderated by Jacine earlier this year about adding markup control in the UI. TL;DR, most were against it as a core option, mostly because it adds another place markup can be set and would introduce more complexity/ pointy-clicky maintenance time, which front end devs hate. (FYI, I use fences every once in a while and love it, but I love it in contrib.)

Issue summary:View changes

Added various issues to list.

Issue summary:View changes

add other Twig google doc

Issue summary:View changes

update

Issue summary:View changes

remove conversion to l()

Issue summary:View changes

added the one for form elements

Issue summary:View changes

added the one for containers

Issue summary:View changes

add the one for tables

Issue summary:View changes

move l to top section

Issue summary:View changes

added the component thingy

Issue summary:View changes

add item list list

Issue summary:View changes

remove dupe

Issue summary:View changes

more

Issue summary:View changes

divide

Issue summary:View changes

move again

Issue summary:View changes

links first

Issue summary:View changes

links at top

Issue summary:View changes

more

Issue summary:View changes

node-add-list

Issue summary:View changes

renderable table doesn't really belong

I also had a bunch of separate meta issues for each component grouping we'd identified in the process of converting to Twig, but this one is much better. :) I've added them to the summary here too.

Just to update everyone outside Twigland about our involvement in the markup cleanup:

Our process is First to convert all existing template files and theme functions to twig. This gives us a chance to look at every single piece of markup that comes out of core. While doing this we're finding a lot of places where things can be consolidated or eliminated (and creating issues against core). After we get everything moved to Twig (mostly as-is) we'll begin refactoring over in the Twig sandbox.

Second, we'll begin working to create our Theme Component Library (see #1804488: [meta] Introduce a Theme Component Library). We'll do this by consulting Jacine's blog post about what a component library *should* look like, but start by taking the pieces of markup that Drupal uses all the time and making those into our first components. Our components will be twig templates.

Third, we'll introduce these compontents back into core, probably in the system module (?), but much like our current theme functions in theme.inc.

Fourth, We'll start refactoring all the markup generated by each module, to use the component library (and perhaps renderable objects, see #1843798: [meta] Refactor Render API to be OO). In an ideal world, modules will not need to provide their own markup anymore, because they can build their output using the components we've provided. (Drupal is not a perfect world, so we will of course have some new template files provided by modules.) We'd like to use the process of recreating markup for the modules using components to test the breadth of our component library :)

Fifth, we'll create patches against each module, replacing the theme functions and tpl.php files with a Twig alternative that uses the Component Library heavily, (if not entirely).

Since we're not really sure what's going to be happening first (Consolidation, Twigification, Markup improvements, etc) we've been creating issues for every consolidation and markup improvement suggestion against core instead of internally against our sandbox. Our hopes are that by merging with HEAD often we'll avoid duplicating work, and make the fastest progress :)

For the complete Twig roadmap, see: Our Sandbox page.

Status:Active» Postponed

Jen, re: your #27,

Based on our most recent discussion (2012-12-20 Twig Hangout), I see things a little different relative to Twig. Specifically: we should complete conversion before consolidation. This is where I see us (between 1.a and 1.b). (I'll post this to the Twig sandbox):

CONVERSION

Note: We've currently got lots of similar templates -- that's OK -- just wait... this is only *conversion*. And it needs to be done now!

1. Convert remaining theme functions and templates in the Twig sandbox.

1.a. Everyone/Anyone: Finish conversions.

1.b. Unofficial self-selected gate-keepers (jen, myself, joel?, c4rl?): Verify conversions pass all tests.

2. Work from #1757550: [META-63] Convert core theme functions to Twig templates.

CONSOLIDATION

Note: This is where we'll be getting rid of similar theme functions and templates from core.

3. Consolidate theme functions and templates. (This issue).

COMPONENTS

Note: Do this in core? Or first in a sandbox like we've discussed? I think we need a wider audience (Core).

4. Create theme components (special theme functions and/or renderable objects).

5. Implement theme components. File and fix 'theme system cleanup' issues.

Postpone?

Status:Postponed» Active

Sorry why is this postponed on conversion? Can't people be working on it in parallel? There's definitely not time to individually convert every core template to twig then go back and do this afterwards. At this point I'm concerned whether either of them are going to happen at all since there's been no activity in the core queue on either front for nearly two months now.

I thought postpone and only do conversion now since its seems we're way closer to that. I'm just trying to get something towards closure, and it seems that would be conversion only. Looking at form.inc, though, it'd be pretty painful to NOT consolidate now. The next patch I submit will be for consolidation in form.inc in #1812724: Consolidate all form element templates and add theme_hook_suggestons.

@steveoliver & @catch, as noted in the summary, #939462: Specific preprocess functions for theme hook suggestions are not invoked is a blocker for this. Myself and @tim.plunkett got that green 1 Month ago today. Could someone please take a look at it and get it pushed through to completion. @fabianx put it on his list todo, but obviously he is busy, as I am sure you both are. If we could just get someone to look it over, then I can try and address any issues found and move it forward.

I don't think #939462: Specific preprocess functions for theme hook suggestions are not invoked blocks this at all. We already use preprocess functions in this way, the more we use them the more obvious that bug becomes but fixing that issue shouldn't stop any individual patch. If absolutely necessary we can make that issue critical though given a wider use of suggestions.

Issue summary:View changes

remove issues that are linked from other meta issues

The blocker here is this: Before we can use suggestions to consolidate similar templates, we'll need to make sure that suggestions we need will work correctly. To answer #29 this issue was postponed because there's no point in updating calls to from one function that works to another one that doesn't.

I don't think all the sub-issues here will run into that problem, so making this issue active again is probably a good idea. But those working on it these issues should understand that we do still have critical bugs in core that may impede their progress :)

However, we do have a new proposal for a solution to that problem, if you are interested see #2004872: [meta] Theme system architecture changes.

Issue summary:View changes

other issues

tagging

Issue summary:View changes

issue summary cleanup

Issue summary:View changes

reorg

Issue summary:View changes

lists

Issue summary:View changes

containers

Issue summary:View changes

cleanup

Issue summary:View changes

Add issue to remove views_mini_pager theme hook

Issue summary:View changes

Update nid for views-mini-pager consolidation

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.