Right now most node templates start with something like:

<div class="node<?php if ($sticky) { print " sticky"; } ?><?php if (!$status) { print " node-unpublished"; } ?>">

This is ugly. It's over 100 characters long, it contains logic, and it's duplicate code since we have it 4 times in core (and a gazillion times in contrib).

This patch proposes to move the logic to template_preprocess_node() which generates a $node_classes variable, resulting in much prettier node templates:

<div class="<?php print $node_classes ?>">

In addition to the 'sticky' and 'node-unpublished' classes, I also added 'node-teaser' since I've often seen uses for such a flag.

A summary of the previous comments is available in comment #80.

Files: 
CommentFileSizeAuthor
#128 hook-classes-306358-128.patch48.84 KBJohnAlbin
Passed: 11506 passes, 0 fails, 0 exceptions View
#122 hook-classes-5-27a.patch48.04 KBdvessel
Passed: 11503 passes, 0 fails, 0 exceptions View
#110 hook-classes-306358-110.patch48.69 KBJohnAlbin
Failed: Failed to apply patch. View
#105 hook-classes-306358-105.patch49.03 KBJohnAlbin
Failed: Failed to apply patch. View
#95 hook-classes-306358-93.b.patch49.65 KBdvessel
Failed: Failed to apply patch. View
#93 hook-classes-306358-93.patch47.7 KBJohnAlbin
Failed: Failed to apply patch. View
#84 hook-classes-306358-84.patch42.46 KBJohnAlbin
Failed: Failed to apply patch. View
#78 hook-classes-306358-78.patch40.72 KBJohnAlbin
Failed: Failed to apply patch. View
#77 hook-classes-306358-77.patch34.59 KBJohnAlbin
Failed: Failed to apply patch. View
#55 hook_classes.i.patch40.08 KBdvessel
Failed: Failed to apply patch. View
#54 hook_classes.h.patch40.06 KBdvessel
Failed: Failed to apply patch. View
#53 hook_classes.g.patch40.07 KBdvessel
Failed: Failed to apply patch. View
#51 hook_classes.f.patch40.27 KBdvessel
Failed: Failed to apply patch. View
#48 hook_classes.e.patch40.13 KBdvessel
Failed: Failed to apply patch. View
#38 hook_classes.d.patch32.96 KBdvessel
Failed: Failed to apply patch. View
#34 hook_classes.c.patch32.19 KBdvessel
Failed: Failed to apply patch. View
#26 hook_classes.b.patch29.44 KBdvessel
Failed: Failed to apply patch. View
#25 hook_classes.patch27.39 KBdvessel
Failed: Failed to apply patch. View
#16 node_classes.patch4.04 KBfloretan
Failed: Failed to apply patch. View
node_classes.patch4.07 KBfloretan
Failed: Failed to apply patch. View

Comments

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Ah, thats much better.

NikLP’s picture

HELL YES.

add1sun’s picture

Yeah, this would be quite nice. I view it as akin to the $body_classes in page.tpl.php. Give people the basics to work with out of the gate.

yoroy’s picture

Yes please. This is built into the Zen theme which we use all the time, and is something that saves us lots of time, everytime.

mortendk’s picture

oooh yeah the king approves ;)

NikLP’s picture

"+1. This is built into the Zen theme which we use all the time, and is something that saves us lots of time." ;P

webchick’s picture

Status: Reviewed & tested by the community » Needs work

I asked in #drupal for some themers to chime in on this patch. The discussion behind the +1s here is:

* This is a feature of Zen which everyone uses and finds valuable.
* This is stuff that people need to do on every single theme they create.
* It saves boat-loads of code in every theme doing all that nasty inline logic.

So sounds like a no-brainer to me, but add1sun pointed out that the PHPDoc should be more descriptive on what these classes actually can be, which I think is a good idea. So could we itemize them in a list so people don't have to dig into theme.inc to figure it out?

add1sun’s picture

My only nitpick is the help text for the variable:

"$body_classes: A set of CSS classes for the tag wrapping the node.
+ * This contains flags indicating various node properties, like
+ * published status or stickiness.
"

might be nicer to just literally list what it will assign.

JohnAlbin’s picture

I agree with Addison. The only way someone could find out which node classes might be available with this patch would be to dig into the code of theme.inc.

So we must list them all in the node.tpl.php.

dvessel’s picture

This is definitely nice but I think this can go further. Instead of just focusing on nodes, how about supplying classes to everything passed into templates. This is similar to how I normally set up classes and even id's in my themes.

Within template_preprocess, the hook can be pushed as the class. Then any specific template_preprocess_HOOK can add its own specialized classes and right before the template is rendered, it can be cleaned and flattened. Any hook specific preprocess function just needs to concern itself with loading classes so it can be passed along until it's rendered.

The variable used to output the class should be the same across all templates. I normally use $class_attr.

If we standardized on something like this, then even module devs can consistently push classes if they need to. Having a single way of applying this everywhere would be a lot more useful I think.

JohnAlbin’s picture

Title: Add $node_classes to node templates » Add $classes to hook templates

@dvessel. Sounds really good to me. But I've got 2 comments.

I'm not too crazy about $class_attr. Core currently has a $body_classes variable, so maybe the more generalized variable should simply be called $classes or $wrapper_classes? I prefer $classes.

Then any specific template_preprocess_HOOK can add its own specialized classes and right before the template is rendered, it can be cleaned and flattened.

So, for example on the node hook, if $classes is an array that starts out as $classes = array('node');. Then any module can just implement MODULE_preprocess_node($vars) and add $vars['classes'][] = 'my-module-class';. I like that workflow so far…

But from what I can see, the problem with this approach is it breaks down in the base theme and sub-theme phase. Because how do we determine who is going to flatten the array to a string? The $classes = implode(' ', $classes); has to go somewhere. I really do not want to see that PHP snippet in the tpl.php template file. But if it has to go in the preprocess function, then the base theme will be converting it to string and the sub-theme won't have an array to work with. Telling sub-themes that they need to do $vars['classes'] .= ' my-subtheme-class'; instead of the usual will lead to confusion. geh, and I just realized, we can't expect every theme to be required to render those variables. Then they would be forced to implement all kinds of preprocess hooks that they aren't interested in.

So we are forced to either render the array to string before the preprocess function or in the template file. Both are icky.

Perhaps we need a postprocess_hook function? or a prerender_hook function? … Ok, I just saw #134478: Refactor page (and node) rendering which looks like a better (and more ambitious) approach then adding a new layer of theme functions.

Since I oppose more PHP in tpl files since, I think until #134478 lands the best solution would be to pass both an array and a rendered string. And if a preprocess wants to modify the array they need to re-render the string.

i.e.

$variables['classes'][] = 'my-new-class';
$variables[$hook . '_classes'] = implode(' ', $variables['classes']);

Thoughts?

dvessel’s picture

John, I haven't looked closely at how the node is being restructured but if it's very specific to nodes I think this issue should be kept out of it.

I never expected the flattening to happen in any specific theme or templates. It should happen automatically right from the theme function right around where the suggestions are discovered. If that doesn't sound good, a post preprocess as you mentioned can work too which would give a little more control through the theme registry. I'm not sure which would be better.

The only thing a preprocess function would do is feed the array similar to how you would feed suggestions.

$variables['classes'][] = $hook . '-' . $variables['node']->type;

