Garland overrides overrides template with theme function

tracking…

Where are you trying to add your advanced_forum preprocessor? In a module? Theme or sub-theme? A little more information would help.

The phptemplate engine has worked this way since 4.7.x when it was first included in core. Any theme function defined by any module or by core can be overridden by theme_name_theme_function_name(). The special case of phptemplate_theme_function_name() is an override across all themes. It will always be called instead of any theme_name_theme_function_name() override.

This is because the phptemplate engine is by definition a drupal theme. It is literally defined as what the API calls a pure theme or a theme in the old sense like themes used to be in 4.6.x and earlier. The phptemplate engine is actually the only true theme being used. Garland and all other themes like it that use the engine can be thought of as "directives" that instruct the actual theme (phptemplate) on how to behave.

The issue here is that Garland implements the non-overridable version of the theme function theme_comment_wrapper(). However, even changing the function Garland uses to the theme specific version of garland_comment_wrapper() won't fix the problem. This is because Garland is the "theme" being used and any other override of the theme_comment_wrapper() function will not be used except for the special case phptemplate_comment_wrapper().

To sum up, your function is being seen as a theme function and as such can and will always be able to be overridden by any theme at the theme level. This is working as intended. I suggest that extreme caution be taken before any changes to the theme system be made against this long standing functionality. The ability of a theme to override a module or core definition of a theme function is where most of the entire power of the theme system resides. Without this basic functionality the theme system (and a themes ability to customize it's content) would be crippled.

I just had a look and the theme_comment_wrapper() function is not listed in the API for D6 however it is listed for D5. Is it possible that either a) it is missing from the D6 API docs or b) it was supposed to be removed from the comment module in D6 but was not? If it is supposed to still be in the comment module for D6 then maybe you could try implementing hook_comment to achieve what you need to do instead?

Sorry this is getting so long but I am sort of researching as I type.

In examining a module or themes ability to override preprocess functions I found this http://api.drupal.org/api/function/hook_theme/6. List item #8 is override preprocess functions. I am not too familiar with the preprocess functions but is it possible that the Garland theme has set this to TRUE thereby taking full control of all the variables? I apologize if this make my entire theme discussion above completely irrelevant but I leave it anyway in case it's not.

Dude, what you said is true about theme functions. But Michelle is talking about the preprocess function not the theme function.

But I do see the problem now.

Garland is overriding core's comment-wrapper.tpl.php with a theme FUNCTION. Bad garland! And theme functions don't have $vars, thus, no preprocess functions are run.

This is a bug in Garland. Patch coming shortly…

Actually, looking at what Garland is doing (adding a <h2>Comments</h2> except on forum pages) is what the core template should be doing. Zen does the same thing with its comment-wrapper.tpl.php.

This patch changes the core template for comment-wrapper.tpl.php and removes the problematic theme function from Garland.

Also, it looks like we missed one of the phptemplate-prefixed functions in #251758: Purify Garland's page.tpl.php. So this patch also changes phptemplate_comment_submitted to garland_comment_submitted.

Ok John I understand and I have not had time to look, but like I said there was a theme function in D5 called theme_comment_wrapper(). So Garland was overriding this function with phptemplate_comment_wrapper().

Maybe Garland just did not get updated when that theme function was removed in D6, however is not a theme allowed to override virtually any part of the theme system? Garland should be allowed to override comment-wrapper.tpl.php.

The reason I bring this up is that other themes may also attempt to override this, thereby making what Michelle is trying to do not work in those themes either but those themes would be "playing within the rules" of the theme system as nothing prevents them from doing this.

It seems to me that issues is not specifically in Garland but in this mysterious theme_comment_wrapper() function that was in D5. The function theme_comment_wrapper is registered in comment.module in D6 BUT no implementation of it is given in that module. Unless I am wrong, the fact that it is registered is what is allowing Garland to override it.

IMO shouldn't the patch in fact either do it properly, i.e with a template or with a preprocess function?

