There are at least two systems in Drupal that could really benefit from a lazier method of theming that lets the names be more arbitrary. The in core example is form API, which could utilize such a system to allow each piece of a form to be themable, automatically. Let's take an example:

function foo_form() {
  $form['bar']['user']['name'] = array(
    '#type' => 'textfield',
    ...
  );
  ...
  return $form;
}

Since all items in form api end up being a variant on theme_form_element(), if we set this up right we could automatically theme this by creating the right theming function (or template):

function theme_form__foo_form_bar_user_name($element) {
  ...
}

Or...

function theme_form__foo_form_bar_user($element) {
  ...
}

All the way down to

function theme_form__foo_form($element) {
  ...
}

Now, registering all of these for every form is basically impossible. We need a way to figure out which of these will work. If the theme registry could register a pattern, we can accomplish this:

(roughly)

   'form_element' => array(
     'arguments' => array('element' => NULL),
     'pattern' => 'form__*',
     'default' => 'theme_form_element',
   ),

The theme system would then look for anything of theme_form__* or phptemplate_form__* or whatever is appropriate (form__foo_form.tpl.php) and register that.

Now, one more thing is required. Because theme() needs to be as fast as possible, for a function like this there are two ways to approach it, I believe. We do *not* want to have to hit every pattern in the system if a theme isn't found...that's way, way too much overhead. I believe we need a way to inform the system when we're using a theme implementation that fits a pattern, and what pattern it's using (so it knows when to just use the default implementation). There are two ways to do this. one is a new theme() function, and the other is to give theme() a magic argument.

Form 1:

  theme_dynamic('form__foo_form_bar_user', 'form_element', $element);

Form 2:

  theme(array('form__foo_form_bar_user', 'form_element'), $element);

In form 1 it's more clear, but we're going to end up with code duplication. Form 2 sacrifices clarity for for brevity, but also adds a slight (very slight) performance hit since every theme() function must now include an is_array($hook).

I plan to work on this patch ASAP, but I welcome discussion about it before I get to coding.

CommentFileSizeAuthor
#24 dynamic_theme_1.patch9.59 KBmerlinofchaos
#20 dynamic_text.tar_.gz__0.txt869 bytesmerlinofchaos
#19 dynamic_theme_0.patch7.06 KBmerlinofchaos
#6 dynamic_text.tar_.gz_.txt10 KBmerlinofchaos
#5 dynamic_theme.patch6.82 KBmerlinofchaos
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dvessel’s picture

dvessel CreditAttribution: dvessel commented

subscribing

dvessel’s picture

dvessel CreditAttribution: dvessel commented

If anyone is confused on why this is needed in the first place (as I was).. The theme registry would have to store every single theme function. In places like theming forms or even Views where there are multitudes of functions that follow a predictable pattern, the possibility of *wildcards* can be used to prevent the registry from being overloaded.

So, I think the key thing here is to keep the theme registry on the more efficient side.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

Another example that could be really useful:

  theme(array('table__forum_overview', 'table'), $header, $rows);
moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

subscribe

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented
FileSize
dynamic_theme.patch6.82 KB

Finally, here is the patch to do this.

It moves some logic out of phptemplate.engine so that all engines can have easier access to it.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented
FileSize
dynamic_text.tar_.gz_.txt10 KB

Here is a simple module & theme which I used to test the basic behavior.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented
Status: Active » Needs review
merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

by the way, this patch is going to be basically necessary to port Views to 6.x.

Views relies on being able to theme each view independently. It accomplished this with a bit of code that looked to see if theme_views_view_TYPE_VIEWNAME existed, and if not backed down to theme_views_view_TYPE.

That's basically what this system does, but using registry to accomplish it via wildcards. It would be burdensome to have to register for every view, which is the other solution. THat solution could grow the theme registry cache significantly; and formapi can't even do that, as there isn't really any way to know all of the possible form combinations at hook_theme() time.

profix898’s picture

profix898 CreditAttribution: profix898 commented

Really nice. Subscribing.
I hope to review and test sometime this week.

chx’s picture

chx CreditAttribution: chx commented
Priority: Normal » Critical
Senpai’s picture

Senpai CreditAttribution: Senpai commented

Subscribing

dmitrig01’s picture

dmitrig01 CreditAttribution: dmitrig01 commented

subscribe :) love it

Senpai’s picture

Senpai CreditAttribution: Senpai commented

Innocent question here, but wouldn't the theme_dynamic('foo', 'form_element', $element) be far easier to keep track of in a large site with many developers working on it? Not to mention it's a more direct coding approach rather than taking the aforementioned performance hit. However slight, a method that is easier to read, doesn't involve another callback, and is generally more specific gets my vote. There's already enough modules out there that can slow sites to a crawl. Let's not add anything that could negatively affect performance if there's an alternative that's actually easier to read in plain English code.