I mentioned $class_attr because I do the same thing with the html id attribute but that's taken as a variable ($id) and I wanted a common naming convention. Whatever makes sense here is fine by me.

Passing both the array and string to the template would be less of an issue since it can all be handled in the preprocess functions but I wouldn't object to having both. Personally, I'd rather not but some themers may want to throw a lot of junk right into their templates.

JohnAlbin’s picture

Ah! I see where I misunderstood your comment…

Then any specific template_preprocess_HOOK can add its own specialized classes and right before the template is rendered, it can be cleaned and flattened.

emphasis mine

I mis-read that as THEME/MODULE_preprocess_HOOK. :-p Ok. So it will be a string when it gets to the theme/module preprocess functions. I'm fine with that. And it will certainly keep the patch simple. We can leave the classes-as-array variable out of the patch.

It should be pretty easy to extend flobruit's patch. :-)

dvessel’s picture

If it's okay with everyone, I'll start extending on what flobruit started here.

There would be one side effect. Every template will have its own hook as its class. For the themers who hate superfluous markup in their templates, it could get annoying but on the bright side, with a bit of code in a preprocess function, it can be cleaned up from one place if they want to. A lot easier then hunting around to change anything. For example..

function mytheme_preprocess(&$vars, $hook) {
  $remove_classes = array('page', 'box', 'foo');
  foreach ($vars['classes'] as $i => $class) {
    if (in_array($class, $remove_classes) {
      unset($vars['classes'][$i];
    }
  }
}
alexanderpas’s picture

+1 to this feature ;)

floretan’s picture

Status: Needs work » Needs review
FileSize
4.04 KB
Failed: Failed to apply patch. View

Here's a re-rolled patch with a better documentation, as suggested in #8.

@dvessel: I'm not sure I understand what you mean by "Every template will have its own hook as its class" in #14.

dvessel’s picture

flobuit, what I mean is this.

In every template, the $class variable will always have it's own theming hook spit out as a class. Assuming this is used in each template:

<div class="<?php print $classes; ?>">...

Will output this for node.tpl.php:

<div class="node other-classes-added-through-preprocess">...

For page.tpl.php:

<div class="page other-classes-added-through-preprocess">...

block.tpl.php:

<div class="block other-classes-added-through-preprocess">...

In most cases this is fine but it's almost never used in page.tpl.php. It would just be a new consistent behavior that works across all templates.

I don't mean to take over your queue but you see any value in this?

floretan’s picture

@dvessel: thank you for the clarification. I like the idea of having a standard behavior, even if in the case of the page template having the "page" class on the body tag doesn't bring anything.

However, each template type (node, block, field, etc) has specific rules for generating the classes, so we might just have similar code for each of them and having the name of the template type as the first class can just be a convention.

webchick’s picture

Looks like what we're essentially talking about here is a tradeoff between N variables to remember:

$page_classes
$node_classes
$comment_classes
$block_classes
$box_classes
$X_classes (aggregator.tpl.php == $aggregator_classes)?

And one "magic" variable $classes which automatically brings in the correct classes for a given template. It seems like this approach is more consistent, and would result in less duplicated code.

But, if we go for the "magic" variable (which I like from the "only one thing to learn" aspect), how does a themer go about figuring out what classes are added to each one? Crack open theme.inc? :(

dvessel’s picture

Webchick, the docs embeded into each template. –What flobruit already started. List the possible classes and any explanations as necessary.

There's also FireBug or any other debugging tool and this can show up in devel themer since it's just another variable passing through preprocessors. I don't think it would be a problem.

@flobruit, I think we're on the same page. I'll get the patch up soon.

Just want to make clear, it's pretty common I think to package jQuery scripts into modules. That separation between the markup and what modules are allowed to control limits them. Having these classes standard will allow better targeting. We just might need to revisit the naming conventions on classes soon.

dvessel’s picture

Status: Needs review » Needs work

Just a warning.. This patch is going to hit a lot of files. If it's not up by late tonight, it'll be up late Monday.

JohnAlbin’s picture

I thought this was such a good idea, I implemented a $classes variable in Zen 6.x-1.0-beta3 over the weekend. I wanted to try it out in contrib before rolling a patch.

But I see Joon already started working on a patch. Joon, do you need help with the patch?

dvessel’s picture

Thanks John, but I have this covered. I want to make sure all the code in the preprocessors are documented in the templates. Not sure how we can split the work without confusion. There are a lot of things in there interconnected.

JohnAlbin’s picture

@Joon, I realize I completely forgot to document the $classes in Zen's node.tpl.php and page.tpl.php.

But, I did carefully document them for comment.tpl.php and block.tpl.php.

Feel free to copy and paste anything you need from HEAD: http://cvs.drupal.org/viewvc.py/drupal/contributions/themes/zen/zen/

$classes are built in zen_preprocess_block() inside template.php and in _zen_preprocess_comment() inside template.comment.inc.

:-) Looking forward to the patch.

dvessel’s picture

Assigned: floretan » dvessel
Status: Needs work » Needs review
FileSize
27.39 KB
Failed: Failed to apply patch. View

I was trying to hit every template but it would have been impossible to review. I'll do that in a follow up.

The changes only affect a few modules.. The hooks: page, node, block, comment, comment-folded and comment-wrapper.

I'm still not positive in how it should flatten the classes. It's done right in theme() and it converts that same 'class' variable from an array into a string. Any underscores and converted to dashes.

And thanks John, I already had some of the code set but it's similar enough. :)

dvessel’s picture

FileSize
29.44 KB
Failed: Failed to apply patch. View

Forgot one class. 'sticky' was changed to 'node-sticky'.

floretan’s picture

That looks great, I like this approach.

Some unrelated bit got inserted, and should be moved to a different issue:

- * - $display_order
- *   - COMMENT_ORDER_NEWEST_FIRST
- *   - COMMENT_ORDER_OLDEST_FIRST

Also, I'm not clear about why we need to add a $node parameter to theme_comment_folded(). We change the definition in comment_theme() but the function itself isn't modified.

dvessel’s picture

Yeah, those comments didn't have to be removed for this patch. When someone removed the ability to sort comments, they forgot to remove that.

The node parameter is needed to pass some context. That theming call –theme('comment_folded'.. ends up as a template so any parameters setup through comment_theme automatically gets itself pushed into $variables in the preprocessor.

floretan’s picture

Status: Needs review » Reviewed & tested by the community

@dvessel: Thank you, that makes sense.

A similar process could be used for ID's, but that can (and should) go in a separate issue. The code looks good, and works as desired for each of the templates. I'm fine with flattening the array directly in theme().

dvessel’s picture

Great! Thanks for the review..

If we were to do this for ID's, $class_attr would have been nice. $id is oddly mapped to the number of counts it's used on the page. Maybe $id can be changed to $count.. We'll see.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Great. This looks like a nice little improvement, and has a stamp of approval from several themers. :)

Some nit-picking on the comments, since the code itself looks good:

+ *   - comment-folded: The current theming hook.

Sorry, these ones make no sense to me. Which probably means they make sense to people new to Drupal theming. :) Can we be explicit about what these mean in each .tpl.php file's context?

+ *   - comment-unpublished: Unpublished comments visible to administrators.

..visible only to administrators? Same with node-unpublished.

+ *   - comment-by-anonymous: Comment by unregistered users. 

Comment by an unregistered user.

+ * - $classes: Contains a class for the current theming hook.

This one from comment-wrapper.tpl.php is documented differently than the others. Let's be consistent, even though it only has one value (comment-wrapper).

+ *   - node-preview: When previewing new or edited nodes.

How about "Nodes in preview mode" or something that matches the comments around it.

+ *   CSS. It should be placed in a BODY tag. It has one or more of the following values:

How about "<body> tag"

+ *   - front: When viewing the front page.

Chalk this up to too much book authoring/editing, but can we please structure this (and the rest in this section that start with "When") as complete sentences starting with nouns? "Page is the home page." or something similar.

+ *   - node-type-[node type]: When viewing a single node, the type of that node.

Could we get a for example on that one too, just for good measure? Preferably something like "Blog post" whose type name and machine name are different.

+ *   The following only apply with the default 'left' and 'right' block regions.

Should end in a colon, not a period.

Also, I would like confirmation from someone doing some click-throughs in non-Garland themes with a mix of nodes and comments of various statuses that the pages don't look completely out of whack, please.

webchick’s picture

Hm. Also, what happened to box.tpl.php?

NikLP’s picture

merlinofchaos mentioned to me some months ago that he basically forgot to get rid of the whole "box" phenomenon in D6, so I suspect that this is just gone(?)

dvessel’s picture

Status: Needs work » Needs review
FileSize
32.19 KB
Failed: Failed to apply patch. View
+ *   - comment-folded: The current theming hook.

Sorry, these ones make no sense to me. Which probably means they make sense to people new to Drupal theming. :) Can we be explicit about what these mean in each .tpl.php file's context?

