This is a bug report/feature request I've been meaning to submit for a while.

If you go take a gander at http://drupal.org/node/223430, you’ll notice two “notes” about why you should never use engineName_preprocess or engineName_preprocess_hook as the name for your preprocess functions.

A few examples makes things clearer. Imagine you have this basetheme->subtheme hierarchy:
* basetheme
* subtheme1 (basetheme is its parent)
* subtheme2 (subtheme1 is its parent)

  1. If the base theme has a phptemplate_preprocess function, the theme registry loads all 3 theme’s template.php files and has no way to know which theme owns the phptemplate_ prefixed preprocess function. Thus, that phptemplate_preprocess function gets registered by ALL 3 of those themes and gets run three times instead of once. ouch!
  2. Worse case: if the base theme defines phptemplate_preprocess() and then a un-observant subtheme1 creates the exact same function in its template.php, the unlucky themer will get a Fatal PHP error and possible WSOD (White screen of Death)! yay.
  3. Here's another awful consequence: Let's say that both the basetheme and the subtheme1 want to override theme_links. If basetheme defines a basetheme_links() and subtheme1 defines a phptemplate_links(), the one defined by subtheme1 should be used, but themeEngineName_links is assumed to be less specific than themeName_links, so subtheme1's phptemplate_links() override is ignored in deference to the basetheme_links() override.

The original rationale for allowing themeEngineName prefixes was “This makes it easier to move around code between themes and post snippets on Drupal.org.” But we already require themers to copy theme functions from modules and rename them from theme_function() to themeName_function(), so the copy-and-rename-function meme is already out there. Plus, copy-and-rename will help themers who are new to programming concepts understand that you can't give 2 functions the same name.

“Portability of function names” is a crutch that we shouldn't be giving to beginning themers. It just turns them into cripples. We should teach them a little PHP the right way so we don't confuse them more later.

Solution: remove themeEngineName from the list of allowed preprocessor function and theme function prefixes. Note, themeEngineName_engine prefixes are still fine since they apply to theme engines and not to themes.

Files: 
CommentFileSizeAuthor
#34 die-phptemplate-prefix-die-422116-33.patch954 bytesbarraponto
#23 die-phptemplate-prefix-die-422116-23.patch6.01 KBJohnAlbin
Passed: 11805 passes, 0 fails, 0 exceptions
[ View ]
#16 die-phptemplate-prefix-die-422116-16.patch5.3 KBIsland Usurper
Passed: 11805 passes, 0 fails, 0 exceptions
[ View ]
#14 die-phptemplate-prefix-die-422116-14.patch5.99 KBJohnAlbin
Failed: Failed to apply patch.
[ View ]
#8 die-phptemplate-prefix-die-422116-8.patch3.56 KBJohnAlbin
Failed: Failed to apply patch.
[ View ]
#7 die-phptemplate-prefix-die-422116-7.patch2.1 KBJohnAlbin
Failed: 11226 passes, 0 fails, 5 exceptions
[ View ]

Comments

seanr’s picture

I like the idea on paper, but how do you propose getting around the function name collision issue? You can't have to identically named functions.

mfer’s picture

Issue tags:+Needs design review

But, all my blog posts that say use 'phptemplate_something' will be no good!!!

Tagging as needs design review. I'm fine either way but I imagine some others might want to chime in. This is removing a 'feature' after all.

mfer’s picture

@seanr his proposal removes the possibility of name collisions.

quicksketch’s picture

Thanks for raising this issue JohnAlbin! I'm 100% behind it, our use of phptemplate_* in themes has always broken our own rules about namespacing.

Though I wonder, wouldn't it be better to removed "themeEngineName_engine_*" and keep "themeEngineName_*" for the theme engine? Although then we wouldn't be forcing users to stop using phptemplate_ prefixes, for better or worse.

seanr’s picture

Ah, OK, so he's just saying to always use themename_function instead of the current sometimes enginename_function sometimes themename_function paradigm. I'm down with that. It'll certainly mean more consistency in documentation and suchlike.

JohnAlbin’s picture

@quicksketch

Hmm… Shortening the theme engine's prefix from themeEngineName_engine_ to just themeEngineName_ makes sense.

