The primary purpose of the theme registry patch is to make it possible for Drupal core and Drupal modules to provide their theme functions directly as .tpl.php files without impacting performance too badly.

This patch requires all theme functions used by Drupal to be registered by the owning module (system.module registers on behalf of the *.inc files). This registry can declare the themes to be traditional functions, or it can declare them as templates and can declare their arguments. If declaring them as functions it can declare them in an additional file that is loaded on demand (an example is the new theme.base.inc where theme_page, theme_node and theme_block are moved; these theme functions are almost never used).

The basic template rendering engine of phptemplate has been moved to core; phptemplate.engine now primarily does just two things:

1) Discover what templates a theme is using. This means that simply dropping an appropriately named .tpl.php file into the theme directory will (after the cache is cleared) automatically be used. _phptemplate_callback is therefore no longer needed and has been completely removed.

2) Provide default variables that are common to pages. Some of this may later be trimmed down or moved into the theme itself, but that is a patch for elsewhere.

_phptemplate_variables has been changed slightly. _phptemplate_variables is now the 'private' version meant to be used by the theme engine, and phptemplate_variables (as well as THEMENAME_variables) can be used by the theme's template.php.

Additionally, phptemplate_variables_HOOKNAME has been introduced, which drastically simplifies the syntax for modifying the variables for templates. Also, phptemplate_variables now takes a reference and does not return the variables requiring a lot of array_merges.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

m3avrck’s picture

subscribing

dmitrig01’s picture

+1
subscribing

dvessel’s picture

-looks for the subscribe checkbox

Why the big changes? So it's easier to drag & drop module .tpl.php files into your own themes? And does this mean the end of plugging in other template engines? Will have to test this out.

add1sun’s picture

Hunk #4 failed against fresh head. Hackfest has bitten you!

moshe weitzman’s picture

very exciting. subscribe

we are just overloading the heck out of poor system.module. i would love to move the .inc files into required .module files. .inc has no benefit anymore. but thats for another patch. don't discuss that here. contact me privately.

adrinux’s picture

Very interesting. I'll need to checkout head to look at the patch though...subscribing.

merlinofchaos’s picture

FileSize
65.79 KB

Here's an updated patch that will apply to HEAD.

FIXED: I forgot to have phptemplate.engine discover theme functions as well as theme templates; now it discovers both, so phptemplate themes won't have to register functions. (This probably means that I need to go through the .theme functions and provide registration for their functions).

Just FYI: Any time a change gets checked into phptemplate.engine this patch is unlikely to apply; too many major changes. Merge conflicts and all.

kbahey’s picture

Subscribing.

merlinofchaos’s picture

FileSize
65.64 KB

Um. Left some debugging code in last patch. Sorry. Updated:

Caleb G2’s picture

subscribing

laura s’s picture

Beautiful! :D Subscribing and making note. We'll try to test in coming days.

Wim Leers’s picture

Very interesting! Subscribing.

cghobbs’s picture

Subscribing.

merlinofchaos’s picture

FileSize
66.9 KB

Prior patch had issues with install.php/update.php since the maintenance theme is special.

Heine’s picture

Very interesting. So far only two issues (but out of time for today):

- Rename system.module: function theme_system_themes to theme_system_themes_form to match the theme registry.

- drupal_render_form calls theme_get_registry before init_theme has run on admin/build/modules. As theme_get_registry returns NULL in that case, the form is unstyled.

Leaving this to review (no use in rolling patch after patch after patch).

merlinofchaos’s picture

FileSize
67.94 KB

Updated for HEAD after the kitchen sync (sic). Includes Heine's recommendations too.

Steven Jones’s picture

subscribing

rickvug’s picture

subscribing... this is an important one.

webchick’s picture

subscribing.. will try and test this tonight.

merlinofchaos’s picture

FileSize
68.29 KB

This patch fixes a couple of problems caused by the merge with the kitchen sink.

bcn’s picture

tracking...

merlinofchaos’s picture

FileSize
68.3 KB

Updated per chx' commentary on IRC. Mostly cosmetic fixes.

Dries’s picture