I'm not sure how to describe it in another way. The theming handbook is describes it the same way. Anyone have suggestions? I left it for now.

+ *   - comment-unpublished: Unpublished comments visible to administrators.

..visible only to administrators? Same with node-unpublished.

Yes, unpublished nodes and comments don't show for other users. It's only there for admins. The class is useful for easily spotting them. Does that not properly describe it?

Everything else you noted was fixed. I did add a node-[type] class. It's very useful and forgot to add that.

One note, I have used this code before and I think it is solid and I did test the other themes and found no ill effects. Would be nice to have someone verify.

webchick’s picture

I'm not sure how to describe it in another way. The theming handbook is describes it the same way. Anyone have suggestions? I left it for now.

I'm not sure, but isn't it just "the name of the .tpl.php file" in layman's speak?

Yes, unpublished nodes and comments don't show for other users. It's only there for admins. The class is useful for easily spotting them. Does that not properly describe it?

My bad! I meant that those descriptions should say "Visible *only* to administrators" - minor change.

JohnAlbin’s picture

One thing to note. Themers should be getting into the habit of looking at the variables documentation in the tpl.php files to figure out what variables are available and how to modify them in their preprocess functions.

But, while the new $classes var is documented correctly for the context of the tpl.php. $classes will actually be an array in the preprocess context and, after being flattened in theme(), will be a string. Shouldn't we document that in the tpl.php files as well?

dvessel’s picture

Status: Needs review » Needs work

Webchick, but what does that mean to themers? The name of the template is basically the theming hook unless it's a template suggestion and those have the hook prefixed in the template name. If it's mentioned that it's only the name of the template, what connection can be made about it except that it's in the name. I hope I'm making sense.

Calling it a "theming hook" is how I described how each chunk of themable data is handled in the handbook. The name of the template just follows that convention.

John, That's a very good point. I'll try getting a simple explanation on it. Since it is out of scope, it shouldn't explain everything about how to work with it IMO. Documenting it in the handbook more thoroughly would be better for that.

dvessel’s picture

Status: Needs work » Needs review
FileSize
32.96 KB
Failed: Failed to apply patch. View

Is this any better?

dvessel’s picture

So, anyone want to set this to RTBC?

merlinofchaos’s picture

Status: Needs review » Needs work

I'm tentatively against flattening the classes in theme(). It feels like it's very much the wrong place, that it's completely hacked in. The thing is, there's not a preprocess that always happens last.

I think these are two acceptable solutions:

1) Create a preprocess that always happens last.

2) Create a function to do this: drupal_classes() maybe. This is the same problem we run into with theme('links') for links in page.tpl.php, so any solution we do along this path should work for that as well.

merlinofchaos’s picture

Note that method #2 will neatly solve JohnAlbin's concern in #36.

dvessel’s picture

@merlinofchaos: What do you mean in #2? Could you elaborate a bit?

Dropping it into theme() I suspected wasn't a good thing to do but wasn't sure about it so took the easy route. I don't understand how using drupal_classes() would relate to theme('links') outside of the flattening of the array.

merlinofchaos’s picture

The point being is that it will be an array in preprocess and an array in .tpl.php file; thus the value of the variable remains consistent. I think this is a big deal for usability. Changing variables will confuse people. And the comparison to theme('links') is that we already do (or have done) that kind of thing.

dvessel’s picture

Ah, right. I should have known from the start. Here's what I'll do. $classes will be the array and $class_attr the string. Any objections? If none's given I'll have this up tonight.

Thanks for the input.

merlinofchaos’s picture

That does work, and helps the documentation/user confusion issue, but I am still uncomfortable with special code for this in theme(). I'll be happier with it if we make a special preprocess phase. As a side effect, it could solve a number of other issues too, such as $css and $scripts being generated so early in theme('page') that nothing has a chance to modify it, causing inefficiency.

dvessel’s picture

I agree.. I was going to change that too. So another preprocessor to allow the late flattening of the classes. The other parts ($css/$scripts/etc..) I guess can be done in another queue. I don't want to annoy Webchick.

merlinofchaos’s picture

I agree, so long as what's implemented here helps make the other one possible, it should be good.

dvessel’s picture

Status: Needs work » Needs review
FileSize
40.13 KB
Failed: Failed to apply patch. View

I thought I would have this up last night but I passed out with other work. :)

I hope this looks sound. "Preprocess functions" are grouped into two phases now. The block of code that builds the preprocessors for the registry loops through the phases from _theme_process_registry(). This allows all the same manipulations as the normal preprocessor and behaves exactly the same.

Within theme(), it loops again to execute. The check for its existence was removed since I've never seen it missing. I'm pretty confident this is safe.

When working through the preprocessor to add the classes, it's ['attribute_classes'] and I left the string form as $classes.

webchick’s picture

Neat.

A conversation in #drupal leads me to believe that providing two sets variables: one containing the array representation of something, and one containing the string representation of something, would be a good way to go. I like the more "human-readable" name ($classes) being template-facing. But I wonder if it doesn't make more sense to be forward-thinking in how we name the "raw" value ($classes_raw?) so that we could apply this consistently to other variables in future patches.

@dvessel: Is it possible for you to come in #drupal sometime? It'd be easier to hash out some of this stuff in real time I think between you, merlinofchaos, and I. :)

dvessel’s picture

Sure webchick but it'll have to be late tonight. About 11pm EST if you'll be around.

What I said I would do about the naming before, I didn't.. Instead it's this:

attribute_classes == array
classes == string

I was trying to think forward with the naming and attribute_classes made sense to me. Later we can add "attribute_id" if we want. I'm not attached to it though.

dvessel’s picture

FileSize
40.27 KB
Failed: Failed to apply patch. View

Whoops, my editor gave no indication that it didn't save.

webchick’s picture

I'm *always* around! ;)