@gforce301: Yes, a theme is allowed to override what it wants. But it should be overriding comment-wrapper.tpl.php with its own comment-wrapper.tpl.php. (And when I create a back-port patch for D6, garland will be getting its own comment-wrapper.tpl.php.)

But there's no reason for Garland (given what it is actually doing) to override the template with a theme function. Because it turns off all preprocess functions.

@merlin: I think my issue creep might be confusing you. comment submitted is still a theme function in D6 and it shouldn't have a phptemplate_ prefix. I'm just tacking that on to the end of this patch. The rest of the patch is just modifying core's comment-wrapper.tpl.php to do what Garland wants to do; because I think that is a sane default.

The only thing debatable about this patch (IMO) is… should the change go into core's comment-wrapper.tpl.php or into Garland's comment-wrapper.tpl.php?

@John: I am not trying to hassle you, really. Just because there is no reason for Garland to override a template with a theme function IMO is not the actual issue. The fact that it is capable of overriding it is. Whatever Michele is attempting to accomplish will be lost in any theme that does override that function which originally only wrapped all the comments in a in a div as shown here http://api.drupal.org/api/function/theme_comment_wrapper/5.

The API for D6 says here http://api.drupal.org/api/group/themeable/6:

Each module should provide a default implementation for theme_hooks that it registers. This implementation may be either a function or a template...

Therefore I still contend that comment.module registers improperly a theme function @ line 183 in which it no longer implements and which then allows a theme to improperly override template_comment_wrapper.

Nevermind.

Just to reinforce: comment.module is NOT incorrect here.

Yes it is possible to override a template with a theme function.

Yes, doing so can have undesired effects and should NOT be done in a publicly available theme. That said, there are reasons to do it in a private theme, mostly for the sake of performance.

Sorry John, I did read the topic very quickly. You seem to be on top of it, but doesn't that mean the patch should be set to CNW since there's more?

blargh. No $node in comment-wrapper.tpl.php. Re-writing…

Actually, there is a $node variable. :-p

But looking at this again, I don't like the t() function in the template. In general, we should only have print and if statements in template files.

Ok. This patch adds a $title variable to comment-wrapper.tpl.php. And a forum_preprocess_comment_wrapper to remove it for forum nodes.

Hmm… I thought forum_theme() would need to register the forum_preprocess_comment_wrapper() preprocess function, but it looks like it is found automatically during theme registry building.

It's *very* hard to avoid having t() functions in templates, I've found, so I'm okay with those.

Oh, right. Good point.

Ok, so the patch in #15 uses a $title instead of a t('Comments'). And forum uses a preprocess function to blank it out.