The caching is somewhat annoying when working with template files. Did we try benchmarking this with and without caching? Are we sure we need the caching? Also, what are the performance implications in general? It would be comforting to know that we've done some benchmarking.

(I haven't fully tested or reviewed this patch yet, but it's on my TODO list.)

merlinofchaos’s picture

The benchmarking I did, with this patch, showed a roughly .5% decrease in performance (I was simply using devel.module, loaded the page 20 times).

The process of discovering all the themes is relatively slow; by disabling the caching and using just the modules from a default Drupal core installation + devel.module, a 10% performance decrease is noted on the front page; a 5% decrease is noted on other pages. (essentially the cost of caching is static; pages with more info show the decrease less). On my server, the caching process appears to take, given some quick mental averaging of the numbers, about 20 additional ms -- and that's with a relatively small number of modules. I suspect it could get much worse.

The caching is annoying; (I have the same problems with Views). if we think it's a good idea, we could create a developer mode that will skip loading from the cache; this could mitigate the problem somewhat.

sun’s picture

"Interesting."

moshe weitzman’s picture

With some renaming and doc change, we can reuse the current pref for CSS preprocessor ... "Enable caching of templates and css preprocessing. these features spede up your site but can be a nuisance during theme development"

Dries’s picture

We can worry about that extra setting in another patch. A 10% slowdown is a lot -- so the caching must stay for now.

dvessel’s picture

Could the variables list be broken off into a theme override? The "additive" nature of running *all* the _variables_ functions from theme() makes it less flexible than it was before. As it stands now, there's no way to prevent _phptemplate_variables_[page|node|comment|block]() from running. I briefly mentioned this to merlinofchaos in IRC but it looks like it could be made more flexible by letting themers control how the variables are put together.

And there's a typo: $variables_list[] = $theme .'_variables_'; Underscore at the end I'm sure wasn't intentional.

merlinofchaos’s picture

dvessel: In 99% of the cases, I believe, we'll want the variables to be additive, not an override, meaning the way we have it now is the ideal default. We'd need to come up with an easy syntax to tell it NOT to use the engine's variables pages -- and that means the theme is responsible for rather a lot of data. I'm ok with this, except that nothing comes to mind as an easy syntax.

Will fix that typo in next patch.

dvessel’s picture

how about adding this right before $variable_list is executed:

$lineup_variables_function = $theme_engine .'_lineup_variables_function';
if (function_exists($lineup_variables_function)) {
  $variables_list = $lineup_variables_function($variables_list);
}

Your already doing something similar for getting the extension for the template files.

Your also calling theme_discover_template directly. Did you do that intentionally?

dvessel’s picture

I mean within the isset($theme_engine) condition. That would be even more flexible than what's in d5.

merlinofchaos’s picture

how about adding this right before $variable_list is executed:

What do others think about this? I'm reasonably ok with it; it's fairly minor and gives themes real control and doesn't need to be used.

Your also calling theme_discover_template directly. Did you do that intentionally?

Yes; this is partly due to namespace issues with theme_ -- I suppose I could call it drupal_discover_template but I don't really like that. It's not meant to be a theme() function, though.

RobRoy’s picture

Small nit: I would prefer it to be called drupal_discover_template instead of theme_discover_template since whenever I see a theme_ function I assume it is meant to be overwritten.

Steven’s picture

Some comments on a first read through of the patch:

  • All the _themes() entries I can see have the following form:
    +    'node_list' => array(
    +      'function' => 'theme_node_list',
    +      'arguments' => array('items' => NULL, 'title' => NULL),
    +    ),
    

    That is, the function name is duplicated in the key and in 'function'. Is this necessary? It seems quite useless. Even if there is another type of entry (file), we should make the common case as compact as possible.

  • The big $variables assignment block in phptemplate should be nicely indented around the equals signs IMO.
  • Do we really need both _variables and _variables_<hook>?
    +      $variables_list[] = '_'. $theme_engine .'_variables';
    +      $variables_list[] = $theme_engine .'_variables';
    +      $variables_list[] = '_'. $theme_engine .'_variables_'. $hook;
    +      $variables_list[] = $theme_engine .'_variables_'. $hook;
    

    Seems redundant. Is there enough use of the former to justify using it over the latter? We should not leave in cruft just for backwards compatibility (and it seems the patch already splits up / simplifies existing _variables hooks in core themes).