And yes, I understand the distinction in the patch. But what I mean is that going forward, I don't know if $attribute_links and $attribute_styles and $attribute_blocks makes equal sense. But it'd be nice to be able to communicate to themers, "Any time you see a composite variable like X, there is a corresponding variable Y that has the 'raw' version." If that makes sense. It might not! :P I'm going by the discussion by a few people on IRC.

dvessel’s picture

FileSize
40.07 KB
Failed: Failed to apply patch. View

Everyone agreed on $classes_array being the array form and $classes being the string in irc.

Re-rolled.

dvessel’s picture

FileSize
40.06 KB
Failed: Failed to apply patch. View

There was a little error in building the registry that I missed from the previous patches. This should be solid.

dvessel’s picture

FileSize
40.08 KB
Failed: Failed to apply patch. View

Only changed the comments. When I moved some code around in _theme_process_registry() it made less sense.

merlinofchaos’s picture

Overall this looks pretty good. I'm a little concerned with the term 'post preprocess' but it's not too bad and I can't think of a better alternative.

dvessel’s picture

Cool..

If anyone can come up with a better name, please speak up!

JohnAlbin’s picture

ok. I've been holding off on these comments for a couple weeks, since I thought it should be in a separate issue, but since this latest patch has a "post preprocess" phase of functions, let me pull out my notes…

First, let me approve of the idea of a new phase of processing. If we add a new phase of functions, we can render all the data structures in that phase. Then we could move the awful <?php print theme('links', $primary_links, array('class' => 'links primary-links')); ?> from page.tpl.php.

Second, I've heard some people say the "preprocess" name isn't very clear what is going on. And adding a "post preprocess" is worse. What the template phases really are doing is:

  1. modify variables or their data structures (currently called preprocess functions)
  2. render the variables using the data structures (needs a name)
  3. render the template using the variables (always uses theme_render_template())

I think laying out the usage should make it easier for us to come up with good names. The unsaid part of “preprocess” functions is that it is preprocessing variables, so if we wanted to keep the preprocess name, couldn't the next phase be “process”? Or, we could keep with the "you really need to read the docs" mentality and have "preprocess" and "prerender" (as it is the phase right before the render template). Alternatively, if we were okay with renaming "preprocess", then we could have "modify variables" and "render variables", or the shorter "modify" and "render".

Third, one of the issues of the existing preprocess phase is that if a subtheme wants to manipulate/render a variable, the parent themes, modules, and theme engine have possibly already rendered the variable, so the active theme has to re-do that work. That's inefficient.

Simply adding a new phase of processing almost fixes that problem, but the active theme still goes last. So, I think we should reverse the order of execution for the new phase; start with the active theme, then base theme, modules, and back to theme engine. That way the active theme gets first crack at rendering the variables. Later rendering functions just need to check if the rendered variable exists so that they know “nothing to do here; move along."

In other words, each "render variables" function should do something like this:

// Render the variable, if earlier "render variables" functions haven't already done so.
if (!isset($vars['classes'])) {
  $vars['classes'] = implode(' ', $vars['classes_array]);
}
dvessel’s picture

I think laying out the usage should make it easier for us to come up with good names. The unsaid part of “preprocess” functions is that it is preprocessing variables, so if we wanted to keep the preprocess name, couldn't the next phase be “process”? Or, we could keep with the "you really need to read the docs" mentality and have "preprocess" and "prerender" (as it is the phase right before the render template). Alternatively, if we were okay with renaming "preprocess", then we could have "modify variables" and "render variables", or the shorter "modify" and "render".

Hmm.. I like "process". It makes sense to me. The only little point that might be confusing is that not all "preprocess" phases will have a "process" counterpart. It's minor though.

I don't think we should rename preprocessors though. It's already been established so it may do more harm than good and preprocess is a good term.

Third, one of the issues of the existing preprocess phase is that if a subtheme wants to manipulate/render a variable, the parent themes, modules, and theme engine have possibly already rendered the variable, so the active theme has to re-do that work. That's inefficient.

Simply adding a new phase of processing almost fixes that problem, but the active theme still goes last. So, I think we should reverse the order of execution for the new phase; start with the active theme, then base theme, modules, and back to theme engine. That way the active theme gets first crack at rendering the variables. Later rendering functions just need to check if the rendered variable exists so that they know “nothing to do here; move along."

But the two phases solve what you mention. You just have to be aware of where you have to modify your variables. In most cases the "processor" or "post preprocess" should not be used. Here's a partial cut & past from the theme docs in addition to the new phase.

It will run in this order:

  1. template_preprocess
    –initial "hook class" added here.
  2. template_preprocess_hook
    –Hook specific classes added here.
  3. moduleName_preprocess
    –Modules can add their classes here.
  4. moduleName_preprocess_hook
    –modules can add their classes here on specific hooks.
  5. engineName_engine_preprocess
  6. engineName_engine_preprocess_hook
  7. engineName_preprocess
    –Themes can add their classes here. - This is the first preprocessor that can be used inside the theme.
  8. engineName_preprocess_hook
    –Themes can add their classes here on specific hooks. - Another preprocessor named after the engine but specific to a single hook.
  9. themeName_preprocess
  10. themeName_preprocess_hook –END OF FIRST PHASE
  11. template_process –START OF THE SECOND PHASE
    - This is supplied by core and always added. At the moment, this is where the classes_array is flattened since it applies to all hooks. This start of the second phase will *always* come after the normal preprocess functions.
  12. template_process_hook
    - The module or core file that implements the theming hook supplies this. We can add the what we discussed as a followup so $styles, $primary_links, $secondary_links can be flattened here as they are specific to a single theme hook.
  13. moduleName_process
  14. moduleName_process_hook
  15. engineName_engine_process
  16. engineName_engine_process_hook
  17. engineName_process
    - This is the first "processor" or "post preprocessor" that can be used inside the theme. Most themes *will not* require this.
  18. engineName_process_hook
  19. themeName_process
  20. themeName_process_hook

Note: For anyone who's not familiar, the above are the possible preprocess/process functions. In most cases, it would only be three of them, sometimes four.

So, I don't see how we need to change the order. Reversing the order seems interesting for the latter stages but is there another use case? It may help when there's a competing processor but I can't see a situation where that'll be the case.

JohnAlbin’s picture

It may help when there's a competing processor but I can't see a situation where that'll be the case.

In core, there should never be two "process" functions trying to render the same variable. But in contrib and in themes, I see this happening all the time.

Here's an example of what I was meaning: if, in page.tpl.php, we replace the print theme('links', $main_menu, array('class' => 'links main-menu')); with just print $main_menu; (which we should definitely do, btw), we would have a $main_menu_array variable to play with in the preprocess phase.

Then, in template_process_page, we'd have:

$main_menu = theme('links', $main_menu_array, array('class' => 'links main-menu'));

But if a theme wanted to use a completely different rendering method (like implode(' | ', $main_menu_array)), the theme's process function would be re-rendering the primary links (because it always comes after template_process_page). Double the work with no added benefit.

That's why I was suggesting reversing the order of the process phase and having each process function make sure the variable hasn't already been rendered. Granted, having them check the existence of the variable before rendering is a non-enforceable convention, but its the easiest solution to the double-work problem.

The other option would be to keep the process phase in the same order as preprocess, but then require rendering functions to be overridable theme function and be uniquely identifiable for that variable; of course, that's a non-enforceable convention, too.

dvessel’s picture

John. I like your ideas but lets discuss this in another queue. Any changes after this won't be difficult. But that name "process".. Anyone else want to weigh in? Good/bad? And some testing would be nice. I tested myself and no problems here.

dvessel’s picture

Assigned: dvessel » Unassigned
Category: bug » task
webchick’s picture

FWIW, I would *love* to commit this to UNSTABLE-2, set to release on Sunday.

What's needed to move this along? Just the naming of the prepreprocess function, or..?

dvessel’s picture

Just the naming for now IMO. I thought "process" sounded good or we can stick with "post preprocess" for now.

sun’s picture

Friends, can we take a step back and ask us what we are doing here?

Let me try a take: We are programatically adding meta information to containers in our markup. However, is meta information in HTML markup limited to classes? No. HTML markup supports other attributes, too. Oh, and guess what? We already have drupal_attributes()!

And, we are already using drupal_attributes() in a lot of places throughout core, f.e. in theme_item_list() or theme_links().

So, if we really want to do this (+1 in general, yeah!), why limit it to $vars['classes'] and do not use $vars['attributes'] instead? Just think of $vars['attributes']['id'] for blocks, and other attributes that could be set programatically.

If we would do that, this would be major step towards semantic data and output. Of course, there still would have to be a lot of follow-up issues and patches to attach those attributes to (page) elements, resp. entities (think HTML/XML/JSON), but in the end, this patch would not only provide us classes for hook templates, but also be a step into the right direction.

Just my $0.02

dvessel’s picture

sun, I have thought about that and didn't like the restrictions it imposes.

Using drupal_attributes would force *every* class to be added programatically. We don't want that. There will always be a few classes that are static and using drupal_attributes would force something closer to this.

<div <?php print $html_attributes; ?>>
  ...
</div>

Instead of this:

<div id="eyedee" class="<?php print $classes; ?> clear-block some-other-whatever" >
  ...
</div>

There are some classes we wouldn't want to push through drupal_attributes. Static classes should be exposed right inside the template IMO.

JohnAlbin’s picture

Could we do drupal_attributes() in the preprocess functions and then render them out to separate variables right before they hit the tpl.php files?

Then we could still do the (extremely important, btw) static classes:

<div id="<?php print $id; ?>" class="<?php print $classes; ?> clear-block some-other-whatever" >
  ...
</div>

Just thinking out loud.

sun’s picture

First of all, dvessel is right.
Secondly, we cannot use drupal_attributes(), because that function renders a complete HTML attribute string, which is what we want to avoid.

BUT, we can still use $attributes['classes'] instead of $classes, and hence, open the door for $attributes['id'] and whatever else we might want to add programatically someday.

What counts is the paradigm shift in starting to see templates as containers, i.e. entities that need to have programmatic attributes. If we'd want to go crazy, we would even use $variables['#attributes'], i.e. for laying the corner stone to let drupal_render() take over the rendering of other formats than HTML someday.

dvessel’s picture

Well, the only thing drupal_attributes does is flatten all the attributes into string while passing it through check_plain. We can easily extend what the patch does so it includes any attribute we want.

Altering drupal_attributes would require a lot of code changes in other areas (mostly forms I think) which I don't think is necessary.

dvessel’s picture

re: sun.. Exactly!

sun’s picture

Status: Needs review » Needs work

So this means effectively PNW. I find $vars['#attributes'] much more attracting and door-opening with regard to drupal_render() than $vars['attributes'] - what do others think?

webchick’s picture

I'd love to get eaton/pwolanin/chx's take on that.

But in general, we avoid the use of #properties unless we have to. In FAPI this is necessary because we support multi-tiered arrays and need a way to judge a property from an element. I'm not sure that we have such restriction on HTML attributes. But I suppose without that, we'd need some special-casing in drupal_render().

JohnAlbin’s picture

EclipseGC pinged me about this issue in IRC. I promise to give this a go soon; especially since the updated node.tpl.php would need this.

To recap what work needs to be done on this patch now:

  1. I proposed that the new phase be called "process". Dvessel agreed. But we had no other suggestions or anybody else agreeing/disagreeing with the calling the new variable rendering phase "process". See #59 above for how/when the new phase would work. So we'd need to re-roll the patch with the new name.
  2. The $classes variable needs to be renamed $attributes['classes']. Actually, should this be $attributes['classes'] or $attributes['class']? I'm thinking “class”.
alexanderpas’s picture

  1. process is good
  2. it certainly should be $attributes['class']
eaton’s picture

My thoughts on the matter:

First, I like the consistent use of $classes throughout all tpl.php files. It makes me very happy.

Second, drupal_attributes() makes me want to stab kittens. It's a reasonably useful solution for handling special cases in widget-rendering that we didn't anticipate, but it sucks mightily in this scenario. What problems does it add to the mix?

  1. CSS IDs, the only obvious use case for #attributes in non-FAPI contexts, are problematic. ID collision is a serious issue, and modules have no way of discovering what other IDs are in use on the page. It's best left in the hands of the themer. In core, we've had to work hard to move to class-based selectors whenever modules do their work to avoid validation collisions. Adding a global mechanism to jigger CSS IDs around willy-nilly feels a bit dangerous.
  2. Moving past IDs, there are few HTML attributes that even make sense to add via code. Adding arbitrary XML attributes via code assumes we know what tag the attributes will be attached to -- and that's in the hands of the tpl.php designer, not the coder jamming #attributes full of metadata.
  3. As others have noted, drupal_attributes() renders one giant string for all attributes -- an unacceptable limitation for tpl.php designers.
  4. If we pre-render each type of attribute and let designers print them out separately, we encounter the 'how can I print things I don't know are there' problem. Most will just print $attributes['classes'] and move on.

The mismatch between our string-printing tpl.php files and our hierarchical metadata drupal_render() structures is definitely problematic. But the issue of CSS classes isn't the right place to begin pulling them closer. $classes should be simple, easy, and straightforward.

JohnAlbin’s picture

Assigned: Unassigned » JohnAlbin

@Eaton: thank you for the comments! Your concrete points add validation to that tingly, “not quite right” spidey sense I was having over the issue.

The $attributes[*] thing didn't quite seem right to me. And I agree, adding $attributes['id'] programatically is dangerous. And I can't think of any other attributes that would need such treatment. Therefore, $attributes[*] is a premature optimization at this point. So, I'm going to revert to our original, simper $classes variable.

Ok, I'm starting work on writing this patch now. If I'm not back in 24 hours, smack me in IRC!

JohnAlbin’s picture

FileSize
34.59 KB
Failed: Failed to apply patch. View

For reference (my own mostly), here’s a re-roll of Joon’s patch in #55. Now I’ll work on updating it with the most recent discussions.

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
40.72 KB
Failed: Failed to apply patch. View

ok. So the new fields in core modules have *_process functions that match theme functions. Which means we get a ton of errors like this:

Warning: Missing argument 3 for number_process() in number_process() (line 294 of /Users/jwilkins/Sites/Drupal-CVS/core-HEAD/modules/field/modules/number/number.module).
Warning: Missing argument 4 for number_process() in number_process() (line 294 of /Users/jwilkins/Sites/Drupal-CVS/core-HEAD/modules/field/modules/number/number.module).
Notice: Undefined index: #field_name in number_process() (line 295 of /Users/jwilkins/Sites/Drupal-CVS/core-HEAD/modules/field/modules/number/number.module).
+ 11 more

However, if we rename those functions, the errors neatly go away. Longer function names are bad, but conflicting APIs are worse.

Ok, here's the updated patch. Review!

mfer’s picture

subscribe. need to review this.

JohnAlbin’s picture

Issue tags: +theme, +tpl-refresh

Summary of Comments So Far

Ok. I got asked in IRC for an overview comment. So, here’s the story so far… [queue LOTR soundtrack]

We started with a simple idea, generalize the $body_classes variable so that there will be a $classes variable for each tpl.php file. In order to make adding/modifying the classes easier, we decided to put each class as an element in an array, the $classes_array with the array imploded to string in $classes.

There still remained the problem of having to re-render the $classes string if you modify the $classes_array. In #45 (and later), merlinofchaos and dvessel talk about adding a new "phase of preprocess functions" that would run after the current preprocess functions so that variables like $classes_array could be rendered in this new phase. We decided to call the new phase "process functions" and dvessel laid out the new order of theme processing functions in #59.

There was a very interesting discussion about replacing $classes with a drupal_render() in #65-#72. But in #75, eaton describes how drupal_render() is a poor match for html classes and IDs. And, without any other pressing use cases, I agreed with eaton saying “$attributes[*] is a premature optimization at this point.” We can certainly consider other use-cases of drupal_render() for non-class and non-id attributes in another issue.

Then I took dvessel's excellent 90% finished patch from #55 and polished it up.

The best way to test this patch would be to apply the patch and not rebuild the theme registry. Your page layout will instantly break and you will get an error about the variable $classes not being defined. This is because the patch replaces the $body_classes string with a $classes_array array in template_preprocess_page(). Since the temporarily-out-of-date theme registry doesn't know about the new process functions, the $classes_array won't be rendered to a string and saved in $classes.

In order to fix those broken bits (and prove the patch works), you'll have to rebuild the theme registry. (Side note: I had some issues with devel.module cache rebuilding and also with going to /admin/build/themes, so I had to temporarily add drupal_theme_rebuild() to index.php; I'm not sure what was up with that.)

JohnAlbin’s picture

Title: Add $classes to hook templates » Add $classes to templates with new theme_process_hook functions

I think the old issue title was burying the lead a little bit, so I'm renaming.

Also, perhaps this is obvious (to some), but let me spell out the implications of adding THEMENAME_process_HOOK functions in this issue:

We can create a series of follow-up issues that delay rendering many tpl variables to the process phase, allowing themers greater control over how the page data's HTML is generated.

Status: Needs review » Needs work

The last submitted patch failed testing.

JohnAlbin’s picture

You know… it would be really helpful if the bot would send an email when it changes the issue status to "needs work". :-p

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
42.46 KB
Failed: Failed to apply patch. View

Updated patch.

dvessel’s picture

Status: Needs review » Needs work

There are a few notes I would correct. I missed this in my previous patch too.

#336  // Check for default _preprocess_ functions. Ensure arrayness.

Should be changed to something like:

Check for existing variable processors. Ensure arrayness.

Since it handles both preprocess and process functions. A more general term? There are a few lines below that that mention "preprocess" and they should also be changed.

This is next one is a nitpick but where the variables processors are set for themes when templates are not owned by the theme.. There's a few lines of repeating code. How about looping through the two phases like it's done before? Very minor.

Also, I'm not familiar with the number_process() and why it's being changed to number_elements_process(). Is that part of another patch? Same with options_buttons_process() and a few others.

Everything else works just fine here.

Thanks John for picking up the slack. My mind's been elsewhere.

JohnAlbin’s picture

Quick note: number_process() was changed to number_elements_process() because the former was being picked up as the theme_process hook for the new number.module. The later number_elements_process() is immune from that confusion.

dvessel’s picture

Ah, okay. I missed that.

geerlingguy’s picture

Things seem to be moving along here - I was simply wondering if we're getting close to the finish line on this issue, and, if so, what kind of things will need to happen in themes to account for the changes here?

Jeff Burnz’s picture

This is looking great, just raising a few questions:

Were zebra/odd even classes left out for any reason, these are quite handy, especially for comments
I always liked and often used Zens section- class, I think this was quite useful also.
Will there be some consistency in where we can modify $classes, appears some are done in preprocess and some in process.

Lastly, and this is a pretty big question:

The only way I can see to modify these is using preprocess and process functions, at the risk of sounding insane, could we not store each class in a token and allow end users to select which tokens/classes they want to use?

For example on the theme settings page we have something similar to what pathauto has where we can insert tokens, wouldn't this also open up the possibility for an amazing array of tokens thus classes to be used?

Just thinking out loud, tell I'm mad and I'll shut up:)

