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.
Comment | File | Size | Author |
---|---|---|---|
#128 | hook-classes-306358-128.patch | 48.84 KB | JohnAlbin |
#122 | hook-classes-5-27a.patch | 48.04 KB | dvessel |
#110 | hook-classes-306358-110.patch | 48.69 KB | JohnAlbin |
#105 | hook-classes-306358-105.patch | 49.03 KB | JohnAlbin |
#95 | hook-classes-306358-93.b.patch | 49.65 KB | dvessel |
Comments
Comment #1
moshe weitzman CreditAttribution: moshe weitzman commentedAh, thats much better.
Comment #2
NikLP CreditAttribution: NikLP commentedHELL YES.
Comment #3
add1sun CreditAttribution: add1sun commentedYeah, 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.
Comment #4
yoroy CreditAttribution: yoroy commentedYes please. This is built into the Zen theme which we use all the time, and is something that saves us lots of time, everytime.
Comment #5
mortendk CreditAttribution: mortendk commentedoooh yeah the king approves ;)
Comment #6
NikLP CreditAttribution: NikLP commented"+1. This is built into the Zen theme which we use all the time, and is something that saves us lots of time." ;P
Comment #7
webchickI 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?
Comment #8
add1sun CreditAttribution: add1sun commentedMy 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.
Comment #9
JohnAlbinI 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.
Comment #10
dvessel CreditAttribution: dvessel commentedThis 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.
Comment #11
JohnAlbin@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
.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.
Thoughts?
Comment #12
dvessel CreditAttribution: dvessel commentedJohn, 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.
Comment #13
JohnAlbinAh! I see where I misunderstood your comment…
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. :-)
Comment #14
dvessel CreditAttribution: dvessel commentedIf 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..
Comment #15
alexanderpas CreditAttribution: alexanderpas commented+1 to this feature ;)
Comment #16
floretan CreditAttribution: floretan commentedHere'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.
Comment #17
dvessel CreditAttribution: dvessel commentedflobuit, 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:
Will output this for node.tpl.php:
For page.tpl.php:
block.tpl.php:
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?
Comment #18
floretan CreditAttribution: floretan commented@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.
Comment #19
webchickLooks 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? :(
Comment #20
dvessel CreditAttribution: dvessel commentedWebchick, 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.
Comment #21
dvessel CreditAttribution: dvessel commentedJust 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.
Comment #22
JohnAlbinI 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?
Comment #23
dvessel CreditAttribution: dvessel commentedThanks 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.
Comment #24
JohnAlbin@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.
Comment #25
dvessel CreditAttribution: dvessel commentedI 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. :)
Comment #26
dvessel CreditAttribution: dvessel commentedForgot one class. 'sticky' was changed to 'node-sticky'.
Comment #27
floretan CreditAttribution: floretan commentedThat looks great, I like this approach.
Some unrelated bit got inserted, and should be moved to a different issue:
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.
Comment #28
dvessel CreditAttribution: dvessel commentedYeah, 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.
Comment #29
floretan CreditAttribution: floretan commented@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().
Comment #30
dvessel CreditAttribution: dvessel commentedGreat! 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.
Comment #31
webchickGreat. 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:
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?
..visible only to administrators? Same with node-unpublished.
Comment by an unregistered user.
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).
How about "Nodes in preview mode" or something that matches the comments around it.
How about "<body> tag"
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.
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.
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.
Comment #32
webchickHm. Also, what happened to box.tpl.php?
Comment #33
NikLP CreditAttribution: NikLP commentedmerlinofchaos 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(?)
Comment #34
dvessel CreditAttribution: dvessel commentedI'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.
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.
Comment #35
webchickI'm not sure, but isn't it just "the name of the .tpl.php file" in layman's speak?
My bad! I meant that those descriptions should say "Visible *only* to administrators" - minor change.
Comment #36
JohnAlbinOne 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?
Comment #37
dvessel CreditAttribution: dvessel commentedWebchick, 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.
Comment #38
dvessel CreditAttribution: dvessel commentedIs this any better?
Comment #39
dvessel CreditAttribution: dvessel commentedSo, anyone want to set this to RTBC?
Comment #40
merlinofchaos CreditAttribution: merlinofchaos commentedI'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.
Comment #41
merlinofchaos CreditAttribution: merlinofchaos commentedNote that method #2 will neatly solve JohnAlbin's concern in #36.
Comment #42
dvessel CreditAttribution: dvessel commented@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.
Comment #43
merlinofchaos CreditAttribution: merlinofchaos commentedThe 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.
Comment #44
dvessel CreditAttribution: dvessel commentedAh, 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.
Comment #45
merlinofchaos CreditAttribution: merlinofchaos commentedThat 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.
Comment #46
dvessel CreditAttribution: dvessel commentedI 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.
Comment #47
merlinofchaos CreditAttribution: merlinofchaos commentedI agree, so long as what's implemented here helps make the other one possible, it should be good.
Comment #48
dvessel CreditAttribution: dvessel commentedI 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.
Comment #49
webchickNeat.
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. :)
Comment #50
dvessel CreditAttribution: dvessel commentedSure 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.
Comment #51
dvessel CreditAttribution: dvessel commentedWhoops, my editor gave no indication that it didn't save.
Comment #52
webchickI'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.
Comment #53
dvessel CreditAttribution: dvessel commentedEveryone agreed on $classes_array being the array form and $classes being the string in irc.
Re-rolled.
Comment #54
dvessel CreditAttribution: dvessel commentedThere was a little error in building the registry that I missed from the previous patches. This should be solid.
Comment #55
dvessel CreditAttribution: dvessel commentedOnly changed the comments. When I moved some code around in _theme_process_registry() it made less sense.
Comment #56
merlinofchaos CreditAttribution: merlinofchaos commentedOverall 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.
Comment #57
dvessel CreditAttribution: dvessel commentedCool..
If anyone can come up with a better name, please speak up!
Comment #58
JohnAlbinok. 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:
preprocess
functions)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:
Comment #59
dvessel CreditAttribution: dvessel commentedHmm.. 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.
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:
–initial "hook class" added here.
–Hook specific classes added here.
–Modules can add their classes here.
–modules can add their classes here on specific hooks.
–Themes can add their classes here. - This is the first preprocessor that can be used inside the theme.
–Themes can add their classes here on specific hooks. - Another preprocessor named after the engine but specific to a single hook.
- 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.
- 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.
- This is the first "processor" or "post preprocessor" that can be used inside the theme. Most themes *will not* require this.
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.
Comment #60
JohnAlbinIn 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 justprint $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:
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.
Comment #61
dvessel CreditAttribution: dvessel commentedJohn. 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.
Comment #62
dvessel CreditAttribution: dvessel commentedComment #63
webchickFWIW, 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..?
Comment #64
dvessel CreditAttribution: dvessel commentedJust the naming for now IMO. I thought "process" sounded good or we can stick with "post preprocess" for now.
Comment #65
sunFriends, 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
Comment #66
dvessel CreditAttribution: dvessel commentedsun, 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.
Instead of this:
There are some classes we wouldn't want to push through drupal_attributes. Static classes should be exposed right inside the template IMO.
Comment #67
JohnAlbinCould 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:
Just thinking out loud.
Comment #68
sunFirst 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.
Comment #69
dvessel CreditAttribution: dvessel commentedWell, 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.
Comment #70
dvessel CreditAttribution: dvessel commentedre: sun.. Exactly!
Comment #71
sunSo 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?
Comment #72
webchickI'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().
Comment #73
JohnAlbinEclipseGC 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:
Comment #74
alexanderpas CreditAttribution: alexanderpas commented$attributes['class']
Comment #75
eaton CreditAttribution: eaton commentedMy 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?
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.
Comment #76
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!
Comment #77
JohnAlbinFor 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.
Comment #78
JohnAlbinok. So the new fields in core modules have *_process functions that match theme functions. Which means we get a ton of errors like this:
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!
Comment #79
mfer CreditAttribution: mfer commentedsubscribe. need to review this.
Comment #80
JohnAlbinSummary 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 adrupal_render()
in #65-#72. But in #75, eaton describes howdrupal_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 ofdrupal_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 intemplate_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.)Comment #81
JohnAlbinI 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.
Comment #83
JohnAlbinYou know… it would be really helpful if the bot would send an email when it changes the issue status to "needs work". :-p
Comment #84
JohnAlbinUpdated patch.
Comment #85
dvessel CreditAttribution: dvessel commentedThere are a few notes I would correct. I missed this in my previous patch too.
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.
Comment #86
JohnAlbinQuick 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.
Comment #87
dvessel CreditAttribution: dvessel commentedAh, okay. I missed that.
Comment #88
geerlingguy CreditAttribution: geerlingguy commentedThings 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?
Comment #89
Jeff Burnz CreditAttribution: Jeff Burnz commentedThis 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:)
Comment #90
ultimateboy CreditAttribution: ultimateboy commentedWhy are we stopping at just classes? Why not expand this to all attributes - all running through a single function.
Comment #91
dvessel CreditAttribution: dvessel commentedjmburnz, 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.
Comment #92
Jeff Burnz CreditAttribution: Jeff Burnz commented@#91, yeah, apologies, got excited and ran off topic, my bad.
Comment #93
JohnAlbinPer 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.Comment #94
dvessel CreditAttribution: dvessel commentedThanks 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.
Comment #95
dvessel CreditAttribution: dvessel commentedWhat 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.
Comment #96
JohnAlbinThe only difference between dvessel's patch (besides code comments) and my own are the 4 additional lines in
theme_install_page
andtheme_update_page
. I tested the patch and those changes make 100% sense to me, so I'm RTBC'ing them.Comment #97
catchDid some quick benchmarks, no measurable performance impact that I can see.
Comment #98
Damien Tournoud CreditAttribution: Damien Tournoud commentedNice.
Looking at the patch, I'm thinking we should probably remove the old
*sidebar*
classes, and replace them withhas-region-<region>
, but that's out of the scope of this patch.Comment #99
geerlingguy CreditAttribution: geerlingguy commented@ 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.
Comment #100
sunLooks 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.
Just an idea - how about preventing this in the first place and doing the following instead?
So my little themer can simply do:
Thoughts?
Comment #101
dvessel CreditAttribution: dvessel commentedSun, 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:
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.
Comment #102
geerlingguy CreditAttribution: geerlingguy commented@ sun / @ dvessel : +1 to a snippet being posted.
Comment #103
Damien Tournoud CreditAttribution: Damien Tournoud commented+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.
Comment #104
sunHad 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.
Comment #105
JohnAlbinOk, 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:or
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.
Comment #106
sunYes, 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
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:
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.Comment #107
dvessel CreditAttribution: dvessel commentedI 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:
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?
Comment #108
Damien Tournoud CreditAttribution: Damien Tournoud commentedI'm with sun against the magic replacement of class names. This replacement (if necessary) should the responsability of the caller.
Comment #110
JohnAlbinReviewing 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
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
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()
andunset()
). 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?
$classes['class-to-add'] = TRUE;
Remove classes with:
unset($classes['class-to-remove']);
$classes['class-to-add'] = 1;
Remove classes with:
unset($classes['class-to-remove']);
$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.
Comment #111
JohnAlbinComment #112
dvessel CreditAttribution: dvessel commentedAdding 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. :)
Comment #113
dvessel CreditAttribution: dvessel commentedOne 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.
Comment #115
JohnAlbinNeeds themer review
Comment #116
webchickMaking 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.
Comment #117
JacineI 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...
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.
Just thought it was worth bringing up.
Comment #118
JacineRe: #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.
Comment #119
Jeff Burnz CreditAttribution: Jeff Burnz commented@#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.
Comment #120
geerlingguy CreditAttribution: geerlingguy commentedPlease, 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.
Comment #121
dvessel CreditAttribution: dvessel commentedThanks for the feedback.
Just to illustrate the differences directly on *adding* classes:
Keyed classes:
Class as value:
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.
Comment #122
dvessel CreditAttribution: dvessel commentedI'm done with this one. Whatever happens with this one I'll leave others to decide on.
Comment #123
webchickI 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).
Comment #124
Jacinethumbs up! ;)
Comment #125
webchickJacine: 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?
Comment #126
Jacine@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.
Comment #127
webchickGreat! :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!
Comment #128
JohnAlbinThis patch only differs in 2 ways from dvessel's patch in #122:
template_process()
to remove thestr_replace()
since we agreed in #110 and #112 to implement consistent class names in #464862: Add drupal_css_class() to clean class names and rename form_clean_idEverything else is identical to his patch.
Letting the testbot validate this patch, then I'll set back to RTBC.
Comment #129
JohnAlbinAll’s good.
Comment #130
geerlingguy CreditAttribution: geerlingguy commentedI'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?
Comment #131
webchickIn a final look through this, the only kind of weird things I see are:
comment.tpl.php:
block.tpl.php:
Why are $zebra and clearfix not included in the $classes array? Every other template simply does print $classes;
Comment #132
JohnAlbinOh! 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.
Comment #133
webchickSounds 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.
Comment #134
Jeff Burnz CreditAttribution: Jeff Burnz commentedHooray, yes really, thankyou sooo much to everyone who worked this, a great improvement.
Comment #135
dvessel CreditAttribution: dvessel commentedhttp://drupal.org/node/254940#variables-processor
docs added.