The other option would be to create a comment-wrapper-forum.tpl.php template file. But that would have to go in the modules/comment/ directory. (Because “Suggestions only work when it is placed in the same directory as the base template.” http://drupal.org/node/190815#template-suggestions) But seeing as this is some functionality that forum module wants to do, it seems inappropriate to place its template file in comment module's stuff.

So I'm going to leave the patch as-is for now.

I apologize for trying to help around here anymore. I was not saying not to fix Garland. Obviously fix whatever you think needs to be fixed. I believe that you are all missing my point and I will try to state it one last time.

@Michelle: You can rightly use any mechanism that you choose to accomplish what you need. I was not trying to suggest you could not. This site is full of plenty of people who are still learning the very very basics of Drupal. The fact that a theme "publicly available" or not can override whatever it is you are trying to do seems to me that it might be a big warning to maybe look for a different mechanism to use. IF the functionality is so critical to the workings of advforum then it seems to me that you are just going to fill your issue queue with many people wondering why the features of your module do not work in this theme or that theme.

I really only try to help but I guess whenever I see the "BIG DRUPAL NAMES" show up where I thought about helping I will go elsewhere. At least the newbies appreciate my effort even if my understanding or a problem is not perfect.

@gforce301 Questioning is good, this is just a case of a theme going a bit far. I think the only reason it /is/ an issue is because its a core theme, and a widely used on at that.

If it breaks this sort of ability for a module to hook into the theme, then it discourages other themes from supporting it(many new theme developers use the popular ones as a base) and kill the ability for us to have cool modules like advanced forums.

There really isn't another way prior to 6. That's why the predecessor to advanced forums was a small group of functions to help developers do this in their themes. Now, its completely automated and builds on top of the theme allowing for people to build much richer forums.

I hope that helps you understand why Michelle et al. want to have this fixed.

PS - I hope you understand that the fact that "BIG DRUPAL NAMES" are paying attention is a good thing. They don't have time to fix everything and comments like merlin's above are meant to help thing stay in the right direction not scare you off.

@Michelle:
I did not argue with merlin. In fact I did not argue with anyone. I am arguing now. Please actually read what I was trying to say before you go accusing me of anything. If it's a bug fine it's a bug. If it's improper for a theme to do that then fine it's improper.

I may not be one of the Drupal gurus but I do have a very strong background in programming. I did not wander in here drooling on my keyboard wondering what and object was. From MY background we try to minimize the ability of one part of a system from totally overriding and canceling another part of a system.

MOST them designers will not understand the ability to trash your function with what is rightly in there ability to use i.e. THEME FUNCTIONS. They have been using them since 4.7 and will not understand this issue even to the degree that I don't understand it.

Has anyone ever even thought that since a theme function can override a preprocess function that maybe there could be (gasp) a flaw in that particular interaction within the new theme system? This seems different than a theme function overriding a template. I understand that I do not completely understand the preprocess functionality but please do not come at me like I know nothing.

On that note I will leave you here to do whatever you are going to do because obviously it is impossible to have an valuable insight or to use a system without completely understanding it. Oh, wait, um there are people who started with Drupal who by there own statements barely understood php. Now how is that possible?

Good day.

@gforce301

I would also like to point out that the only way for a module to alter the theme layer is via the preprocess functions. So Michelle won't be able to find an alternative way to do what she wants.

And, please don't get discouraged about commenting in issues. It was actually your last comment of #4 that made me take a look at garland's code. (I had previously believed it was a more general bug in the theme layer itself.)

I just had a look and the theme_comment_wrapper() function is not listed in the API for D6 however it is listed for D5.

So, belatedly, let me thank you for commenting! :-)

[Edit: Cross-posted. @gforce301 comment #21: Dude, chill! Michelle is awesome; she helps out in IRC all the time. I think she is just frustrated that you kept telling her to look for a different solution, when there is no other solution.]

gforce301: I had no intention of scaring you off this issue. I was simply stating my understanding of the system and my thought processes about why a particular strategy exists.

It is an unfortunate fact of the system that if a theme *function* overrides a theme *template* that all preprocessors go away, but it is also an immutable fact, and it's something we have to live with.

I do not believe that the existence of this possibility is a reason not to use hook_preprocess since the whole point of that hook is to allow modules to have a positive influence on small areas of theming. This is why I stated above that public themes should not be overriding templates with functions. It's okay for private themes to do this, but they will have to deal with consequences, such as advanced forum not working because of it.

I'm not quite sure that there is actually an argument here; there seem to be some unnecessarily ruffled feathers over this. You made an observation, and came to a different conclusion than what was intended, and we have given information as to why this conclusion isn't fitting with the generally perceived notions about Drupal. I don't see a particular need for ruffled feathers here, nor has anyone suggested or even, as near as I can tell, implied that anyone else should go away. For the most part, we've had a conversation that's pretty typical of Drupal issues, and these conversations are valuable. Certainly no one has questioned your skills as a developer nor a contributor, so there is no need to be defensive on that score.

If anything, I'd say you've gained valuable knowledge in a section of the theme system that might be a little murky at this point.

@merlin: Yes I have gained some knowledge about the new preprocess functionality.

I have been looking at the preprocess functions in the API for D6 and I still have some questions. The API states:

The following functions may be used to modify the $variables array. They are processed in this order when available...

It then goes on to list all the versions of the preprocess functions. template_preprocess, MODULE_preprocess_HOOK etc.. My question is in regards to the above quote. The API does not say that the various functions are overridden by any given version down the list. It implies that if one of the functions in the list is implemented then it will also be executed thereby giving a cumulative effect on the $variables array.

So my first question is: How and/or why is it be possible for a theme to override a preprocess function when the stated intent is for all of them to run? A themes version of the preprocess should run after the modules version, not replace the modules preprocess.

Second question: If I am not reading the API page wrong then, Does this not constitute a break in the intended functionality of the preprocess part of the theme system?

It's not that the them overrides the preprocess, its' that theme functions are not part of the preprocess system. Only theme templates.

Therefore, if core has a theme template, and a theme re-implements it as a function, all preprocess functions for that theme call will no longer be run. And yes, people often say "Well then theme functions should have preprocess functions run" but there are two reasons we decided not two: 1) theme functions primarily still exist for performance. They are a LOT faster than templates, and sometimes a particular theme function may be run as many as 100 times on a page (theme_placeholder is a good example) and that has a major impact on performance.