Will test more later, but this looks very good already.

dvessel’s picture

What do others think about this? I'm reasonably ok with it; it's fairly minor and gives themes real control and doesn't need to be used.

This can solve a small problem and open up plenty of flexibility for hardcore themers.

  1. Any modification to the style sheet queue (drupal_add_css) means drupal_get_css would have to be called twice producing 2 times the aggregated css files + 2 times the processing (and usually an ugly function call from page.tpl.php). The theme author could simply add another variables function to run before _phptemplate_page and not call drupal_get_css the second time. -There's a similar problem in Garland due to _color_page_alter but that's another issue.
  2. How about if a theme doesn't have side bars and there's no need for $zebra, $id, etc.. in the theme. Why would the theme need to run _phptemplate_variables then? Would you consider that cruft in the theme?
  3. This would also allow themers to organize variable functions to however they see fit. For example, a theme I'm working on does funny things with taxonomy to load classes and css files. With this ability I can line up a queue of variable functions and just let them run instead of calling functions from within functions spread all over the file.

I'm sure if this goes in, others smarter than me will find a great use for this. Overall, this patch is in the right direction. big ++ from me.

I don't want to sidetrack this issue so I'll say no more, but anyone have an opinion on this?

dvessel’s picture


eh, forgot closing tag.

merlinofchaos’s picture

That is, the function name is duplicated in the key and in 'function'. Is this necessary? It seems quite useless. Even if there is another type of entry (file), we should make the common case as compact as possible.

Well in the theme it would be function => 'THEMENAME_hook' and likewise 'phptemplate_hook'. Now, we *could* just put the prefix there, but that then requires more code to process and put the key + prefix together, and I think is slightly less understandable than just embedding the entire function name. Moshe brought up this point as well; I'm willing to waver on this, I suppose, but having it this way gives us the flexibility to name the function arbitrarily.

The big $variables assignment block in phptemplate should be nicely indented around the equals signs IMO.

Fair enough. I haven't seen much of this done in Drupal core in the past, and the original structures didn't do this, but this is a simple cosmetic change.

Seems redundant. Is there enough use of the former to justify using it over the latter? We should not leave in cruft just for backwards compatibility

To explain the use:

_THEMEENGINE_variables and _THEMEENGINE_variables_HOOK are for the exclusive use of the theme engine; phptemplate uses these for page, node, comment, etc. It uses the former for the universal zebra striping.

THEMEENGINE_variables and the associated _HOOK are for template.php, designed for shared code. In the same way that we prefer phptemplate_HOOK functions to make it easier, we will still prefer that for _variables.

THEMENAME_variables are then added for themes that don't have a theme engine (so even chameleon could make use of this stuff if it wanted to) and if one sub-theme wants to handle the variables slightly differently than the master theme.

I believe they are all necessary.

merlinofchaos’s picture

dvessel: I'm generally in favor of the variables queue manipulator; if anyone else will volunteer an opinion...

Crell’s picture

@ #28 and #29, if the variable array is being passed by reference, then couldn't any function just junk anything that came before it with $variables = array() to reset it? That's an easy way for any level to cancel any defaults from lower levels.

merlinofchaos’s picture

Crell: It can, but that still doesn't prevent the code from being run, which is theoretically a performance problem.

merlinofchaos’s picture

chx and I just thought of a solution to the caching vs development problem that I wish we'd thought of a year or two ago.

devel.module should have a "don't cache mode" where it calls its clear cache function on hook_exit().

Developers turn that on and voila, no more caching problems (at the expense of a slower site). chx said he would work on this today.

merlinofchaos’s picture

FileSize
76.3 KB

New patch, updated to HEAD again.

1) renames theme_discover_template to drupal_discover_template
2) fixes chameleon (and a notice bug that I threw in)
3) adds documentation to @defgroup themables and function theme() -- additional documentation is needed in the hooks doc for the new hook_themes()
4) changes _phptemplate_variables() to phptemplate_engine_variables()