ultimateboy’s picture

Why are we stopping at just classes? Why not expand this to all attributes - all running through a single function.

dvessel’s picture

jmburnz, zebra can be added through $zebra if you wish. We have enough classes in there as it is. And the token classes can be done from your theme if you wish. It's outside the scope of this patch.

ultimateboy, that's been mentioned and everyone agreed it's not a good idea.

Jeff Burnz’s picture

@#91, yeah, apologies, got excited and ran off topic, my bad.

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
47.7 KB
Failed: Failed to apply patch. View

Per comment #85, I've updated tons of comments in includes/theme.inc.

Also, I added a loop over the code where "variables processors are set for themes when templates are not owned by the theme". This meant I had to set $template_phases earlier in the the _theme_process_registry function.

dvessel’s picture

Thanks for the re-roll! Was tempted to do it myself but was afraid it would sit here waiting for a review.

I'll check it out this weekend if someone else doesn't beat me to it.

dvessel’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
49.65 KB
Failed: Failed to apply patch. View

What is going on? This is the second time my comment just vanished..

As I was saying.. The patch John created is solid. The only thing we forgot about was the install and update pages. They work within the theme system like no other so I had to add the variable process functions manually. This was already done before but with the new tweaks, it needed more.

Also tweaked the docs above theme() to make clear that the variables processors work in two distinct phases. The rest John added in there is very good.

I think this is ready.

JohnAlbin’s picture

The only difference between dvessel's patch (besides code comments) and my own are the 4 additional lines in theme_install_page and theme_update_page. I tested the patch and those changes make 100% sense to me, so I'm RTBC'ing them.

catch’s picture

Did some quick benchmarks, no measurable performance impact that I can see.

Damien Tournoud’s picture

Nice.

Looking at the patch, I'm thinking we should probably remove the old *sidebar* classes, and replace them with has-region-<region>, but that's out of the scope of this patch.

geerlingguy’s picture

@ Damien - Yeah, I'd say create a new issue, as this current issue, once committed, will allow us to finally finish up #382870: Update and Polish Node Template Output.

sun’s picture

Looks good.

But, let's also think about use-cases. Let's suppose:

I am a themer. I don't know PHP very well. I have $classes in my template. I see how I can add more CSS classes there. But how can I change or remove one of the pre-defined classes?

Ah! hook_preprocess_node() it is. Let's see.

$classes = $variables['classes_array'];
// Hm.
// var_dump($classes) ...
unset($classes[1]);
// Oh!  Why is it in $classes[2] elsewhere?  And why in $classes[3] there...?
// Hm.
// Found this funny snippet on drupal.org.  Let's see.
$key_teaser = array_search('node-teaser', $classes);
unset($classes[$key_teaser]);
// Finally.  Wow, that's ugly.

Just an idea - how about preventing this in the first place and doing the following instead?