And theme functions don't actually receive a $variables array anyway, so a preprocess can't actually transmit its data to the function.

Yes, we are aware that this setup can cause a little confusion (such as what we have here) but the performance reasons are very compelling, and they are why theme functions should be discouraged in a public theme. At least, functions where the original was a template.

Ok I understand your explanation. I am afraid that I have another question though.

In the "old days" if in a theme I implemented say theme_node like MYTHEME_node a template was still loaded was it not? Did not the theme_node function only return what eventually became the $content variable passed to whichever node template was called?

So have we gained easier access / actual access to the $variables array at the module level (rather than the old _phptemplate_variables hook) only to sacrifice the flexibility? In the old system we could implement the theme function, the template, both or neither.

Now one is mutually exclusive of the other at a theme's level it would seem. I am not asking for change nor complaining. I am only trying to understand.

In the old days, the theme function actually is what implemented the template. So in a way you're right, but in a way you're not. Yes, you could have a template, if your theme function implemented it, or the theme engine implemented it. But in that system, only the theme/theme engine could be involved in the template. It was moving that bit of code more into core that gave more systems access to the preprocess stuff.

THe preprocess stuff really is entirely new; it's based a bit on _phptemplate_variables, but that is a phptemplate.engine specific piece of functionality, and it required that your theme function run through _phptemplate_callback() and did whatever it was it was going to do.

Now preprocessing happens *before* the theme even gets to it.

Now preprocessing happens *before* the theme even gets to it.

Not trying to poke fun but apparently not. If that were the case then themes would not be able to override templates with theme functions.

and in the old theme_node:

<?php
function theme_node($node, $teaser = FALSE, $page = FALSE) {
  if (!$node->status) {
    $output  = '<div class="node-unpublished">';
  }

  if (module_exists('taxonomy')) {
    $terms = taxonomy_link('taxonomy terms', $node);
  }

  if ($page == 0) {
    $output .= t('!title by !name', array('!title' => '<h2 class="title">'. check_plain($node->title) .'</h2>', '!name' => theme('username', $node)));
  }
  else {
    $output .= t('by !name', array('!name' => theme('username', $node)));
  }

  if (count($terms)) {
    $output .= ' <small>('. theme('links', $terms) .')</small><br />';
  }

  if ($teaser && $node->teaser) {
    $output .= $node->teaser;
  }
  else {
    $output .= $node->body;
  }

  if ($node->links) {
    $output .= '<div class="links">'. theme('links', $node->links) .'</div>';
  }

  if (!$node->status) {
    $output .= '</div>';
  }

  return $output;
}
?>

No callback is registered and no template implemented. That was the responsibility of the engine and the _variables hook.