Be sure to clear your cache after applying, as the _engine_ name change will give you very uninteresting results if the cache isn't cleared.

As per Dries, we're not going to put in dvessel's suggestion this patch. dvessel: After this patch makes it in, open a new patch.

Remaining TODO:

Evaluate whether or not we want to adjust the timing of function_exists() in the various _variables calls. -- perhaps this can be a followup performance tuning patch?

Steven’s picture

Merlinofchaos: if so, couldn't we just use 'function' => 'theme' and have the theme system automatically append '_'. $key ? What use is there to have a theme function with a different name from the key?

As for variables vs variables_hook, I see your point. Thanks for explaining that.

merlinofchaos’s picture

Steven: While I like the theme_HOOK convention, it's not clear to me that enforcing it is really a completely good idea, just to reduce a little extra noise in hook_themes().

Let's take a couple of interesting examples that we might consider down the line. Just for the sake of argument, I'm not actually really suggesting these in particular, but it's easy for me to grab an analogy.

<?php
array(
'node_page' => array(
'function' => 'theme_node',
),
'node_story' => array(
'function' => 'theme_node',
),
);

Two different theme hooks, with the same default implementation.

The impact of this will likely be most quickly noticed in forms api theming functions, where fapi does its own special lookups; and with Views, which looks for a theme function specific to a view, then falls back to the generic function if it doesn't exist.

dvessel’s picture

@merlin, it is such a small addition but it's definitely a separate issue. I'll keep testing your patches and keep track. -still trying to _fully_ grasp the new system.

Frando’s picture

What about allowing both versions?
Iow if the 'function' key is unset, the processing function sets it to theme_KEY ...

e.g. if you had
'node_list' => array(
'arguments' => array('items' => NULL, 'title' => NULL),
),
the processing function would set the 'function' key to theme_node_list, as it's unset. One can, however, still set the function key to an arbitrary value, if one needs to.

merlinofchaos’s picture

Frando: At the moment it uses the presence of the function key to determine if it is implemented as a function or template. Simply leaving that out might be a little bit ambiguous, though I find your solution to be the closest one to workable.

Ok, so I'm willing to do this: If 'function' doesn't appear, and 'file' doesn't appear, it will assume you mean the default function (and it can make this assumption for whatever layer you're on, too). That's actually a little cleaner, and I'm ok with that, and it leaves the full power.

I do want to comment on something I was talking to chx about a little bit:

Drupal does a LOT with magical naming, and every time discussion like this comes up, people get very defensive over the magical naming schemes. In part because magical naming reduces a little bit of code. But we have to be careful with magical naming. Magical naming is actually harder for newbies; it is a slight barrier to entry. Remember, new Drupal developers won't know all of the magical names, and when assumptions are made invisibly, it isn't obvious what is going on.

When the names are specified fully, it is much more clear what is going on. A little bit easier for newbies, at the expense of making it a little harder for the experienced developer.

That said, I'm going to cogitate more on Frando's suggestion of if !file && !function (function = layer .'_'. hook);

merlinofchaos’s picture

chx and I were also talking; using the naming technique above (creating several theme hooks that all default to the same function), form api could grant itself a lot of power by registering a set of theming hooks for every form. Identifying and implementing these may significantly ease some of the barrier to form theming.

dmitrig01’s picture

FileSize
205.8 KB

add in theme_menu_local_tasks, otherwise tabs won;t show

dmitrig01’s picture

FileSize
207.62 KB

Above patch didn't work

dmitrig01’s picture

FileSize
153.76 KB

patches--

neither worked :)

merlinofchaos’s picture

FileSize
71.37 KB

New patch based upon comments from Frando: if 'function' and 'file' are not set, a default is created. If 'file' is set, but not 'function', it is assumed to be a template. It's possible we may prefer to explicitly describe a template, since the current method is kind of magic, but I'm not sure that doing so is an improvement.

This includes the theme_menu_local_tasks fix from dmitrig01.

merlinofchaos’s picture

FileSize
72.06 KB

Fixed: need to clear theme registry when modules added/removed.

dvessel’s picture

Status: Needs review » Needs work

Patch 11 doesn't have theme_menu_local_tasks(). It's registered but the function is nowhere to be found.