+  // Gather node classes.
+  $variables['classes_array']['node-' . $node->type] = 1;
+  if ($variables['promote']) {
+    $variables['classes_array']['node-promoted'] = 1;
+  }
+  if ($variables['sticky']) {
+    $variables['classes_array']['node-sticky'] = 1;
+  }
+  if (!$variables['status']) {
+    $variables['classes_array']['node-unpublished'] = 1;
+  }
+  if ($variables['teaser']) {
+    $variables['classes_array']['node-teaser'] = 1;
+  }
+  if (isset($variables['preview'])) {
+    $variables['classes_array']['node-preview'] = 1;
+  }

...

+function template_process(&$variables, $hook) {
+  // Flatten out classes.
+  // Basically just added array_keys() here. 12/05/2009 sun
+  $variables['classes'] = implode(' ', strtr(array_keys($variables['classes_array']), '_', '-'));
+}

So my little themer can simply do:

// Drop 'node-teaser' class.
unset($variables['classes_array']['node-teaser']);

Thoughts?

dvessel’s picture

Sun, look at #14. Someone can post that as a snippet and all the themer has to do is fill the array with the classes he/she wants to remove. Minor alterations will allow swapping if needed like so:

function mytheme_preprocess(&$vars, $hook) {
  // Fill me with things to swap. 'original' => 'changed'
  $swap_classes = array('page' => 'body', 'box' => 'square', 'foo' => 'bar');
  foreach ($vars['classes'] as $i => $class) {
    if (isset($swap_classes[$class])) {
      $vars['classes'][$i] = $swap_classes[$class];
    }
  }
}

I don't think we need to use another layer of array keys in this case. We have template suggestions which are added the same way. It makes this consistent with how we're working with classes here.

geerlingguy’s picture

@ sun / @ dvessel : +1 to a snippet being posted.

Damien Tournoud’s picture

Status: Reviewed & tested by the community » Needs work

+1 to keying classes_array by the name of the class. It's consistent with what we do mostly elsewhere. I see no compeling reason not to.

sun’s picture

Had further thoughts on this - another use-case:

Modulitis! Two independent modules may want to re-use the same class, each one being able to set it individually. If both modules set it, and we're keying $classes_array by class name, you won't end up with duplicate classes in your output.

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
49.03 KB
Failed: Failed to apply patch. View

Ok, I considered the key = value option.

But, while that works for adding static classes, like $vars['classes_array']['node-teaser'] = 'node-teaser';, it doesn't work well for adding programmatically-determined classes like:

$variables['classes_array']['one-sidebar sidebar-' . $variables['layout']] = 'one-sidebar sidebar-' . $variables['layout']; // Ugly long

or

// form_clean_id won't return the same value if called twice.
// I hate form_clean_id for classes, but adding a form_clean_class or drupal_clean_class is a separate patch.
$class = preg_replace('![^abcdefghijklmnopqrstuvwxyz0-9-_]+!s', '', form_clean_id(drupal_strtolower($suggestion)));
$variables['classes_array'][$class] = $class;

If we went with that key = value option, we would make removing classes easy, but adding classes hard.

So, this new patch uses the key = TRUE option. And template_process just implodes the array_keys of classes_array.

sun’s picture

Yes, using form_clean_id() is totally wrong for CSS classes. We definitely need drupal_css_name() and rename that former to drupal_css_id(), re-using drupal_css_name(), but applying additional unique-logic. All of that is a separate issue though.

When reading

+    $variables['classes_array']['no-sidebars'] = TRUE;

my first thought is that TRUE has some meaning elsewhere and I should better look that up. Why didn't you go simply with 1 as value? Admittedly, it does not explain more, but it is the commonly used value for fake array values of this kind of arrays.

Additionally, I would like to respectfully ask whether we can remove this magic here:

+  $variables['classes'] = str_replace('_', '-', implode(' ', array_keys($variables['classes_array'])));

There may be use-cases where someone would explicitly want - or requires - underscores in class names. Think of an external JavaScript library that requires you to add the do_funny_thing CSS class to target entities on the page. Impossible with this magic replacement.

dvessel’s picture

I don't agree with using the keys for classes. In most situations, classes will simply be added so it should be simplified to do just that. It's not impossible to do other things with it just because we don't have them keyed.

As for this:

+  $variables['classes'] = str_replace('_', '-', implode(' ', array_keys($variables['classes_array'])));

Without that, then there would be no consistency in class naming. (I keep saying this but consistency is a big deal to me.) We are aiming for what Drupal is likely to output and if you need to use another library that uses underscores, $variables['classes_array'] is still there which can be flattened without replacing. Assuming an implode is cheap, why worry?

Damien Tournoud’s picture

I'm with sun against the magic replacement of class names. This replacement (if necessary) should the responsability of the caller.

Status: Needs review » Needs work

The last submitted patch failed testing.

JohnAlbin’s picture

FileSize
48.69 KB
Failed: Failed to apply patch. View

Reviewing the existing code, I see that we currently require the caller to replace the underscores. I've removed that from template_process() in this patch. But it would be very nice to put that functionality in: #464862: Add drupal_css_class() to clean class names and rename form_clean_id with a $enforce_code_style = TRUE parameter.

@sun

When reading $variables['classes_array']['no-sidebars'] = TRUE;, my first thought is that TRUE has some meaning elsewhere and I should better look that up. Why didn't you go simply with 1 as value? Admittedly, it does not explain more, but it is the commonly used value for fake array values of this kind of arrays.

And my first thought was that 1 has some meaning elsewhere. :-D I picked TRUE after a brief code search and discussion with DamZ in IRC; but I don't remember the examples I looked at to arrive at my decision.

@dvessel

In most situations, classes will simply be added so it should be simplified to do just that.

When i start mucking with layout and moving pieces of content into the sidebars (an advanced theming technique, I’ll admit), I often have to delete the layout body classes. And it took me quite a while to figure out how to delete an item from an array by given-value (using a technique involving array_search() and unset()). And it wasn't until DamZ pointed out $classes = array_diff($classes, array('class-to-remove')); that I knew of a one-liner to accomplish that. But those are non-trivial code snippets to figure out how to delete classes.

So… which of these 3 is easier to understand and/or to figure out on your own?

  1. Add classes with: $classes['class-to-add'] = TRUE;
    Remove classes with: unset($classes['class-to-remove']);
  2. Add classes with: $classes['class-to-add'] = 1;
    Remove classes with: unset($classes['class-to-remove']);
  3. Add classes with $classes[] = 'class-to-add'
    Remove classes with: $classes = array_diff($classes, array('class-to-remove'));

Its the removal bit of #3 that makes me lean towards #1.

JohnAlbin’s picture

Status: Needs work » Needs review
dvessel’s picture

Adding drupal_css_class looks like a good idea. The issue of the caller doing the cleaning might not matter much here if the default *is* to clean the underscores (which I believe it should) since the caller will be from a default process function. Outside the control of most themes. There might be other ways around that can be discussed in the other queue.

If most of you think the classes should be keyed, then I'm okay with that. The reason I object is that it reads in an odd way. In most situations I imagine the key as a pointer and the end value as the value. There are always exceptions of course.

Also, working with variables in these processors as a whole, having keys two levels deep is not something that's normally done. Not a problem for most of us who understand how it would work but it's another mental barrier for new themers. Adding multiple values in an array structure is done without the extra keys in the .info file. It's the same with how it's currently done in preprocess functions unless it starts to get on the complex side. -Am I grasping here with trivial examples?