I am not getting in your case though, from what I have learned due to this thread I like the new preprocess stuff a lot. I have used a custom modified _variables hook for 2 years that has allowed me to do something similar but not as complex as what the preprocessor stuff is doing in D6. Thanks for helping me understand it.

The decision to override an implementation of a given theme hook is made at registration time, actually, so that happens even earlier than the preprocess stuff, and this data is stored in the theme registry.

phptemplate_theme() is what's handling this for you and basically it does a search to see what templates you have and what theme functions you have and registers them accordingly. It doesn't care of the original implementation was a template or a theme function, so you can do either one however you like.

In the actual flow of the theme() call, there's basically a branch, very early, that checks to see if the implementation is a function or a template; if it's a function, it does almost nothing; it just does what it needs to do and calls the function. If it's a template, though, it goes through all of the preprocess logic (which is necessary for templates to be useful) and does that.

Perhaps the registration part is why the timing seems different.

BTW, with phptemplate, the old theme_node() existed but was never called.

What was actually called was phptemplate_node() -- look that one up. You might find it interesting to explore how phptemplate orginally did things. (The logic actually was kind of nasty).

That is very cool. I am sorry to have taken so much of your time. The new theme system is going to require me to make a lot of changes in how I do things and unfortunately in the environment I am in change is slow. I do not get the luxury of playing with the new toys until they are old toys and the new toys are out. LOL so much for the "fun" of a rapid development environment.

You know, your right. I completely forgot about the engine defining phptemplate_node(). It's been a long time since we developed our base packages and I have not revisited the engine code itself in about 2 years.

Early on we made a 'modification' (*cough* hack *cough*) to the engine itself to provide a layer of common templates between the engine and the running theme. This allowed us to have templates and a template.php file that would be common to all themes. We just keep them in a folder in themes called common.

So with the new theme system I have to learn even more than normal as some of the mechanisms I rely on were inventions of my own. Thanks again merlin.

I think this is where having templates and suggested templates separated could help out. It would allow "comment-wrapper.tpl.php" within the comment module folder and "comment-wrapper-forum.tpl.php" where the forum module exists. This makes the association a lot more clear.

John, in your patch, clearing out the title in a buried preprocessor seems a little awkward. It would be a little more difficult to know what's going on.

For 6, I think back porting should be done by defaulting to a template for Garland with a $node->type check for the title. Garland was never intended to be modified so I think it's safe to change in 6.

I'll be working on the other issue over the weekend. http://drupal.org/node/279573

Yummy! If #279573: Themes can't use node-story.tpl.php without node.tpl.php gets fixed, I would much prefer to go with a comment-wrapper-forum.tpl.php in the modules/forum/ directory! Assuming the same will work for modules as for themes…

I will wait on that issue to reroll this patch.

And, yes, the D6 backport should just be a new comment-wrapper.tpl.php for Garland. I've already got the patch ready.

I had originally wanted to have "Comments" be a heading on the main comment-wrapper.tpl.php and have forums remove that heading. But, since #279573: Themes can't use node-story.tpl.php without node.tpl.php is now postponed until we figure out how to do it, we need to tackle this issue in a more awkward way. :-\ This should have been fixed in D6 six months ago. Sorry, Michelle!

How about this simpler solution? We leave "Comments" as a heading as a Garland-only solution (for now). And we accomplish this by prepending the $content variable with the heading in garland_preprocess_comment_wrapper().

Here's the patches for D7 and D6.

Works as advertised for me. Simple fix.

The solution in #35 feels a bit like a hack to me. For the new d.o, we need to theme comment titles differently, for example. I'd like to see us work on this some more.

Hello folks, I've been out of this for a while but what's wrong with removing the theme function and simply added two templates for garland? "comment-wrapper.tpl.php" and "comment-wrapper-forum.tpl.php"?

I Agree with Dries, the patch is very awkward. Core already sets a suggestion for all node types in comment wrapper and the forum nodes will get picked on its own.