Of course, it gives errors for themes outside of garland.

dvessel’s picture

The update.php page gives WSOD. Tried clearing cache but nothing.

merlinofchaos’s picture

Status: Needs work » Needs review

Drupal core currently doesn't contain theme_menu_local_tasks. That's not the fault of the patch.

I had update.php working; will have to figure out what I broke.

merlinofchaos’s picture

FileSize
72.25 KB

Ok, the 'file'/'function' automation failed for the maintenance theme, which doesn't get processed normally since it's abbreviated.

This patch performs a little processing in drupal_maintenance_theme() to handle that.

Dries’s picture

1. API:

+    'help' => array(
+      'arguments' => array(),
+    ),
+    'node' => array(
+      'function' => 'theme_node',
+      'file' => 'theme.base.inc',
+      'path' => 'includes',
+      'arguments' => array('node' => NULL, 'teaser' => FALSE, 'page' => FALSE),
+    ),

Question: what is theme.base.inc? There is no such fIle, AFAIK. Should that be theme.inc instead? Can we keep this stuff in theme.inc for now? Thanks.

Why do we need both a file and a path? Can't we specify the file as 'includes/theme.inc' instead? Do we separate them because we need to add an extension? It's more explicit and therefore easier to grok. If not, we probably want to document how and when is this to be set. It's not 100% intuitive, and it would be great if it could be more intuitive.

2. Hook names are typically singular. Do we want to use _themes() or _theme()? I think _theme() might be more consistent and therefore more intuitive. Any particular reason you decided to make this plural?

3. The included documentation is much appreciated -- good job. A couple of small remarks:

Themes can implement their own version of theme hooks, either as functions
+ * or files.

You'll want to explain _why_ themes want to implement their own version of theme hooks. What happens when I implement a _themes hook in my theme? (I know the answer, but the reader of the documentation might not know this.) We might also want to explain what the difference is between 'functions or files'. Someone not familiar with Drupal's theme system, might not know how to weigh this choice. Might be worth to touch upon the advantages and disadvantages, or to provide a recommendation.

In general, the documentation is great. If you want to work on the documentation some more, I suggest to sprinkle some practical examples. It might be a bit dense/theoretical for newbies. Some practical examples will help them get up to speed with it faster, and is likely to be very intuitive. Not a requirement for this patch to get committed.

4. Locale

-  $language = $GLOBALS['locale'];
+  $language = isset($GLOBALS['locale']) ? $GLOBALS['locale'] : NULL;

AFAIK, the locale variable does no longer resist. The i18n patch should have replace it by a global $language variable ($language->language).

5. In the themes/engines, the $existing parameter of _themes() is not documented. It took me a while to figure out what it was about.

6. The drupal_maintenance_theme() stuff is a bit hackish, but I can't come up with something nicer. Maybe some of that can always be loaded, and be available in all situations. Don't know.

7. The template_files functionality did not seem to be properly documented.

8. Add a CHANGELOG.txt entry. ;-)

I think this patch is pretty much ready to be committed. Not all of my remarks need to be taken care of, although it would be great. It's probably the kind of questions/confusion that other people experience. Anything we can do to make this slightly easier, will make this patch shine even more.

Let's do one more version of this patch, and get this committed. Thanks Earl.

merlinofchaos’s picture

Question: what is theme.base.inc? There is no such fIle, AFAIK. Should that be theme.inc instead? Can we keep this stuff in theme.inc for now? Thanks.

Err. theme.base.inc fell out of the patch. It used to be in it. Or maybe I screwed up and it's never actually been IN the patch, I just thought it was. Anyway, it exists as an important demonstration of that power, and it 'sets aside' 3 theme functions that hardly ever need to be loaded.

Why do we need both a file and a path?

We only need the 'path' here because in order to make the UI nice, it tries to figure out the path based upon context. However, 'includes' is kind of a special case, so it is given the 'path' directly. If we didn't, under the current layout it would try to find it in the system.module directory, where it doesn't exist. In general, 'path' won't be used except in very slim special cases like this one, where system.module is acting on behalf of the base system.