But we need to make sure that we can prevent a theme from registering the themeEngineName_ functions. That shouldmight… be possible. We'll have to see once we get into the actual patch. But, as long as we can prevent themes from using that prefix, I'm fine with changing the engine's preferred prefix.

JohnAlbin’s picture

Status:Active» Needs review
StatusFileSize
new2.1 KB
Failed: 11226 passes, 0 fails, 5 exceptions
[ View ]

But we need to make sure that we can prevent a theme from registering the themeEngineName_ functions.

Hmm… ok, so the existing code just does a if (function_exists($prefix . '_preprocess')) to check if the function exists. This is done after the theme engine and theme code is loaded. So there really is no way to rename themeEngineName_engine_ to just themeEngineName_ while preventing a theme from registering the later. We'll have to keep themeEngineName_engine_ for now. :-p

Here's the patch. Turns out its really simple. :-) But I expect some tests may explode. testbot? review please!

JohnAlbin’s picture

Assigned:Unassigned» JohnAlbin
StatusFileSize
new3.56 KB
Failed: Failed to apply patch.
[ View ]

BAH! Who left that phptemplate_ prefixed function in Garland!?! >:-(

Hmm… actually, that's a theme function and not preprocess function. I think the last patch just dealt with preprocess functions because with the patch installed, I'm still seeing that phptemplate_ theme function getting registered.

Ok, found it. phptemplate_theme() had to be modified to not allow phptemplate as a legit prefix for theme functions.

New patch.

Island Usurper’s picture

The documentation for theme() needs to be tweaked for THEME_preprocess(). It still refers to ENGINE_preprocess() which should shortly not be in the documentation any more. I'm not sure what it should be changed to, especially when most themes will benefit from phptemplate_preprocess() anyway.

Thanks for getting this cleared up in core. I'd been a little confused lately with the different things I had read in documentation and code.

Island Usurper’s picture

Status:Needs review» Needs work

Oh, um. Needs work, I guess. :P

sun’s picture

If we really want to remove this, we have to introduce an equivalent for this one phptemplate_* use-case in the same patch:

I have x Drupal sites with y modules. Because y modules implement awkward output, I have y common (default) theme overrides in x sites.

If we remove phptemplate_* without replacement, then I need to maintain x * y theme function overrides.

Meaning: Theme developers currently are able to use the phptemplate*-prefix to build a repository of common/custom theme overrides that can be quickly applied to any Drupal site.

quicksketch’s picture

sun, If you're really concerned about building a "library" of overrides, you can still use hook_theme_registry_alter() and point theme functions to those provided by a universal module shared by all sites. Or better yet, you can also create a base theme shared by all sites, which has the added benefit of individual themes being able to override the parent theme functions.

On the other hand, if the sites are all separately maintained, it doesn't seem like renaming the functions would be a terrible burden, since the code in each theme will be maintained separately anyway. We certainly don't want to provide a new equivalent shared prefix, as the common prefix between different themes makes loading of multiple themes at once impossible.

Crell’s picture

If you want to have a common library of overrides for use in lots of themes, put them into a base theme. They'll still work and you can single-source them, and you can override them again for your site-specific subtheme. Zen D6 did that in early versions to work around bugs in core until they got fixed, and it worked fine.

Subthemes themselves are a replacement for phptemplate_*, if you use them properly. I am +1 on removing phptemplate_*.

JohnAlbin’s picture

Status:Needs work» Needs review
StatusFileSize
new5.99 KB
Failed: Failed to apply patch.
[ View ]

Good catch, Lyle!

New patch also fixes the @defgroup themeable comments.

Status:Needs review» Needs work

The last submitted patch failed testing.

Island Usurper’s picture

Status:Needs work» Needs review
StatusFileSize
new5.3 KB
Passed: 11805 passes, 0 fails, 0 exceptions
[ View ]

Re-roll.

webchick’s picture

Issue tags:+Needs themer review

Tagging.

JohnAlbin’s picture

Issue tags:+Quick fix

The new patch works identically to my old, no-longer-working patch. I'm assuming webchick wants more Themer eyeballs on it, but the patch looks RTBC to me.

sun’s picture

Status:Needs review» Needs work
             elseif ($type == 'theme_engine' || $type == 'base_theme_engine') {
               // Theme engines get an extra set that come before the normally named variable processors.
               $prefixes[] = $name . '_engine';
-              // The theme engine also registers on behalf of the theme. The theme or engine name can be used.
-              $prefixes[] = $name;
-              $prefixes[] = $theme;
+              // The theme engine also registers on behalf of the theme. The theme name should be used.
+              $prefixes[] = $theme;

That comment sounds a bit strange to me now. Perhaps simply drop the also as well as the second sentence.

emmajane’s picture

+1

I've read the comments and the patch itself. The patch itself seems benign. It will break a few themes, so "we" need to make sure there is good upgrade documentation for D6 -> D7. The fix is easy though (search and replace from phptemplate to themeName) so I support the patch.

Nick Lewis’s picture

+1
Find/replace is easy enough. My only worry is phptemplate_ functions from past installs silently failing, and curses being shouted around the world as people wonder why drupal isn't noticing phptemplate_foo like they expect. I think a warning message for version 7 would be considerate when drupal detects the prefix, but I don't think its a show stopper. As for the word "also"...

webchick’s picture

Ok, cool. That's enough +1s from enough themer-types that I'm comfortable committing this once it's RTBC again.

JohnAlbin’s picture

Status:Needs work» Needs review
StatusFileSize
new6.01 KB
Passed: 11805 passes, 0 fails, 0 exceptions
[ View ]

Updated comment now reads:

The theme engine registers on behalf of the theme using the theme's name.

Sound good?

Dries’s picture

Looks good to me, but I'll have webchick drive this one home!

sun’s picture

Status:Needs review» Reviewed & tested by the community
webchick’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs Documentation

Committed to HEAD! Yay! :D

Docs please!

emmajane’s picture

Status:Needs work» Fixed

Documentation has been added to the D6.x -> D7.x theme upgrade page. Please see: http://drupal.org/node/254940#function-names-phptemplate and add additional information if you feel more is needed.

emmajane’s picture

Updating the tags as well.
Removing: Needs design review, Needs Documentation, needs themer review, Quick fix,

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Berdir’s picture

Status:Closed (fixed)» Active

Hm...

Trying to figure out something...

I have a rather uncommon use case for theme patterns (which might be bad, then I'm looking for better suggestions!).

Basically, in Privatemsg, I have a table with dynamic number of columns, and each of these columns can be displayed/themed or not. (We have our own query builder in the D6 version so the whole thing is very dynamic).

In D6, I have used theme patterns to be able to theme every field in a separate theme function and easily allow other modules/themes to override single columns, both the header definition and the actualy fields.

The problem in D6 already was that modules can't provide theme suggestions based on theme patterns. That's why I used phptemplate_ as the prefix. See also http://blog.worldempire.ch/api/group/theming/1 for a description of what I'm doing, and http://blog.worldempire.ch/api/drupal/privatemsg.theme.inc/1/source for the currently defined theme functions.

Now, this patch removed the phptemplate_ prefix which makes it impossible for modules to provide theme suggestions, since it only looks for the theme anymore.

Am I just doing it wrong(tm) (If yes, then please explain me how to do it correctly while having a similiar flexibility) or how am I supposed to adapt that system to D7?

Berdir’s picture

Status:Active» Closed (fixed)

Ok, I was able to "solve" the issue by adding the following code to my hook_theme() implementation:

<?php
 
// Include the theme file to load the theme suggestions.
 
module_load_include('inc', 'privatemsg', 'privatemsg.theme');
 
$templates += drupal_find_theme_functions($templates, array('theme'));
  return
$templates;
?>

This looks for theme suggestions prefixed with theme_, which is what I wanted in the first place. Please tell me if that's not a good idea :)

barraponto’s picture

Project:Drupal core» Documentation
Version:7.x-dev»
Component:theme system» Correction/Clarification
Status:Closed (fixed)» Needs review
Issue tags:+docs theming

phptemplate_ prefix is still suggested in hook_theme (as a default preprocess)? i guess it isn't anymore, so i am removing it.

barraponto’s picture

StatusFileSize
new954 bytes

forgot to add the patch