Michelle, let Garland leverage what's already there in core. The suggestion for comment wrappers based on the node type will do the rest. One template with the titles and one without and the stuff advanced forums needs to do should work itself out.

You could manually add a suggested template from advanced forum that's specific to it and provide the default template inside your module. This way, Garland or anything other theme will not interfere with it.

This would be a tiny bit more work for themers but it might be worth it and could have other advantages. I could discuss that with you in the advanced forums queue.

I'm sorry John. I wasn't paying close attention and now I see your solution to be okay. It is as you mention, specific to Garland and it works around the problem of duplicating another template. Until we fix that, this is fine by me.

But the patch for head removes the wrong function.

But the patch for head removes the wrong function.

Wow! I really pasted that code into the wrong place. :-p

Updated patch for HEAD is attached.

This applies and works as advertised. Seems to make the most sense to remove an override of a theme function (seems like core should avoid overriding itself and just fix the problem). This also seems like the best solution, until #279573: Themes can't use node-story.tpl.php without node.tpl.php is in.

Ice cream?

Got pointed to this. In addition to re-rolling so testing bot quits complaining, we should be checking against any node type that can be posted to forums, not just 'forum' node types. This should be anything that's attached to variable_get('forum_vocab') or whatever it's called.

/me didn't realize this was back to cnw. Will re-roll shortly.

Grrr… testbot.

Can we please solve this problem in unified manner? There are several intiatives what try to fix <h2>Comments</h2>, see: #517850: Update and polish default comment-wrapper.tpl.php and associated css, #523950: Update comment rendering, #279573: Themes can't use node-story.tpl.php without node.tpl.php. Could we have a single point of entry for this debate and mark others as dupes?

Apparently, some other commit modified comment-wrapper.tpl in a way that would break the tests, but garland’s fubar garland_preprocess_comment_wrapper() was shielding that change. So this new patch fixes that broken test; no other code changes. The test change is trivial; I just removed two closing >s since comment-wrapper now has a class="[stuff]" after its id="comments".

@kika: Those 3 issues are all distinct. This one in particular is aimed at fixing a bug in D6's garland. The other two are D7-only issues that are changing the markup in the tpl and changing the comment variables, respectively. We could postpone #517850: Update and polish default comment-wrapper.tpl.php and associated css until #523950: Update comment rendering is committed, but if the comment rendering doesn't go in, the HTML markup still needs tweaking.

Taking the liberty of setting back to RTBC as the testbot will just smack it down if I'm wrong and the test doesn't work as it does on my system.

Actually, it turns out #523950: Update comment rendering has to extend to comment-wrapper, and actually fixes this and #517850: Update and polish default comment-wrapper.tpl.php and associated css.
It removes the custom theming in garland altogether, and uses the following default markup in comment/comment-wrapper.tpl.php:

<div id="comments" class="<?php print $classes; ?>">
  <?php if ($node->type != 'forum'): ?>
    <h2 class="comments"><?php print t('Comments'); ?></h2>
  <?php endif; ?>

  <?php print render($content['comments']); ?>

  <?php if ($content['comment_form']): ?>
    <h2 class="title"><?php print t('Post new comment'); ?></h2>
    <div>
      <?php print render($content['comment_form']); ?>
    </div>
  <?php endif; ?>
</div>

@yched, that may be. But until something is committed to D7, we can't fix this in D6. And this is the fix we need for D6; obviously we can't use #523950: Update comment rendering in D6.

[Edit: I like #523950: Update comment rendering, btw. But this issue has been open too long.]

Obviously #523950: Update comment rendering will not fix this thread for D6. But it is clearly the way forward for D7, and will most probably be committed pretty soon.

The patch here is the fix for D6. We can commit it to D7, but it will only force #523950: Update comment rendering to reroll and erase the changes anyway, so I'd advise we wait a day or two. that issue has been open for a whole year...

#523950: Update comment rendering was just committed, so back to D6 then. :-)

The patch in #35 is still good to go for D6.

Committed to Drupal 6, thank you!