We also don't want to include the path directly; that makes it difficult on the module/theme systems that have their path in a variable and then they'd use drupal_get_path a lot which would result in an uglier syntax.

Hook names are typically singular. Do we want to use _themes() or
_theme()? I think _theme() might be more consistent and therefore more
intuitive. Any particular reason you decided to make this plural?

It sounded better? I have no particular attachment to _themes() over _theme() except that to me, _theme() feels like it's going to be different. I'm trying to think of another hook that's like this one. I come up with hook_node_types(), which is plural, and hook_menu() which is not (but also wouldn't sound right plural).

Documentation work

...more documentation work can always be done. Will sprinkle more in as I have time; even if this patch goes in ahead of that I will try to continue to work on it.

AFAIK, the locale variable does no longer resist.

I'll have to compare this to the original patch. Keeping phptemplate.engine up with HEAD is quite a bit of work every time it gets updated; I may well have gotten the merge wrong, though I was trying to be careful.

In the themes/engines, the $existing parameter of _themes() is not documented.

Sorry, documentation for hook_themes() lives in contrib so there isn't an obvious place to put documentation for the patch.

The drupal_maintenance_theme() stuff is a bit hackish, but I can't come up with something nicer.

I think it's a bit hackish to start with; I'm considering a following patch to try and help out the maintenance theme a bit by making it pluggable via a .profile, which might clean it up a little. It got uglier after the code to automatically figure out theme_HOOK function names, since it's using an abbreviated (no database, no hooks) model that can't easily look everything up.

The template_files functionality did not seem to be properly documented.

Hmm. Need to figure out exactly where to document this. Will think on it.

Let's do one more version of this patch, and get this committed. Thanks Earl.

Excellent! I want to here more discussion on hook_theme() vs hook_themes(); I lean toward leaving it hook_themes() but not enough to try and press the too much if anyone is opposed.

dvessel’s picture

hook_theme() sounds right. It's pointing to a single entity.. the theme itself. Same with the menu system, hook_menu() it's the single menu system even though it contains many parts, it's pointing to itself. Looked up hook_node_types(), did you mean hook_node_type() since that's what's commonly used. The _types() are used for working with multiple types. I'm not a mod dev so take it for whatever it's worth.

I beat on it pretty bad... Nothing broke.

Dries’s picture

I don't have a strong preference for _theme() vs _themes() but I'm leaning towards _theme(). As mentioned, it feels more consistent (i.e. _menu(), _link(), etc).

The only thing I feel strong about is theme.base.inc -- I think it should be folded back into theme.inc, at least for now. Maybe I don't fully understand the 'power' of this, but I think there is room for some simplification here.

Other than that, I'm OK to commit the next version of this patch 'as is', and to add incremental improvements later on.

Bonus points for improving the performance of theme().

Maybe other people have something to add, I don't know. I'll send an e-mail to the development mailing list.

merlinofchaos’s picture

The only thing I feel strong about is theme.base.inc -- I think it should be folded back into theme.inc, at least for now. Maybe I don't fully understand the 'power' of this, but I think there is room for some simplification here.

This can be done, of course, and will be quite easy, but the reason for it is this: rasmus pointed out that loading code accounts for > 50% of Drupal's runtime when he showed his top level performance runs; my own performancing showed this as well.

One of the side-effect intentions of this patch is that core should move toward being able to load code selectively, reducing its overall footprint. One of the places that it seemed very, very obvious to start with was to take 3 theme functions that have a 99.99% probability of never being called (theme_page and theme_node are almost always overridden at the theme layer) and set them aside so that they aren't taking up codespace. It's a small step, but that's what baby steps are about.

I'm perfectly ok with making that a second patch that does a more thorough job of setting theming functions aside, but felt it was a good idea to have one in here as an example of how to go about doing that.

jlambert’s picture

I agree. The performance implications of this patch alone make it a no brainer.

+1 +props

Dries’s picture

jlambert: did you actually benchmark this patch? It actually slows things down.

merlin: yes, let's keep that for a separate patch. There is more we can do in this area. Thanks.

ac’s picture

subscribing

adrian’s picture

I've looked at the patch, and I like the approach taken.