I wouldn't put much weight in how a class can be pulled out in a single line. There are other ways to do it where it's still easy to read and understand.

I'm not sure how strong my arguments are since I'm no longer new to theming. Let's just get this in already. :)

dvessel’s picture

One thing I'd like to add. When I started theming, I remember picking up on patterns and coding conventions which helped me in a big way. The less there is to learn and the more consistent everything is, the faster it can be picked up. I hope we can apply this everywhere it's reasonably possible. Not just here.

Status: Needs review » Needs work

The last submitted patch failed testing.

JohnAlbin’s picture

Issue tags: +Needs themer review

Needs themer review

webchick’s picture

Issue tags: +new feature blocker

Making up a new tag.

This issue is blocking a lot of nice improvements to the core template files. Would love to see this sucker fixed.

Jacine’s picture

I think handling all attributes is important, not just class.

Has anyone seen the way the studio theme handles this?
http://cvs.drupal.org/viewvc.py/drupal/contributions/themes/studio/canva...

/**
 * Create a string of attributes form a provided array.
 * 
 * @param $attributes
 * @return string
 */
function canvas_render_attributes($attributes) {
  if($attributes) {
    $items = array();
    foreach($attributes as $attribute => $data) {
      if(is_array($data)) {
        $data = implode(' ', $data);
      }
      $items[] = $attribute . '="' . $data . '"';
    }
    $output = ' ' . implode(' ', $items);
  }
  return $output;
}

Preprocess example:
http://cvs.drupal.org/viewvc.py/drupal/contributions/themes/studio/canva...

$vars['node_attributes']['id'] = 'id-to-add';
$vars['node_attributes']['class'][] = 'class-to-add';

Seems to me, with the render_attributes function, removing classes could be an easy addition. Could be something like:
$vars['node_attributes']['remove']['class'] = 'class-to-remove';

This result is more consistent templates. $attributes is named $attributes no matter what template file you are in, just like $content.

<div<?php print $attributes; ?>>
  <?php print $content; ?>
</div>

Just thought it was worth bringing up.

Jacine’s picture

Re: #110

If I had to pick one of these it would be #3. While, the array_diff() is probably more on the advanced side, I think:

1 - Adding classes is probably a more common occurrence than removing them, so why force someone who might be uncomfortable with PHP in the first place to use = TRUE or = 1.
2 - I think any themer really digging in to remove classes can be easily guided to remove classes by including a commented example.

Jeff Burnz’s picture

@#110

I like #3, adding classes is intuitive.

#1 is nice, yes indeed, but I can see themers trying this $classes['class-I-want-to-unset'] = FALSE;, which would be intuitive. Either way a nice example in comments would do the trick.

I prefer #3.

geerlingguy’s picture

Please, please give snippets/examples somewhere in the handbook (I will do this if no one else does... but they can't be put in until we've committed this patch anyways). That makes this oh-so-much easier to grok for non-programmers. I like #1 mostly.

dvessel’s picture

Thanks for the feedback.

Just to illustrate the differences directly on *adding* classes:

Keyed classes:

function mytheme_preprocess_node(&$vars) {

  if ($vars['teaser']) {
    // Theme a teaser node by type.
    $vars['classes_array']['node-teaser-' . $vars['node']->type] = TRUE;
    // Add a striping class.
    $vars['classes_array']['node-' . $vars['zebra']] = TRUE;
  }
  // Is the current user the author if the current node?
  if ($vars['uid'] == $vars['user']->uid) {
    $vars['classes_array']['node-by-viewer'] = TRUE;
  }

Class as value:

function mytheme_preprocess_node(&$vars) {

  if ($vars['teaser']) {
    // Theme a teaser node by type.
    $vars['classes_array'][] = 'node-teaser-' . $vars['node']->type;
    // Add a striping class.
    $vars['classes_array'][] = 'node-' . $vars['zebra'];
  }
  // Is the current user the author if the current node?
  if ($vars['uid'] == $vars['user']->uid) {
    $vars['classes_array'][] = 'node-by-viewer';
  }

}

There are many ways to remove them and the one liner examples in the real world would fall into 3 since it should check for it's existence before removing. Theme's should be more systematic in its approach anyway to prevent it from going out of control. Something I've been experiencing quite often.

dvessel’s picture

Status: Needs work » Needs review
FileSize
48.04 KB
Passed: 11503 passes, 0 fails, 0 exceptions View

I'm done with this one. Whatever happens with this one I'll leave others to decide on.

webchick’s picture

I think #117 is worth doing, but we should handle that in a separate issue. This patch has become the very definition of scope creep. :) Let's keep it to $classes and $classes only.

I'm happy to commit this, once we get a final thumbs-up from someone on the theming crew (Jacine, jmburnz, geerlingguy).

Jacine’s picture

thumbs up! ;)

webchick’s picture

Jacine: If it's really thumbs up, then you need to change the "Status" field to "reviewed and tested by the community." :) Are all of your concerns addressed?

Jacine’s picture

Status: Needs review » Reviewed & tested by the community

@webchick Yes, the patch looks good to me. I appreciate your asking for for themer input. I'm also happy that you think #117 is worth doing and I agree it belongs in a separate issue.

webchick’s picture

Great! :D

Providing there are no other issues raised before tomorrow (and don't be shy!), I'll go ahead and commit this then. Thanks so much for the reviews, folks!

JohnAlbin’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
48.84 KB
Passed: 11506 passes, 0 fails, 0 exceptions View

This patch only differs in 2 ways from dvessel's patch in #122:

Everything else is identical to his patch.

Letting the testbot validate this patch, then I'll set back to RTBC.

JohnAlbin’s picture

Status: Needs review » Reviewed & tested by the community

All’s good.

geerlingguy’s picture

I'm good too. I'm guessing the issue will be marked "needs documentation" soon... Could we get a snippet for adding, and a snippet for removing a class?

webchick’s picture

In a final look through this, the only kind of weird things I see are:

comment.tpl.php:

+<div class="<?php print $classes . ' ' . $zebra; ?>">

block.tpl.php:

+<div id="block-<?php print $block->module . '-' . $block->delta; ?>" class="<?php print $classes; ?> clearfix">

Why are $zebra and clearfix not included in the $classes array? Every other template simply does print $classes;

JohnAlbin’s picture

Oh! Garland's comment.tpl. No wonder I wasn't seeing it in modules/comments/comments.tpl.php.

Actually, this patch isnt' tackling specific comment classes; we're just giving it the generic $classes treatment (via template_process()). There's actually a lot of templates that could use $classes lovin'. But we just tackled page and node in this patch.

Can we tackle comments and others in the tpl-refresh issues?

Also, "clearfix" is only used in a few select spots and it affects layout tremendously. And its not a dynamically determined classname. So I'd rather not hide a static class inside a variable. Also, it does give a good hint to themers about how they could add static classes to the tpl.

webchick’s picture

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

Sounds good to me. This poor issue has suffered enough.

Committed to HEAD!!

Thank you SO much everyone! This is a tremendous improvement for themers!!

Marking "Needs Documentation" until the theme update page is updated.

Jeff Burnz’s picture

Hooray, yes really, thankyou sooo much to everyone who worked this, a great improvement.

dvessel’s picture

Status: Needs work » Fixed

Status: Fixed » Closed (fixed)
Issue tags: -Needs Documentation, -theme, -tpl-refresh, -Needs themer review, -new feature blocker

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