Merlin, awesome idea here. I really like this.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

I setup the whole test environment for this but i honestly don't know how to verify the output versus expectations.

One bummer about this is that the 'template log' feature in devel HEAD can't learn about these candidate theme functions like it does for templates now. i would appreciate if earl looked into that. devel hooks into all theme calls by defining a phptemplate_preprocess()

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

I picked the theme() version to do initially because theme_dynamic() will have a less efficient footprint in the end; either because it will have to duplicate a bunch of code from theme() or because it will call into theme() and then run (again) a bunch of code that's already been run.

Moshe: Interesting issue you raise. I don't have any good answers immediately. I'll have to cogitate upon it and see what comes up.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

Moshe: The results of the test script are largely determined by whether or not there is an override for the specific cases.

In the template.php for the theme I included, there are function overrides for 'a' and 'c' which should output 'a' and 'c' respectively.

There is a template override for 'd'.

Those are teh only overrides; anything other than 'a', 'c', or 'd' should output the results of the default theme.

dvessel’s picture

dvessel CreditAttribution: dvessel commented
Status: Needs review » Reviewed & tested by the community

Okay, I got the wrong idea. It's not about the stored registry since every iteration of these found is stored. (did a print var_dump(cache_get("theme_registry:test_theme", 'cache'));. it's all in there.). It's about how these functions are called and having it fallback by looping through with $candidate. --each function in the parameter passed as an array.

Just like theme() has a fallback mechanism by checking the theme, theme engine and original function. This goes in another direction so it can check each iteration of theme__a_b_c, theme_a_b, theme_a or theme__* pattern. Sorry for the confussion.

Tried it with the test files and other functions I made up. Works very well.

I've been out of it for a while. Is this still valid? If so, I'd say it's RTBC. Hope it's not too late.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Needs work

- This looks like something that we'll want to benchmark?

- What do you mean with "May be an array, in which case the first existing hook will be used."? Extend the code comments to explain why you might want to pass in an array.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented
FileSize
dynamic_theme_0.patch7.06 KB

Attached is new patch with better comments.

In order to benchmark this, I modified the test script to do 10,000 calls to theme() and throw away the output.

With the patch: 0.015594 sec.
Without the patch: 0.013845 sec.

Net change: For 10,000 calls to the theme function, it costs us just under 2ms. This tells me the is_array() check is very fast.

With backup it costs us a bunch more time, but this is to be expected -- we're looking to see if a theme function exists and it doesn't, so we're going to a backup.

I'll attach the new script so people can play with it. (Note that without the patch it generates a bunch of NOTICE errors; these are expected).

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented
FileSize
dynamic_text.tar_.gz__0.txt869 bytes

New test script.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented
Status: Needs work » Reviewed & tested by the community

I think this successfully addresses both of Dries' points, so back to RTBC.

Dries’s picture

Dries CreditAttribution: Dries commented

Thanks for benchmarking and improving the help text. Much appreciated.

The documentation is still a bit vague-ish so I'm not going to commit it yet. Maybe someone will stop by to massage the documentation some more. For example, it's probably useful to add another sentence that adds a small example of how that would work.

I see no reason to block a patch that is 99% ready, so this patch is hereby guaranteed to be committed in the next couple of days (unless a significant show-stopper comes up). This buys us a bit more time to massage the documentation (or not).

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

I thought about an example, but that doesn't feel like something that should go on the doxygen, but rather in the developer's handbook. That said, I'm not sure about that; I have real troubles finding my way around Drupal documentation much of the time, and I think part of the reason is that there's not often a clear answer for "where should this be documented."

I will poke around at the documentation and see.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented
FileSize
dynamic_theme_1.patch9.59 KB

This one includes more documentation in the section on theming in general (which feels, to my mind, like a more appropriate place to put an example), as well as adding a little more to the theme() doxy itself. It also fixes a problem I noticed in the documentation with the way doxy works.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

I read over the documentation changes. While there are a couple places where things could stand to be a bit more clear, it makes a lot more sense to address this in kind of an over-arching "fix the theme documentation as a whole" effort than "overly polish one aspect of it" imo.

I'd approve of this being committed as-is (the docs here are more than adequate) and a clean-up effort for the entire theme.inc file for extra sparkly polish coming as a subsequent patch.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed. Thanks.

Anonymous’s picture

(not verified) CreditAttribution: commented
Status: Fixed » Closed (fixed)
drewish’s picture

drewish CreditAttribution: drewish commented

i've been puzzling over the whole pattern thing for the last few hours... i'll never complain about my patches being held up because of bad documentation ever again. if i'd come across this issue before it was committed i'd have asked for the feature to be used in at least one place in core if only for demonstration purposes.