It was the approach we originally took with the templates patch before, but there are a couple of differences, which
lead to better performance than the original patch.

Firstly, there's no theme_link anymore, whereas previously theme("link") was called hundreds of times, on every page, slowing the system down considerably.

Secondly, the approach to converting to templates is far more pragmatic, in that some heavily used theme functions remain, causing a fair speedup over the all templates approach.

I believe this patch should go in.

There are a couple of things we could look at afterwards that might help speed things up as well.

Primarily we wrote a script which turned all the theme calls into the format theme("thing", array('param' => $value)).
This made the mapping step unnecessary , and removed the requirements to map the theme arguments to an associative array
of defaults. Doing this could also greatly reduce the size of the registry required, and lower memory usage.

merlinofchaos’s picture

adrian: It would take a fairly good benchmark to show me that changing theme to work that way is really the right way to go. It really adds a ton of extra code to the theme() calls. If PHP's array syntax were a little more compact, perhaps, but I don't like the syntax of passing the keyed array into the theme function.

merlinofchaos’s picture

Oh, and it makes it harder to document what a theme function's parameters actually are. This method actually improves documentation, since there's a nice dump from hook_theme(s) to get one started. It just needs descriptions.

Crell’s picture

Trying to close whatever i tag went ape...

While there are plenty of advantages to jQuery-style config parameters as adrian suggests, I believe from the l() revision patch it was shown that there was a small performance hit for them. They're also not as self-documenting. As merlin said, the benchmarks would have to be pretty conclusive to make that sort of change. Even if we did, that would be a separate patch, IMO.

Dries’s picture

Yep, let's re-roll this patch with some of the latest suggestions and commit it to CVS HEAD.

merlinofchaos’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
68.3 KB

Ok, final big patch! Updated for HEAD. Changes hook_themes to hook_theme, adds a little more documentation and a Changelog.txt entry. Reduced a little piece of code. This includes a NOTICE fix in chameleon (which in turn is also an update for language/locale that wasn't in the language/locale) patch. Possibly that should be a separate patch, but it needs to go in anyway and it was needed to make chameleon work right, so it's just in.

Returned theme_page, theme_block and theme_node to theme.inc.

As per Dries, this patch should now be RTBC. Anybody want to give it a quick spin to make sure I dind't break anything with the last wave of changes? (I cleared cache and clicked through a bunch of pages and didn't see any new breakage).

m3avrck’s picture

Typo in changelog...

+- Added the theme registry:
+    * Modules must implement hook_themes() to register their theme hooks

hook_theme() right?

awesome work earl!

merlinofchaos’s picture

FileSize
68.3 KB

Fixes typo m3avrck spotted. Whoops! Thanks for the heads up.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to HEAD. Great job Earl! :)

AmrMostafa’s picture

Docs docs ;-)

merlinofchaos’s picture

Ok, hook_theme documentation was ready to go and I just committed it.

We will need some fairly extensive documentation in the handbook itself. I may need to talk to a few members of the documentation team to figure out the best way to go about it.

Here's a fresh documentation issue: http://drupal.org/node/134258

Anonymous’s picture

Status: Fixed » Closed (fixed)
AmrMostafa’s picture

Status: Closed (fixed) » Needs review
FileSize
772 bytes

I was checking this new awesome improvements and found a small bug:

When the registry item has a function and file set, the path bit is not honored.
e.g., in a similar construct:

'test' => array(
  'file' => 'test.theme',
  'path' => 'test',
  'function' => 'theme_my_test',
  'arguments' => array(),
),

The theme system will try to include test.theme while it should include test/test.theme.

AmrMostafa’s picture

eh :-)

AmrMostafa’s picture

sorry for multiple submits..

AmrMostafa’s picture

Rerolled for HEAD.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

looks like a trivial fix. i'm not sure how to test it though. i think it is safe to RTBC

merlinofchaos’s picture

I agree with Moshe. Trivial fix, RTBC.

Dries’s picture

Committed to CVS HEAD. Thanks.

AmrMostafa’s picture

Status: Reviewed & tested by the community » Fixed
Dries’s picture

Status: Fixed » Closed (fixed)
mbria’s picture

subs.

Thanks for the patch.