Here is a patch that allows write('foo'); instead of print render($page['foo']);. Reason http://drupal4hu.com/node/290 here.

Files: 
CommentFileSizeAuthor
#105 bartik-node-tpl-php-1158090-105.patch10.71 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 32,183 pass(es). View
#105 bartik-node-tpl-php.txt8.68 KBDavid_Rothstein
#63 var_print_test.patch9.3 KBdvessel
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch var_print_test.patch. View
#35 1158090-render-in-templates-34.txt1.88 KBeffulgentsia
#13 jenlampton.patch6.51 KBchx
FAILED: [[SimpleTest]]: [MySQL] 31,809 pass(es), 3 fail(s), and 0 exception(es). View
#10 jenlampton.patch5.58 KBchx
FAILED: [[SimpleTest]]: [MySQL] 31,578 pass(es), 283 fail(s), and 442 exception(es). View
jenlampton.patch5.33 KBchx
FAILED: [[SimpleTest]]: [MySQL] 31,841 pass(es), 3 fail(s), and 0 exception(es). View

Comments

chx’s picture

The reference in $render_stack[] = &$variables[$info['render element']]; is very unlikely to be needed. Some testing like write('foo');write('foo'); and with embedded templates (essentially, convert node.tpl.php too) is necessary.

Small adjustments require write() to work so we can convert print render($content); . We should do something with hide and show as well. supress, activate, deactivate are some words that come to me:

  supress('links');
  supress('comments');
  write();

how does that look? Edit: I think we rather need to define ALL and then write(ALL); is better understandable than write();.

chx’s picture

Note that the code is written with a strong 7.x focus -- the API is very strictly backwards compatible. If you have called theme_render_template in the past, you are still welcome to do so. All in all, no old code will break as long as they do not have a function called write() however that's an illegal function name because every module should have modulename_ as prefix.

Status: Needs review » Needs work

The last submitted patch, jenlampton.patch, failed testing.

chx’s picture

Also, we need to deal with if ($foo['bar']) (also see #953034: [meta] Themes improperly check renderable arrays when determining visibility) which we could with something like if (!blank('foo')) which function will actually call render into a drupal_staitc and then write can take that and print it.

tim.plunkett’s picture

sub, coming from #953034

pounard’s picture

While I totally agree with the original statement that exposes the fact that no complex data structure should get to the template, I totally disagree with this method.

You are basically creating the exact same methodology by adding an extra code layer that basically call the old one, that's what I call a dirty patch.

render() is almost an alias (with some additional checks) to drupal_render(), you are basically creating the write() function which is a shortcut to render() which is itself almost a shortcut to drupal_render(). Complex data structure is still in the template, you only mask it using this function.

This is 100% code obfuscation, while template should definitely remains a KISS (Keep It Stupid Simple) system, you are basically complexifing it.

Why wouldn't we just downgrade to <?php echo $my_stuff, $this_other_stuff; ?> ? I mean, if I put fields in my nodes, I'd like just to do <?php echo $my_field ?> and if my field is not present, then it just displays nothing.

EDIT: I agree with http://drupal4hu.com/node/290#comment-1521 on a way is that calling a function, weither or not it has one or two arguments, just don't care, it remains calling a function instead of using a variable. You definitely have the right problem, but really not the right solution. The only problem it solves from the begining is the ability to hide some fields in order to render them later (to take the node example) while this is the real problem that has been solved from the very begining by the render() function, the real problem is probably that stuff like fields shouldn't get to the $content variable array in the first place.

Re-EDIT: I think adding the write() method is pretty much what we call code indirection: adding a new layer to solve a problem instead of fixing it in the lowest one. It, in most cases, ends up by spagetti code.

tim.plunkett’s picture

If a template_preprocess adds $variables['links'], and there is also $variables['content']['links'], how will write distinguish one from the other?

jenlampton’s picture

I think write, suppress, activate and deactivate might make sense - but I'm having second thoughts about this approach too. I like that's it's not a huge patch, and maybe it's the first step in the right direction, but...

Pounard has a point that we're just adding another layer of abstraction. It would be *more* words in the Drupal vocabulary - when the problem is that we already have to many.

If what's wrong is that we're getting too far away from
<?php if ($tabs): print '<ul class="tabs primary">'. $tabs .'</ul></div>'; endif; ?>
Why can't we just go back there?

Drupal needs to be a little stupid at the highest levels. That doesn't mean that people who know better can't dive right into the abstraction and get to all the good stuff.

geerlingguy’s picture

I am a huge fan of the Render API, but I do remember there was a point during D7 development when I was completely befuddled by what had happened to the beautiful node and page.tpl.php templates, with all this 'hide,' 'hide,' 'render,' 'hide' madness...

In many ways, I was content with how everything was working in D6 (as a themer), but I've also tasted the sweet, sweet wine of the Render API, and I don't want to go back. In my beginning stages of theming for Drupal, though, I much preferred an "if ($this) { print $this }" than anything more complex.

chx’s picture

Status: Needs work » Needs review
FileSize
5.58 KB
FAILED: [[SimpleTest]]: [MySQL] 31,578 pass(es), 283 fail(s), and 442 exception(es). View

Why, you can. But, even this won't let if ($page['header']) go away because there is no __toBool magic method.

Edit: In PHP 5.3 we can do if ($header()) and use __invoke(). But D7 is 5.2.

Edit2: we can have a function called blank($foo) which is just return ! (string)$foo; and then we can write if (!blank($header)).

Status: Needs review » Needs work

The last submitted patch, jenlampton.patch, failed testing.

bleen’s picture

Re: #6

While I agree that this patch adds (yet another) abstraction and is without question code obfuscation, lets take a step back and think about what is being obfuscated and from whom. To the developer that also themes, pounard is 100% correct and this patch is just adding more complexity. But to the themer, the pure themer, this patch is a great step towards making things nice and simple...

So who is the real audience for this patch? Obviously the answer is both groups, but I wouldn't be too quick to start backtracking on this idea - in my opinion, this patch gets us half way there.

I dont have a good suggestion (yet) for satisfying both groups, but I like that we are talking about it...

chx’s picture

Status: Needs work » Needs review
FileSize
6.51 KB
FAILED: [[SimpleTest]]: [MySQL] 31,809 pass(es), 3 fail(s), and 0 exception(es). View
xmacinfo’s picture

Removing data structure from templates should be done in 8.x only.

For 7.x I would suggest supporting both syntax:

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

and

<?php print $content; ?>

Reason number 1 being the number of books and documentation already using data structure.

moshe weitzman’s picture

One main reason why we introduced render() and friends is to solve the problem where templates are rightfully printing parts of $content all over the place such as fields, taxo links, user picture, etc. Once you do that, you don't ever print out $content. As a result, your template is hard coded. When you add new module or new field, it won't work! It's content never gets printed.

Another benefit is that we get rid of the major security risks opened by encouraging people to directly print bits of $node->field_foo unfiltered data.

This was all discussed at length at #455844: Allow more granular theming of drupal_render'ed elements. Please read that, if you want to express an opinion now. We were quite undecided there, so I'm not surprised to see waffling again, 2 years later. In the end, i think we made a good tradeoff.

And remember that the theme belongs to you. You can easily make a theme that does all its work in preprocess and avoids render() in the template. Nothing wrong with that approach at all.

JurriaanRoelofs’s picture

subscribe

ckng’s picture

yes, this should help pure themer and some end users who happen to tinker with the templates.
subscribe.

Status: Needs review » Needs work

The last submitted patch, jenlampton.patch, failed testing.

idflood’s picture

subscribing

droplet’s picture

it removed render() but added blank(), so we are going to remove blank() in Drupal 9? Probably themer would ask what is blank($string)? it's more complex than render().

$page['foo'] is pretty nice for themer, when they understand a bit of programming, they will try print_r($page) to see what inside this $page.

but if convert all back into $foo, how can I find which one is print footer? comment ? links ?

In fact, hide(), show() concept is more confused themers.

hide($content['comments']);
print render($content);
show($content['comments']);
print render($content);

do you know what you get ??

adrinux’s picture

Status: Needs work » Postponed

Good grief. We're what, 4 months into the Drupal-7 release cycle and you're already claiming these theme system changes are a failure?

1. Few of the books/blog posts/documentation posts required to explain the render api are out there yet.
2. A large number of themer's likely haven't even touched Drupal-7 yet!
3. Every time you change names and syntax it causes confusion to *existing* themers.
4. Every time you change names and syntax it invalidates documentation/books/blogs and makes it that bit harder for noobs to find relevant information (Google has a long memory. I helped someone in #drupal-themes the other day that was about to try a 5.x theme snippet in 7.x, it's just what they found first).

I'm not saying this change shouldn't ever happen, but I really think it's too early in the release cycle to judge the success of these changes, I'd suggest this issue is postponed until at least Drupal-7's first release anniversary. I'm also uncomfortable with the fact this change is essentially happening because of the observations of one trainer, however great Jen may maybe.

Jeff Burnz’s picture

Status: Postponed » Needs work

I am not clear what the issue is here. Its seems like Jen and chx IRC conversation blossomed into a patch and now we're suddenly talking about which word is best to use for a function.

The problem as I read it is that apparently render($foo['bar']) is scary, then the issues drifts off into something about data structures or other such conversations that no designer understands or probably cares that much about.

We have to be careful not to get too high level here, in the early part of this, I know you want to talk about high level stuff but frankly most designers and themers don't know how to participate in this conversation. Those are the people we're trying to help, so we need to discuss the problems with them, directly, get feedback on what real-world-problems they face day to day building Drupal themes.

I appreciate chx code examples in #1, this is a good start. However its too simple and doesn't tell me how I might do something like this:

print render($content['field_image']['#items'][0]['title']);

My point here is that this is a real world designers problem - how to dig into the array and get one bit and print it with no markup. If we can make that more simple then great. For me its pretty simple - just debug with kpr() and Devel, easy. Maybe this is about documentation and that its quite different to D6 so takes time to adjust and learn the new ways - certianly changing this for D8 has to be a paradigmatic shift towards far easier - not just a little bit different if you want to have a big impact. Right now I personally think there is a good balance of difficulty and power.

I have a very good idea of what professional designers find hard in Drupal, and frankly I do not think its render() per se. I think maybe complete beginner might find it scary, but lets face it, if that is scary then we're screwed right from the start - ANY templating language looks scary at first. I remember learning Tal, wow, what a curve. Smarty does my head in. XTemplate I never really bothered to learn and D6 was heaven (imo). D7 and render() is better, very powerful and frankly pretty easy to use once you get your head around it.

So please, step back for a minute and ask themers and designers what they find hard in Drupal theming, I think rocking into changes like this is a mistake, I appreciate the effort but please lets talk about this in more depth first. Its way to early I think to rip into changes, give it six months and some lengthy consultation with designers and themers, then we assess what the root issues are the attack those first.

mfer’s picture

Has anyone done research into what works (rather than opinions) and evaluated other systems to see their strengths and weaknesses? If we are going to change for Drupal 8 we have the time to do some R&D first.

Jeff Burnz’s picture

@mfer - I think this is a good idea, actually lately I've been looking into other systems and how to build themes for them (responding to Dries cta a few weeks back to do look carefully at what other CMS's are doing), right now I am playing around with eZPublish and Expression Engine. I would be more than happy to take on one of those for an in depth analysis. Clearly EE pitches itself really hard at designers (just look at their homepage).

pounard’s picture

Agree with #22, a lot of template engines are being scary. But a lot of them also are a lot simpler, and if I had to choose, I'd go for a better business logic separation and for no data structures in templates at all (but probably variables being configurable for templates, on a per template basis by themers or something like it).

zoon_unit’s picture

This discussion reminds me of the old saying "to a hammer, everything is a nail." As one of those "non-php developers" I can attest to the confusion around the new template. I was just getting used to the code in Drupal 6 and while I can appreciate the new render array, I just don't understand how to find the values I want to print.

The coder's first inclination is to solve the problem with code, but would it not be simpler to just add an extensive help file to all the Drupal core themes that explains exactly how to parse out the render array? I know there is probably plenty of info scattered in the handbook section, but that's the problem. It's so hard to negotiate help in the handbook that many people avoid that option altogether. Put the help right where it is needed: in the theme folder, in the form of a text file or two. It doesn't bloat the theme code, slow anything down, create code updates, break simpletest, or cause debate.

I just want a developer to tell me how I can find the values I want to display on a page, in a template. Why can't the developers collaborate on a very clear document that lays the whole templating system out clearly and concisely? I'd write it myself, but I'm not a developer, so . . . Once you developers "train" newbies in the proper use of render, then we all become better themers and the community is richer for it.

I'm having this problem right now. I'm trying to position the "addthis" module block in a particular location on the page that doesn't have a specific region and I have no idea how to formulate the php code to "print" a block in a specific location. I imagine that the block's output is available as an array somewhere, but am clueless how to access it.

A good theming help file might just be the best and fastest solution. Once generated, it could easily be added to any of the contrib themes as well.

pounard’s picture

#26 has a good point.

Jacine’s picture

We definitely need to simplify things, but I agree with many others here that what's proposed doesn't seem to solve anything. Also, maybe it's possible, and I just don't know how, but how we can change this in Drupal 7 without wreaking havoc on existing themes?

I think it would be nice if the data structure could be simplified, but I can't imagine how that would work. Like Jeff in #22, I'd like to see more detailed examples of going deeper into the structure.

Here are the things that I find to be problematic:

  1. The way variables are printed is inconsistent. There are render arrays, regular arrays and strings, and the main and secondary menus go through scary theme functions, right there in page.tpl.php. It's not a great first impression, and there is really no good reason that I can see for all the differences.
  2. There's no consistent way of editing variables. Some variables are tied to theme settings, others are defined in preprocess or process functions and then there are render arrays which generally need to be modified via alter hooks. This is a hell of a lot to expect the average newbie to grasp.
  3. A lot of the time, what we are looking to change is nested too deeply.
  4. Some variables are entirely out of each in certain circumstances. Just the other day, I was struggling trying to prevent the filter help text from printing on a comment form and failing hard. It wouldn't work in a preprocess, process function, or a form alter, because there is a pre-render and process function manipulating it. The only place it would work was in the template itself. This meant implementing a hook_theme() to use a template for the form just to accomplish a simple hide(). That's way too much troubleshooting and steps to accomplish something that small.
  5. There is too much freedom around rendering. I've run into problems on D7 projects where module developers are freely using hide()/render() all over the place. This is not immediately apparent in the arrays I'm printing out. It's often unexpected, so it's hard to even trust the data at times.

Documenting all these inconsistencies is needed, but it's also incredibly hard to do because it really doesn't make a whole lot of sense.

xmacinfo’s picture

I am a themer and I have already converted two themes to Drupal 7.

I did not do advanced stuff in the themes like I did in D6, though. It might be more complicated in D7 to output only the content of a block or a field directly. But with good documentation, any themer will find how to output his block or fields.

For D7, we need good documentation, not change the template engine again. I don't want to convert again the themes I made for D7.

We should split this issue in two: D7 for the documentation part, and D8 (new issue) for any refactoring.

pounard’s picture

Pretty much agree with both #28 and #29.

chx’s picture

Just a few things: noone said you can't run render any more. We certainly support that. The only thing here, if you use render then you probably can't use the flattened-out variables. But, I can think of ways to fix this so this is a non-issue.

And, notice that although we added blank, it does not use arrays. It rather solves an existing issue while removing arrays. That's never a bad thing.

highermath’s picture

I believe that this is primarily a documentation issue as mentioned in #29. There is nothing in this that requires even basic overall php skills if the process is explained well.

Internally documenting templates, particularly in core themes and base themes in the manner of Drupal core would go a long way towards making theming easier.

xmacinfo’s picture

@chx: I am eager to see what you have in mind. :-)

moshe weitzman’s picture

-        <?php print render($page['sidebar_first']); ?>
+        <?php print $sidebar_first; ?>

Alas, I think this just trades problems. With this patch, folks have no clue that lots of useful bits are hiding within $sidebar_first. The render() call and the name $page and the fact that $page is an array all suggest that more is available there.

Really, all you have to learn is one drupalism, render(), and you are off and running.

FYI, my original patch at #455844: Allow more granular theming of drupal_render'ed elements had the following 2 lines toward the top of each template but we removed them when someone claimed its output is 'too scary'.

+// Uncomment the line below to see what variables are available in this template.
+// print '<pre>' . check_plain(print_r(get_defined_vars(), TRUE)) . '</pre>';
effulgentsia’s picture

I agree with #28: there's a lot that's inconsistent in our templates, and our usage of hook_*_alter(), #process, #pre_prender, and hook_preprocess_*(). It's a natural consequence of parts of the render system having evolved after initial D7 code freeze: we had to solve some specific problems, and then did not take the time to go back and simplify. Much of the simplification will need to wait to D8 (since we can't break BC in D7 any more). For D7, better documentation will go a long way.

If it's true that the existence of render() calls, interspersed inside markup, within template file is raising the barrier to entry to Drupal for designers and themers, then we should try to solve that. If documentation alone doesn't suffice, how about something like this patch (attached as a txt file, since it's not a fully working patch yet)? In other words, we expose to the person editing the template that some variables are not yet ready for use in markup, because the person editing the template may want to break out some of the contents of the variable into a separate variable, for more markup control of that individual piece. That way, we move the scary stuff to the top of the template file, put some comments in to explain why the person may want to customize what's inside that scary bit, and then once the markup begins, everything's nice and easy.

It doesn't handle accessing content that's deep inside the array, but by the time the themer wants to do that, let's assume they already crossed whatever threshhold this issue is aimed at reducing, and at that point, the themer can call render() within the markup as desired.

Again, this issue, from what I can tell, is aimed at making it easier for people who don't need much control of the subparts of a variable. So something like this makes the markup part of the template look like a D6 template. But it also provides a gentle on ramp to the level of control available in D7.

Anyway, curious what you all think, and also looking forward to the next iteration of chx's work.

effulgentsia’s picture

This comment in http://www.chapterthree.com/blog/jennifer-lampton/why-did-creating-theme... is so good, it's worth repeating here:

maybe the increase in requests for theme training is related to the fact that themeing changed a lot in 7, rather than there being some kind on intellectual deficit regarding data structures that themers just can't overcome.

is there any data that suggests people in general (as opposed to the 6 people listed above) have a specific problem with data structures, and that the proposed fix in #1158090: Separate usage of hide(), render(), and other template features from print statements to enable non-developers to create themes will improve things?

i think it would make more sense to test this theory (in a usability testing kind of way) before spending time developing and advocating for adding new features (and hence complexity) to the theme layer in the name of simplicity.

for example, you could provide someone without previous exposure to drupal and present them with one of three page templates - a drupal-6 style page template, a drupal 7-style template, and a new version with the proposed write() function, and see how well each group performs trying to make basic manipulations to the template.

chx’s picture

I find it amazing that people say "Really, all you have to learn is one drupalism, render(), and you are off and running." when the very title of this post is about removing data structures.

Another thing I thought of, we could make these upper case (PHP identifiers are case sensitive) to avoid a clash with existing variables which are almost 100% lowercase or at most CamelCase. Eg <?php print $SIDEBAR_FIRST; ?>.

moshe weitzman’s picture

OK, I was too brief. Learning about render() includes learning about what sort of variable it accepts. This is not new knowledge. You always had to understand this in order to print out bits and pieces of a node. But back then, you also had to know when to add output filtering, whereas render() takes care of that.

Uppercase? Really? I don't see the WTF factor decreasing there.

xmacinfo’s picture

@chx: If I look only at the title issue (Remove data structures from templates), I would either close this issue as won't fix, or move it to 8.x. Moving to uppercase or camelCase would only confuse more people, make books obsolete and require a reconvertion of all current Drupal 7 themes (although at a smaller scale).

As mentioned in @29: For D7, we need good documentation, not change the template engine again.

So lets split this issue in two: D7 for the documentation part, and D8 (new issue) for any refactoring.

Learning about data structure is something any themer can do with proper documentation.

pounard’s picture

Even if I really think that current templates are insane, I vote +1 for #39, plus, D8 will have a lot of structural changes already, let's not confuse at least themers :)

Dave Reid’s picture

I actually find using render() in templates *easier* than Drupal 6. But I'm obviously in the minority. Who is our intended themer audience?

pounard’s picture

@#41 render() is one more level of code indirection compared to using directly variables and echo them. For a developer I agree this is quite simple to understand. The real problem for most people is probably inspect those render arrays (which content is not guaranteed not moving through time by the way, it can be volatile data) it's a painful thing to do. Personally, I always read module's and core code, so what's inside is not a secret to me when I want to know, but most people won't have the time to this IMHO.

mfer’s picture

Three unfortunate things.....

  1. There is no clear or unclear definition of what a themer is. What skill set should we expect them to have or learn? Without some more clarity around this we are going to give opinions based on individual definitions that may vary greatly rather than discussing how we can help enable a common user group. I understand we often don't like to define a group. To try and solve a problem that is not clearly defined will be... problematic.

    Based on what I've read so far in this thread and other places the definition of a themer is someone who knows html and css but not php. The level of php they work with is stuff that makes sense in plain english (e.g., print $foo and if ($bar)).

    If this is the case then render() in templates is beyond the expected skill set. If a Drupal themer is someone expected to know some Drupalisms or more than the definition above render() may not be out of place.

  2. I'm still not sure what's opinion, what's complaining about change, what's legitimate, and what needs more training. Data structures are not new to templates (e.g., Twig and Smarty). Do we expect our themers to be able to be able to work in these other template engines as well if they were working on projects with them? Or, do we have a different expectation? If so, what is it?
  3. Anything we do to Drupal 7 would have to be backwards compatible. So, we are talking about massive changes being in Drupal 8. If this is something we are adding to Drupal 7 someone could start with a module that uses hook_process() to add more to the templates in terms of old school variables or any other backwards compatible thing. This would be a good way to try out ideas, share them, and try them out in the real world. A good place to start is to take the points Jacine noted in #28, turn them into tickets for D8 (in a positive tone of change), and build a contrib module so we can start to get some of this in D7.

I'm completely on board with cleanup for consistency and removing WTF moments. But, the notion of removing data structures from templates all together drives at opinions around an undefined user type where there are a lot of personal unsaid definitions. Wouldn't that make this the definition of a bike shed thread?

Fidelix’s picture

I don't think we should trade practicality or power for easiness.
Whatever the way we can code and deliver quicker, we should stick to that, even if it means a bigger learning curve.

barraponto’s picture

Comment #28 shows several paths for improvement. I am a theme developer and while print $something is a little bit better than print render($array['something']), I don't think that is the main issue. Actually, write($someting) would only mean we drupalers can't do things in a predictable (print) way. In D6 all there was was the print statement, rendering happened in preprocess, and it was good. I guess the best path would be to magically flatten stuff from process functions to template files. Hides and Shows can happen on (pre)process functions, can't they? Shouldn't happen on tpl files.

Now looking at what Jacine points in #28:

  1. Let's flatten it out between the (pre)process function and the template file to make sure there are only strings there. If the theme function requires parameters, let's define them in the preprocess function.
  2. every array (renderable or not) should be modified from (pre)process functions. Of course they can be exposed on object forms or theme settings, but they should accept any modification in (pre)process functions. This has been a WTF that haunted me for a long time in Drupal.
  3. This is because of the render() aproach, I guess. And because some variables are of the same 'kind' (like fields) and take several common arguments. But it sure makes things harder. How to approach this in the theme layer is a good question
  4. hide() can't be called from (pre)process functions? now this is something flawed, isn't it?
  5. and this is just a matter of stablishing the good practices.
zoon_unit’s picture

Holy cow. Do developers have "documentation phobia" or something? In the time it's taken all you guys to debate numerous changes to Drupal 7 core, you all could have written a comprehensive help document to add to all the core themes! Good documentation is the great equalizer. There is no such thing as a "typical" themer. We all have different mixtures of skill sets.

But the ONE UNIVERSAL thing that can help everyone, regardless of skill level, is well written documentation. There is so much "black box" functionality in Drupal that even experienced PHP developers need a lot of time to familiarize themselves with it.

Just write some good documentation.

1) Explain the render object in concise terms
2) Explain the "hide" function
3) Tell us what values are available to themers through the render object
4) Give us specific code examples of how to "render" a custom content field, a breadcrumb, output from a block, a user's information, a "read more" link stripped from content, etc. Try to think of all the classic use cases and give an example of each in code.
5) Place the well formatted text document in the themes folder and watch themers worldwide exhale all at once.

It's simple guys. One semantic way of rendering something in a theme is no simpler than another if you don't know what kind of data is available for access. Conversely, semantics are unimportant once we have the instructions to use the language effectively.

Just do it! :-)

effulgentsia’s picture

Do developers have "documentation phobia" or something?

Many, but not all, developers who choose to spend their free, unpaid, time contributing to an open source project prefer to spend that time writing code and improving APIs. Some people have helped out a lot already with D7 documentation, but more is needed.

Just do it! :-)

We'd love your help. Your ideas above are really good. To help make it happen, http://drupal.org/contribute/documentation.

zoon_unit’s picture

Many, but not all, developers who choose to spend their free, unpaid, time contributing to an open source project prefer to spend that time writing code and improving APIs.

I think that statement gets to the crux of the problem, really. It's natural for coders to want to spend their free, unpaid time, on their specific joy and talent, which is coding. But coding in a community project like Drupal is extremely difficult. First, the code changes must be discussed (as in this thread) and agreed upon. Then it must be assigned, written, tested, debugged, and eventually committed.

Sometimes good, concise documentation can obviate the need for a lot of that coding, thereby freeing the developers to spend their free, unpaid time to produce more productive changes to the software they love and use. Documentation is just a much simpler process and doesn't break existing code.

I'm certainly capable of writing documentation, but the conundrum comes from the fact that the best documentation writers often don't have the specific knowledge necessary to actually WRITE the documentation. That's why developer input is so vital.

That said, I'm going to try and do my community duty and put together a basic THEMING_README.TXT file to use as a starting point if you guys can help flesh out some of the details. I'd rather post it here rather than the documentation area because I firmly believe this is a file that should be added to the Drupal 7 codebase, in the themes folder. PROXIMITY is a key aspect of good documentation as demonstrated by the great improvement in the interface documentation in Drupal 7.

I think Drupal core documentation needs to be considered part of the codebase and should be debugged and improved via git just like the rest of the code.

Give me a couple of days and I'll follow up with something....

effulgentsia’s picture

Title: Remove data structures from templates » Document usage of hide(), render(), and other template features to enable non-developers to contribute awesome themes
Component: base system » theme system
Assigned: chx » Unassigned
Category: feature » task
Issue tags: +documentation

From #29:

We should split this issue in two: D7 for the documentation part, and D8 (new issue) for any refactoring.

Ok, let's do that.
@chx: are you ok with opening a new D8 issue for your work? I'm open to considering whatever comes from that for backporting to D7 if it makes sense to do so.

@zoon_unit: thanks for offering to take the first cut. I'll help review, and I think others here will as well.

effulgentsia’s picture

Issue tags: +D7TX

adding the "themer experience" tag.

[EDIT: doesn't look like there's many issues using that tag. Is there a better tag for this?]

xmacinfo’s picture

@zoon_unit: I'd love to read your first take on documentation. Welcome aboard.

Jacine’s picture

I've written quite a bit about the render API in my theming chapters for The Definitive Guide to Drupal 7. The book hasn't been printed yet, but it's going through final reviews now. I'd be happy to offer some of it up as a starting point if you guys want it when that process is complete. It should be ready within the week.

barraponto’s picture

Issue tags: +DTX

Where would be the best place to document the functions and the objects? If a variable is available in a template file, should it be documented there (as usual)? Should we link to it, like @see core-objects-structures.txt ?

by the way, the D7TX tag is wonderful, maybe DTX would be even better as we aim for Drupal 8.

Jacine’s picture

Issue tags: +Front end

As far as tags go, I'm going to be using the tag "Front end" to organize issues that affect themers going forward. In Drupal 7, we used "markup" for lack of a better component, but didn't work well for various reasons discussed here: #898538: Remove markup, CSS and JavaScript components. Issues tagged "Front end" are also piped out to @drupalfrontend to broadcast them to those interested in helping out. There's not much tagged yet, but I'll be on it shortly. Feel free to pitch in. ;)

dvessel’s picture

It'd be nice to see the discussion moved on to what the render arrays enable. I absolutely love it and I wish everything was a data structure.

I did some experimenting that allows CSS like queries to target elements in the array. (RendElements) There are some issues with that but if it worked and performed perfectly, It'd be pretty sweet IMO. :)

Everyone knows how to use css selectors, right? It becomes less about knowing the exact data structure and more about rules and understanding what's available. Far easier to understand since everyone knows the rules (css, jquery). All you need to know is what is available.

xmacinfo’s picture

@dvessel: This is indeed impressive and I am sure documentation will document what render arrays enable.

As for your RendElements sandbox, I'd love to see this in core. Any CSS-like queries are welcome.

chx’s picture

unsubscribe :(

tim.plunkett’s picture

Why wasn't documentation split off into a new issue? I wanted to see where chx continued with this idea. It's only been 3 days.

chx’s picture

You won't.

dvessel’s picture

Sorry chx, I don't see how your approach makes things easier.

If we want to simplify it and go back to <?php print $sidebar_first; ?> then one possibility is to have it as an ArrayObject and use the __toString magic method. That way the rendering can happen when it's actually printed and not hidden in some preprocess function.

I don't know if that would be another layer of confusion as I haven't thought it through but it should be invisible for most theme devs.

pounard’s picture

#61 Pretty good usage of the PHP magic methods (IMHO) it might be a solution, can be slower, but needs to be tested and measured to be sure.

dvessel’s picture

FileSize
9.3 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch var_print_test.patch. View

Not sure if I should be posting here but here's a rough patch to get a feel of how the __toString would work and doing straight print $vars.

There's a helper function whose sole purpose is to check if the element has content. Works with the array object or anything else. Just enable Bartik and pop open the page.tpl.php file.

One nice side affect is that while you can do <?php print $sidebar_first ?>, you can also access sub-elements with <?php hide($sidebar_first['some_key']); print $sidebar_first; ?>. The __toString goes into effect only when printing the base variable. Sub-elements will have their array keys available and they'll act like any normal render array.

tim.plunkett’s picture

"Just enable Bartik and pop open the page.tpl.php file."
Also, clear the caches if you want it to work.

This is looking really cool.
This makes it even clearer to me, and I already know what's going on under the hood.

More documentation will help, but I do think something like this would be a big improvement.

moshe weitzman’s picture

So how do I, in my template, print out just one block within sidebar_first? Am I supposed to create new vars in preprocess? Thats the two file (and two concept) dance that we avoid with the current solution.

// Gather all render array elements and convert them to RenderArray objects.

It is a bit crazy to put code like this in the theme layer. render arrays need to start as objects.

dvessel’s picture

Moshe, the biggest objection I have to using preprocess and render arrays is rendering them. All this does is prepare it as an object so the __toString can trigger rendering. There is no rendering in preprocess. Either way, I'm fine with not doing this but if it really is a problem for new themers then this is better than a straight render from preprocess.

Rendering in preprocess just kills it for any theme running and we cannot have that at all.

So how do I, in my template, print out just one block within sidebar_first? Am I supposed to create new vars in preprocess? Thats the two file (and two concept) dance that we avoid with the current solution.

You can still do a <?php print render($sidebar_first['menu_devel']) ?> and that $sidebar_first references the parent $page array. I don't think creating the objects from preprocess would be expensive so having that the default output might be reasonable. Then the theme doesn't have to worry about setting it up on its own. All that cpu time should be spent when the actual print happens.

It is a bit crazy to put code like this in the theme layer. render arrays need to start as objects.

Agreed. Would love to see that in 8.

dvessel’s picture

I'm realizing this very late but the later patches by chx has considerable overlap to the patch I created. I was thrown off by the write() and suppress() usage in his earlier patch. A big WTF on my part. Forgive me for the noise. Weird we had a similar solution.

David_Rothstein’s picture

I really like @effulgentsia's patch in #35. If we can't all agree to continue with that one, can we split this into two issues so it doesn't get lost?

What I like about that approach is that by putting the complicated preparation steps at the top of the template file and the HTML parts at the bottom, we have a solution that is (a) completely generic, and (b) 100% backportable to Drupal 7.

But I wonder, instead of this (from the patch):

// Variables documented above as requiring a render() call can be rendered into
// strings here, allowing the rest of this template to be easier to work with.
$render_variables = array(
  // Render $title_prefix and $title_suffix, without separating any subparts.
  'title_prefix' => array(),
  'title_suffix' => array(),
  // Render $content, but separate the links and comments subparts into their
  // own variables: $links and $content_comments.
  'content' => array('links', 'comments' => 'content_comments'),
);

// Do not modify the following line.
extract(template_render_variables($variables, $render_variables));

Couldn't we be even more straightforward and just do it like this?

// Variables documented above as requiring a render() call can be rendered into
// strings here, allowing the rest of this template to be easier to work with.
$title_prefix = render($title_prefix);
$title_suffix = render($title_suffix);
// Render $content, but separate the links and comments subparts into their
// own variables: $links and $content_comments.
$links = render($content['links']);
$content_comments = render($content['comments']);
$content = render($content);

If the top of the template uses render() calls directly (rather than hiding them behind another layer of abstraction), then someone who is ready to "graduate" to the next level will have an easier time understanding that part also.

And I think that making it easy to take that next step is just as important as making it easy to take the first step... We don't just want people to be "successful newbies" but also want to make it so the system naturally helps educate them about how things really work, once they reach the point where they're ready to learn more :)

jenlampton’s picture

Title: Document usage of hide(), render(), and other template features to enable non-developers to contribute awesome themes » Separate usage of hide(), render(), and other template features from print statements to enable non-developers to create themes

I don't think that removing render() would help the "themer". I think professional theme developers will either know how to deal with data structures and new functions, or they'll learn - however hard it is. People like moshe weitzman and Dave Reid and chx can build a really smart theme - there's no doubt about it. And Jacine, I'm positive that you'll figure out all the new theme stuff in D7 too, even if the process of learning is frustrating.

I'm worried about the people who are completely new to Drupal. People who have HTML and CSS and Design skills, and know they can "build a Drupal site" by enabling modules and creating content. These people just want to make their Drupal site look better (and let's face it, Drupal needs some help in the looking-better area). This worries me because our community has been historically hard for HTML + CSS + Design people, and now by making their place in the code even less hospitable to people who don't know PHP (yet) we certainly aren't helping our contributor base grow any.

I agree that we should do some testing on this - but whatever we do we need to do it soon. I don't want to go ~2years turning away potential new "front end" contributors because we've made our themes more awesome for developers.

I'm getting a little tired of hearing that the solution to every problem is more documentation (or more training, which I also hear a lot). I think we can do better than that - by building things that just make more sense! (within reason, of course).

I certainly don't think books being written already is a good reason to hold back progress, but I do think it would be great if the solution we come to wouldn't break anything in D7, and also support simplification - right now. I think we can do that! :) I like this approach in #68, and I also think it might help solve #953034: [meta] Themes improperly check renderable arrays when determining visibility if used correctly :)

xmacinfo’s picture

Version: 7.x-dev » 8.x-dev

As per previous comment.

Please note that any fix to 7.x must me done in 8.x and backported to 7.x.

chx’s picture

MMMMM! #68 is a SANE idea for now. It will scare people a little yes but we can perhaps write good enough text to say "This is a scary part, skip to friendlly HTML below"

Note: Once again David Rothstein waltzed in an incredible complex issue and talked sense as if it'd be the most trivial thing on earth.

Jacine’s picture

I must admit, I hate how #68 will clutter the template files. I also don't think it really solves anything because it just moves the complicated parts around. I figured out D7 over a year ago, but was really just trying to say that it shouldn't have been as hard as it was (and that I still have WTF moments). While all the inconsistencies can be learned, they are not intuitive at all and in many cases they are unnecessary and I think this is where the root of the problem lies.

However, if people think it will help, then I guess #68 is okay as a temporary band-aid for D7. But... I really believe/hope that we can do a lot better than this for Drupal 8.

Jeff Burnz’s picture

#68 and hide($content['field_tags']); in node.tpl.php = Fatal error: Only variables can be passed by reference.

dvessel’s picture

What Jacine said. I don't see how a new themer will have an easier time understanding two parts of a template. They are not supposed to learn about preprocess functions? What this is essentially saying is "here are your templates, don't go any further".?

Making templates easy to understand.. I'm all for it but I don't think we are giving designers much credit. Lose the new conventions and maintain a few key manageable concepts on theming and I think the majority will be okay.

jenlampton’s picture

I don't think a first timer should have to understand both parts of the template file. They will need to learn it eventually, but that level of understanding shouldn't be required for the first template file created. (and I'm not sure they'll understand everything that's in the PHP comments anyway.)

I think that there should be some part of the template file that they can understand - in it's entirety - with little effort. I expect they'll see the part at the top of the template file in the comments, but scroll past it. Then they'll see the bottom part of the file and go "Aah! I get this!" and start changing the HTML, and moving stuff around on the page. I'd like them to be able to have some success with that before needing to understand things like hide, render PHP data structures, and preproces functions.

A little success early on will reward, and encourage people to keep learning more. :) It's not about saying not to go any further - because they'll have to eventually. It's about making that very first step less of a challenge. I also didn't mean to generalize that Designer's won't get it. I'm just worried about all people out there who know some HTML but nothing about PHP.

Agree, David Rothstein++

dvessel’s picture

One of the key ideas of theming is dealing with preprocess functions. Setting a new convention of preparing variables on top of templates only weakens it.

What will templates from contrib or custom themes look like a few revisions in? How complicated will these templates become? Well, core has this thing with placing more complex blocks of code up there, why not go a bit further introducing more inconsistencies?

What is the difference between printing a variable and printing a function call? For the beginner, I imagine the difference is not a big one. It's just something you do knowing you'll get something you need out of there. Eventually you learn the reasoning behind it and it starts to make sense. This will more likely satisfy those who are familiar with older releases than anyone new coming in.

Until it is proven that the current state of templates is too much to handle, we should wait and see.

Nathan Smith wrote a nice article on his work with Drupal 7 and HTML5. He is not new to writing code but he is relatively new to Drupal and I found his conclusions very interesting. This doesn't prove anything but it's something to think about.

Nathan Smith’s picture

I saw this thread via dvessel's link to my blog post. Like he said, I'm not new to code, but am (relatively) new to Drupal, having started with Drupal 6. I ported my site's theme from D6 to D7, so everything I'm about to say can be seen through that lense.

I consider myself first and foremost a designer (slash) front-end developer, and my favorite language is JavaScript. As for Drupal theming, I'll say that the ...

if ($foo) {print $foo;}

... style made sense to me in D6. I was confused, though only briefly, by the render() API being added in D7. But I've never been one to "look a gift horse in the mouth," ya know?

I realize Drupal is open source, and a lot of hard work goes into making it what it is, driven by people whose livelihood is directly staked in its roadmap. For me, as a casual tinkerer who occasionally downloads the Drupal tarball, if someone says "This is the way it works in D7, and has changed from D6," I'm like: "Okay cool, no biggie" and I adjust accordingly.

My philosophy is: To get to the browser, everything has to go through the gateway of HTML at some point. As long as I have control over the markup, be that in a direct manner, or wrestling it back from the CMS, I'm fine. I just see Drupal as just another beast to tame...

http://www.slideshare.net/nathansmith/refresh-okc/22

So yeah, that's my two cents. I like Drupal because it enables me to do a lot of things I wouldn't otherwise be able do. I also appreciate the focus on security and scalability, with things like the Performance area built-in (not an afterthought), enabling me to concatenate CSS/JS, etc.

I'm less nitpicky about ...

print $foo;

-versus-

print render($page['foo']);

... Though I will say that from a front-end standpoint (and having made video game levels in the past, where "render" is a loaded word when it comes to 3D graphics), when someone says "render" (aka "draw"), he/she usually means "cause to exist in the DOM," which the Drupal render() function doesn't actually do. I did a double-take when first reading up on the changes from D6 to D7, expecting the code to read...

render($page['foo']);

... Where "render" would case the content therein to be spat out to the page. It'd make more sense to me if that's what render() did in Drupal, handling: check for existence/!blank, and the printing to the HTML, all in one fell swoop.

But again, I'm just speaking from my own experience jumping back into D7 theming, after having taken a brief hiatus once I felt like I had D6 theming figured out. I guess it would make more sense if everything required a render() wrap, for consistency. To me, it's ultimately a classic case of: "Six of one, half-dozen of the other," because *how* data gets to the HTML isn't what I'm primarily concerned with, just the output itself.

WARNING: UNRELATED...

Honestly, I care less about the nuances of how variables get to the page, and more about usability aspects. Such as, the form generated by the contact module has *no* indicator that an email was sent successfully, instead just dumping the user back out at the root/index page. If it instead posted to the contact page, with a nice user-friendly confirmation message (or at least was able to be specified in the theme somewhere) that would rock. Again, not complaining about freeware, just sayin'. :)

dvessel’s picture

<?php print render($page['foo']); ?> is a bit redundant, isn't it? :P Well, the feedback is appreciated. Thank you.

It would be nice to get feedback from someone without a programming background but to be honest, the most notable "designers" are not strictly designers. Piling on little bits here and there for their sake could end up being more of a turnoff. Keep the list of patterns/conventions/whatever short and document them. Anything new introduced should outweigh the negative consequences of having to understand the how and why it's done that way.

I'm not saying we should ignore those who never touched a line of code but #68 is not a pretty solution and it's out of place.

pounard’s picture

print render is incredibely redundant! Plus, render() is basically used only in themes (it's a proxy for drupal_render() with some more checks). I don't really like the original write() and stuff patch, but I must admit that using just render($something); which is finally exactly the same thing as the write() patch seems nicer to read :)

Jacine’s picture

I know there were reasons why this couldn't happen at the time, but I completely agree with these two statements and this is the direction I'd like to see things move in:

It'd make more sense to me if that's what render() did in Drupal, handling: check for existence/!blank, and the printing to the HTML, all in one fell swoop.

I guess it would make more sense if everything required a render() wrap, for consistency.

pounard’s picture

You can basically use render() for everything since it also handles strings. But yes it lacks some stuff, like the function that basically checks if behing the $something variable there really is something.

While D8 may need to make this consistent, I'd pretty much like that themable objects (meaning functions and templates) being objects, pretty much as Zend views, don't know if it would really be a performance killer (could be a good thing to do some tests and benchmarks) and it would be pretty easy to keep the exact same API for templates themselves (but would probably break the whole preprocess syntax, not the concept, the syntax only). render() would then become a local functions applied to an object (basically a method, let's call a duck a duck) or a proxy function to a local contextual object (the view).

Don't take this as a troll, actual ways of theming works really great today, I don't say it needs to change, I'm just saying it could be a nice thing to explore. I think still today in D7 the differences between functions and templates are still too high and that's somehow a shame (why, but why the pager... dudes!).

Just some ideas here, really not a troll :)

jenlampton’s picture

I agree that print render() is redundant, and I can see how the inconsistency between sometimes just printing and sometimes print-rendering could also be confusing. But even if we replaced every occurrence of print in a template tile with render, I'm not sure it would solve the data structures problem I was originally worried about. Plus there is the issue with if-exists not being properly handled. :/

I do see what you are saying though about adding *some* code to the top of the template files being a slippery slope towards missing the whole point of preprocess.

I think it would be good to find a non-programmer to look at these files. I'm going to ask around and see if I can get some more feedback tomorrow.

David_Rothstein’s picture

Issue tags: +needs backport to D7

A couple of points:

  1. The decision to put this kind of logic in template files rather than preprocess functions wasn't something we just invented here; that was decided two years ago in #455844: Allow more granular theming of drupal_render'ed elements after lots of discussion (and note that the other alternative was almost certainly process functions, not preprocess).

    While it may make sense to reopen that debate in Drupal 8, I don't think this is the issue to do it in. From the very beginning, this issue has been focused on coming up with something that could be backported to Drupal 7. And since we want whatever improvement we make here to apply not just to particular themes but also to the default template files (e.g., modules/node/node.tpl.php), I'm having trouble seeing how we can do that using (pre)process functions in a truly backwards compatible way...

  2. I can sort of see the argument that even though the logic is already in the template files, moving it to the top of the template file will imitate a preprocess step a bit too much. However, I think we can solve that very easily with (inline) documentation, explaining why we do this part in the template file and why more complicated things should be done in preprocess instead.

    In fact, moving the code to the top of the template file gives us a standard place to add such a code comment, as opposed to the current situation where we have hide() calls and other bits of programming logic scattered throughout the template haphazardly.

  3. Regarding print render($page['foo']) versus render($page['foo']), that's also an issue that was discussed extensively in #455844: Allow more granular theming of drupal_render'ed elements and there are pros and cons to each side, but it's already been demonstrated there that render($page['foo']) by itself won't work because it cuts off some use cases, so you would need something more complicated if you want render() to print things also... read the issue for details.

    Anyway, as Jen said, that whole debate is only tangentially related to the current issue. (Also, I don't see any way it's backportable to Drupal 7.) So I'd suggest creating a separate D8-only issue if you want to reopen that discussion.

  4. Note: Once again David Rothstein waltzed in an incredible complex issue and talked sense as if it'd be the most trivial thing on earth.

    I'm honored and humbled by that comment, but just want to point out that @effulgentsia came up with about 90% of the proposal; all I did was tweak it a tiny bit... Which also means that if it turns out to be a terrible idea in the end, I only want 10% of the blame :)

emmajane’s picture

I'd like to go back to my recommendation(s) in #455844: Allow more granular theming of drupal_render'ed elements which is to put "hard" stuff into the template.php file (leaving themes with control, but making it easier for newcomers to manipulate only HTML to rearrange tpl.php files...and also the recommendation to be consistent in how render() is applied. In other words: apply it to everything, or nothing. But it sounds like we're too late for that opinion to work in D7, eh?

I agree with the complaints that teaching data structures in tpl.php files via documentation is at best very, very difficult. If you can convince someone to show up at training, it's "trivial" to explain which variables need to be rendered. If you're expecting them to read documentation...well...um...

Can someone please remind me why it's a problem to move rendering of page regions to template.php per theme..or at least create variables for the regions that use the region name instead of digging it out of an array?

dvessel’s picture

Rendering from template.php takes away control over how the data is pushed out. The render array remembers its state on what was rendered so when you're in the .tpl.php file, having a pre-rendered element makes it hard to pull bits and pieces from that array while maintaining the state of what's been pushed out.

template.php example:

function THEME_preprocess_page(&$vars) {
  // Render the content.
  $vars['content'] = render($vars['page']['content']);
}

page.tpl.php example:

<!--
  Basic print. Not a problem... -->
<?php print $content; ?>

<!--
  ...but what if I want to pull a specific bit of content? -->
<?php print render($page['content']['system']); ?>

<!--
  Since the 'content' was already rendered way too early, this will completely
  disregard the above render of 'content > system' and output it again. -->
<?php print render($page['content']); ?>

<!--
  This wouldn't work either since hiding only works before the parent element
  is rendered. Even if it did, it's ugly. -->
<?php hide($page['content']['system']); ?>
<?php print render($page['content']); ?>

<!--
  Only way to work around it is to reset the state by clearing the printed
  state and accumulated child renders. In the end, it's ugly and you're
  rendering multiple times for the same elements. -->
<?php unset($page['content']['#printed']); ?>
<?php unset($page['content']['#children']); ?>
<?php print render($page['content']['system']); ?>
<?php print render($page['content']); ?>

The rough patch I posted in #63 and the later patches by chx does work around the issue. As long as the rendering happens from the .tpl files, it's all good.

emmajane’s picture

Okay... But in most themes you will either (1) want that control or (2) not. Bartik and other core themes don't need that level of site-specific control because they are generic themes. So, again, why can't we move rendering to template.php?

eaton’s picture

I'm generally in favor of this. Way back when we were discussing the possibility of render(), I was very much in favor of using a generic function like p() to encapsulate 'output this thing I am giving to you, regardless of what it is'. The addition of render() all over our core tpl files has given guru themers some useful power tools, but they've come at the expense of template complexity and conceptual overhead for newcomers.

dvessel’s picture

Because core themes are not touched. For most, they will not matter. If they are touching core themes, well you know how it goes. :) Where it will be experienced is in the core or contrib provided templates since they are copied into their themes.

John Pitcairn’s picture

subscribe.

dvessel’s picture

@eaton, what do you think of using __toString from a class to handle rendering. For the obvious areas of the template, doing a straight print will trigger the rendering. If the render array is an object to being with as Moshe suggested, that could simplify (in appearance at the least) of these template files.

David_Rothstein’s picture

I'm pretty sure @emmajane is right. There is no reason any individual theme (whether core, contrib, or site-specific) can't do all its rendering in template.php if it wants to. I don't think #85 is a concern because the theme itself would always pre-render exactly what it planned to use, right? In other words, you would only put something as simple as $vars['content'] = render($vars['page']['content']) in your theme's template.php file if print $content was all you were using in page.tpl.php. If you wanted something more advanced in page.tpl.php, your template.php would also get more complicated (pre-rendering everything you planned to use into separate variables). It's your job as a themer to keep them in sync.

However, again, the problem is that it only works for template files that the theme is overriding, not for default template files (like modules/system/page.tpl.php), right? We could certainly go through core and make all of Bartik's, Garland's and Seven's template files completely clean (by moving all logic into template.php), but I still don't think there's any (backportable-to-D7) way to do it for the default templates, is there?

So then the question becomes, would you rather have the core theme templates be beautiful (but inconsistent with the default templates), or would you rather have everything in core be mediocre but consistent? :) I'm operating under the assumption that since we're talking about educating new people here, consistency is better, but maybe others feel differently.

effulgentsia’s picture

We could certainly go through core and make all of Bartik's, Garland's and Seven's template files completely clean (by moving all logic into template.php).

I don't think we can. If you render a top-level variable in a preprocess function (or even a process function), then you've removed the ability for a subtheme to render a subpart of that variable separately. It's fine for a site custom theme to choose to not be an easily extendable base theme, but I don't think we'd want to limit our core themes like this.

bleen’s picture

So then the question becomes, would you rather have the core theme templates be beautiful (but inconsistent with the default templates), or would you rather have everything in core be mediocre but consistent? :) I'm operating under the assumption that since we're talking about educating new people here, consistency is better, but maybe others feel differently.

Since many (most?) newcomers will take Bartik, duplicate it and start hacking away to create their custom themes, I would actually err on the side of making the core theme tpl files as clean as possible, even at the expense of consistency with the default tpls. No newcomer is ever going to see the default page.tpl.php - (s)he is going to rely on the one in Bartik

== my 2¢

dvessel’s picture

I don't like the idea of core themes being the goto themes by copying or sub-theming (something I missed initially). There's a lot of other baggage they have to manage and it ends up being more complex than what we are discussing here.

emmajane’s picture

@dvessel re. #94. Whether we like it or not, that's how lots of folks learn: they copy the primary theme from core and start bashing on it to try and make it into what they want. Surely the number of vaguely Garland sites out there is proof enough that people do it? :)

@David_Rothstein re. #91. Right now I'm leaning towards inconsistency:
a. Leave the default tpl.php (per module) files with render(). Improve inline/header documentation to explain which vars need to be rendered (currently missing from the tpl.php inline docs).
b. update core themes so that rendering is moved into the template.php file.

dvessel’s picture

@emmajane, I have no doubts about that. Doesn't mean we should encourage it further. There are enough unanswered forum questions other themers wouldn't touch with a ten foot pole. :p

eaton’s picture

@emmajane, I have no doubts about that. Doesn't mean we should encourage it further. There are enough unanswered forum questions other themers wouldn't touch with a ten foot pole. :p

I'm a little confused about the direction the conversation has taken (ie, base themes and reusability). As I see it, the real problem boils down to a couple of intersecting facts:

  1. Right now, our current core tpl files are littered with print(render($foo['bar']['baz'])); style calls.
  2. In theory, this means that themers can slice and dice their data into rendered HTML any way they'd like.
  3. In practice, it means that tpl files now expose people to far more of Drupal's "data guts" than we used to in D6.

This patch helps things out a bit, by introducing a function that simplifies the print(render($foo['bar'])); structure into write($foo['bar]);. The question of whether template.php or the tpl.php files themselves should bear the responsibility of chopping things up and reshuffling them, though, is still kind of floating around. D6 erred on the side of using template.php, and I can definitely say that the learning curve of that approach was easier for all of the people we were teaching when we did introductory theming classes.

Whether they're going to directly copy Bartik's tpl.php file, or the core one, feels like a different question. The heart of the matter is that our new way of doing things in D7 is more confusing for people who aren't already neck deep in nested arrays. This patch is one way to starting to tilt things back, but there are still outstanding concerns about the division of responsibilities in the theming layer.

Am I missing anything?

David_Rothstein’s picture

@eaton, that's mostly it, but you're referring to the original patch, and a lot has happened since then :) I believe the consensus on the original patch was that this:

-      <?php print render($page['content']); ?>
+      <?php write('content'); ?>
       <?php print $feed_icons; ?>

is not really going to help, for a number of reasons (extra layer of abstraction, still has a random function call, still has random strings mixed with variables, etc).

Instead, what we all really want is this:

       <?php print $content; ?>
       <?php print $feed_icons; ?>

and the rest of the discussion has been about whether it's possible to do that in way that (a) can be backported to D7, and (b) still allows the full flexibility that render() provides.

****

@dvessel, in #90, asked about the __toString() method, which was one of the alternatives and which does meet the goal (for the above code example). However, I thought we moved away from that earlier in this issue because doesn't it mostly push the problem one level deeper in the array? It lets you do print $content, but any code that goes deeper would still need to be print render($page['content']['section'][...]), right? Or anything that uses hide($variable['foo'])...

So it does mean fewer occurrences of arrays in the template files, but newbies would still encounter them in some very popular places.

David_Rothstein’s picture

@effulgentsia (#92), good point, totally forgot about subthemes. I guess that probably puts a damper on the idea of having core themes move this stuff back to template.php (in D7).

It actually might still be possible, though, by using different variable names? For example if template.php did this [edit: fixed stupid mistakes in the code example]:

$content_copy = $variables['content'];
$variables['comments'] = render($content_copy['comments']);
...
$title_suffix_copy = $variables['title_suffix'];
$variables['title_suffix_rendered'] = render($title_suffix_copy);

But that gets ugly pretty fast.

eaton’s picture

@eaton, that's mostly it, but you're referring to the original patch, and a lot has happened since then :) I believe the consensus on the original patch was that this:

Thanks, I realized that I read the original patch's code, but was sticking mostly to the discussions in my attempt to catch up.

@effulgentsia (#92), good point, totally forgot about subthemes. I guess that probably puts a damper on the idea of having core themes move this stuff back to template.php (in D7).

I'm not convinced, honestly. If we assume that the decisions about what to slice and dice, data/markup wise, are part of the design decisions inherent in a given theme, I think it makes sense to allow more of that work to live in template.php. The problem that we face now is that we've turned a lot of stuff like the overal page and the node itself into elements that can't actually be rendered without those decisions being made. IE, render($node) will result in bizarre markup, so we have to make some of those decisions in core's default tpl.php files now no matter what.

Is that an accurate summary?

dvessel’s picture

@effulgentsia (#92), good point, totally forgot about subthemes. I guess that probably puts a damper on the idea of having core themes move this stuff back to template.php (in D7).

I'm not convinced, honestly. If we assume that the decisions about what to slice and dice, data/markup wise, are part of the design decisions inherent in a given theme, I think it makes sense to allow more of that work to live in template.php.

That's true for very custom themes but less so with core themes as they are fairly generic. Sounds like I'm contradicting myself but doing all the slicing in preprocess only makes sense in very focused themes. Not the real question though as you've mentioned.

The problem that we face now is that we've turned a lot of stuff like the overal page and the node itself into elements that can't actually be rendered without those decisions being made. IE, render($node) will result in bizarre markup, so we have to make some of those decisions in core's default tpl.php files now no matter what.

I'm not sure if I am reading you correctly. The decision in core's default .tpl on how elements are split-up and rendered is not a problem. The default markup and assumptions on how the render array is split is done to fit most circumstances. If it needs to change, you override it. If the rendering is done *before* those design decisions are made (preprocess), we end up crippling the render function.

David_Rothstein #98 is where we're at. We're also not sure if "print render" is a big problem to begin with.

John Pitcairn’s picture

Thinking back to my time as a total Drupal theming newbie (not so very long ago), I found being insulated from Drupal's data structures extremely helpful while getting started, even though my php skills were not too shabby and I was certainly familiar with nested array syntax - the D6 approach where everything in the .tpl files is a simple variable is immediately quite easy to grasp and work with. And you just work with what you're given. I also really appreciated the Zen theme's documentation of available variables in a php comment at the start of the .tpl file.

It was the next part of the curve I found quite steep - moving from printing simple variables in the .tpl file to actually working with the underlying Drupal data structures and populating those variables, using preprocess functions and theme overrides in template.php. The main difficulty here was finding the correct function to use/override, then finding good explanation of those functions, and decent examples of things done the right way (as opposed to dirty code in old forum posts). Then figuring out why it's done this way.

Before I understood, there was a huge temptation for me to just start working with the data structures in the .tpl files, and wrap my own html directly around raw data using this sort of thing:

<?php if ($page_node = menu_get_object()): ?>
  <?php if($page_node->field_fieldname[0]['value']): ?>
    <div class="foo">
      <?php print $page_node->field_fieldname[0]['value']; ?>
    </div>
  <?php endif; ?>
<?php endif; ?>

With all that mostly behind me, I believe .tpl files should not expose Drupal's underlying array data structures at all. Discourage the idea that you can go hacking around with the data structures in there.

And print render($content['foo']['bar']['baz']) is awful. It's both a confusing combination of similar terms and a complex-looking nested-array statement for a php beginner.

So I agree with #98. Just print simple variables in the .tpl file if possible. If not print, then a wrapper function like write() would be acceptable if it's used for everything.

But #92 gives me pause:

If you render a top-level variable in a preprocess function (or even a process function), then you've removed the ability for a subtheme to render a subpart of that variable separately. It's fine for a site custom theme to choose to not be an easily extendable base theme, but I don't think we'd want to limit our core themes like this.

So you can't be an easily extendable base-theme and have simple designer-friendly .tpl files that just print variables? If you want subthemes to be able to render child elements you must make your render() calls in the .tpl file? Ack, I'd missed that implication. Doubleplusungood.

eaton’s picture

I'm not sure if I am reading you correctly. The decision in core's default .tpl on how elements are split-up and rendered is not a problem.

Ah, I don't think I explained myself well. I don't mean that the specific decisions about what gets treated independently are problematic, just the decision to put all of that decision-making on the tpl files. The actual syntax for addressing those different content chunks in the tpl.php files is pretty heavy, and actually using the render() functionality requires solid understanding of the various renderables that are used to construct the page. That's where I've seen most newcomers get hung up; things went from 'a list of variables you print' in D6 to 'syntax-heavy data structures and assorted functions you call' in D7.

There are plusses for advanced themers who want to do their chunking in the tpl.php files rather than the template.php preprocess function, I definitely understand that.

The default markup and assumptions on how the render array is split is done to fit most circumstances. If it needs to change, you override it. If the rendering is done *before* those design decisions are made (preprocess), we end up crippling the render function.

Yeah, markup isn't the issue here, pure HTML has always been relatively easy to muck with. It's the way we split up variables, how we handle data structures, and how much of the logic bleeds into the tpl files as opposed to preprocess functions.

I suspect that I'm dragging the issue off of its original topic, and I don't want to do that, just trying to make figure out whether the issue/patch gets at the deeper problem that I've been seeing and hearing about from a few of the Drupal neophytes I'm working with.

dvessel’s picture

@eaton, I knew I misread, but yeah. Disregard the mention of the markup.

Sorry to sidetrack this a bit further but this is related to my previous comment. I'm retracting what I said about the assumptions on how the default .tpl handles render arrays. It's not exactly okay. The hide() and show() functions limits control, i.e., focused render from page.tpl for node.tpl comments or links. That will always be the case when you have a wrapper template trying to pick out an inner one which uses hide/show.

I'd imagine the hide/show functions would be more confusing than a straight render call for beginners. Lose lose situation?

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
8.68 KB
10.71 KB
PASSED: [[SimpleTest]]: [MySQL] 32,183 pass(es). View

So far the only solution in this issue that is 100% backportable to D7 and doesn't have any known side effects is the one described in the issue title. However, we haven't yet had a patch for that, so it's been hard to judge how well it will work.

I therefore decided to go ahead and write an initial patch, using one core tpl.php file (Bartik's node.tpl.php) as an example, so we can see what it would look like if we started converting other tpl.php files to this pattern. (Note that an important part of this effort also involves reorganizing the documentation of the variables at the top of node.tpl.php so it makes more logical sense and is easier to follow.)

I've attached my first pass at this both as a patch and as a copy of the (complete) Bartik node.tpl.php after the changes have been made. Based on everything I know about how people new to Drupal tend to read code that is unfamiliar to them, I really think this will be a huge improvement over the current situation.

What do you all think?

markabur’s picture

I still need to read all of this thread, but it sounds like a good thing. I'm an advanced themer/developer and found this issue because I was confused about when to use render(). E.g. print render ($page['help']) is fine but print render($page) causes a WSOD. (So, apparently some arrays are renderable while others are not. Good to know. I wonder what the difference is?) Yet print $title works fine while right next to it we have $title_prefix needs to go through render()... Similarly, $action_links needs to be rendered, but $feed_icons does not.

I suppose a lot of this is me not really understanding render arrays as well as I thought I did; nonetheless, if the tpl was tidier that would be helpful. The approach in #105 makes sense as far as that goes, and I like that it gives an opportunity to document the variables that are used down in the output area.

jenlampton’s picture

Wow!! I think the documentation + use of basic variables make this page *so much* easier to use. I'd like to roll similar patches for other templates and use them in my class next week to see how it files with people unfamiliar with Drupal. I'm also curious to see what others think.

sun’s picture

Status: Needs review » Needs work

I'm sorry for chiming in late, but this doesn't look and feel right to me.

The proposed resolution is a preprocess after a preprocess and process, in a template file.

To me it looks like a certain problem has been identified, and the shortest path to a solution is attempted. It's trying to hide away complexity without removing it entirely. Actually adding more complexity by adding yet another step in template rendering that people need to care for, evidenced by the amount of additionally required documentation. And without solving any of the other related issues that came up around renderable arrays in templates (e.g., checking for emptiness, access, etc.).

It also looks like a lot of people in this thread already stated that the identified problem is rather an issue with consistency, not complexity.

Quite potentially, part of the problem is also the rather developer-centric documentation that's using a lot of Drupal lingo without providing a pointer to a handbook page describing common terminology and potentially including usage examples.

David_Rothstein’s picture

Status: Needs work » Needs review

The proposed resolution is a preprocess after a preprocess and process, in a template file.

That's not the proposed resolution; that's what the code already does. The proposed resolution is just to improve the code organization.

To me it looks like a certain problem has been identified, and the shortest path to a solution is attempted. It's trying to hide away complexity without removing it entirely.

Yes, that is exactly the goal :) We want to backport whatever comes out of this issue to D7. So far, no one has come up with any other solution that can be backported to D7 without unacceptable side effects.

Quite potentially, part of the problem is also the rather developer-centric documentation that's using a lot of Drupal lingo without providing a pointer to a handbook page describing common terminology and potentially including usage examples.

Agreed that is definitely a problem, though not sure it is the same problem we've been discussing here so far.

c4rl’s picture

Subscribaroo

xjm’s picture

I strongly agree with the fundamental concern here: that templates are far less scannable than they were in previous versions, and that they are correspondingly more difficult for non-developers to understand. The difficulty that non-developers have with basic PHP structures--particularly arrays, objects, and the seemingly arcane distinction between them, but to a lesser extent function calls and assignments as well--becomes quite apparent when one spends some time in #drupal or #drupal-support in IRC. Making templates more readable makes drupal more usable and friendly to a wider audience.

The solution in #105 has pros and cons to my mind. A pro is that it is completely backportable and does not interfere with any existing functionality. A con is that it's a "manual" sort of change; every individual theme would need to switch over to this pattern for maximum benefit.

However, I believe in iterative improvements, and since the method proposed adds trivial overhead and alters no APIs, I give it a +1. While there may be a better way down the road in D8, I don't think we should throw out a decent solution because we don't have a perfect one.

klonos’s picture

wow!!! I just came from #997408: $tabs is always set and #953034: [meta] Themes improperly check renderable arrays when determining visibility and I have to say... what a handful this was! I spent the last couple of hours reading this entire issue and I must have paused a couple of dozen of times along the way in order to check out things/terms/issues mentioned or referenced here. A lot of great minds trying to figure this out. These kind of issues and how we handle them is what makes me love d.o and its community more and more.

...still, -having merely a rough idea of what we are trying to achieve here- I have to say that I'd really love it if things were as simple as

<?php print $content; ?>
<?php print $feed_icons; ?>

...but we still maintained backwards compatibility and at the same time allowed people with more advanced skills to take advantage of the full potential of render() -for example-.

Note: ...this comes from a guy that is still going up and down the "I kick ass" threshold, but has spent a considerable amount of time in the "I suck" threshold before that. What I mean to say is that perhaps it is too late for people like me to benefit from things getting simpler -since we're in too deep now-, but I fully understand how much this simplification would help newcomers. I'm not sure we're aiming for that group of people though. It would surely increase drupal adoption rate.

OTOH drastically changing things now (in d7 I mean) does seem unfair for everyone that has spent a great deal of time and effort either reading or writing documentation/books.

Just my 2 cents here. Thank you all for the love you put in this.

droplet’s picture

haven't follow this issue a while and now looking at last patch #105. I don't see it easier for themer, it add more DOCs & per-defined $vars only?

I just took one line :

 <div id="node-<?php print $node->nid; ?>" class="<?php print $classes; ?> clearfix"<?php print $attributes; ?>>

compare to Wordpress:

<div id="post-<?php the_ID(); ?>" <?php post_class(); ?>>

1. Drupal mixed object ($node->nid;), variables ($classes).
2. in WP, very clearly, adding newClass to it post_class('newClass ')
3. Inconsistency!! in Drupal, we print $classes, it doesn't include tags, but $attributes does.
4. do it able / easy to extend $attributes in tpl?

Question I always myself during theming:
1. How to print contents (contents, comments, ...etc) ?
2. How to extend it (eg. add / change $attributes)
3. Able to hack it from theme? Should I write a module ?

barraponto’s picture

@droplet I guess this is not the place to criticize Wordpress, but it runs a function in the template for the id, and another one for the classes. At least Drupal uses just one function: drupal_render(). And we are trying to figure out a way of getting rid of it in the .tpl.php file (since we have template.php and preprocess_functions).

I agree that classes should be treated as every other attribute. It would make things clearer, though frontend developers might find it harder to add their own classes (and then they would put them where they should, the preprocess functions).

As for your questions: each of the variables available in a .tpl.php file are documented at the top of it. They can be changed/extended in preprocess functions, there is one for each tpl.php file. Which one to use or study are also documented at the top of each .tpl.php file. Say, if you want to change something that gets printed in node.tpl.php you can write a function in template.php (the one in your theme) called YOURTHEMENAME_preprocess_node.

moshe weitzman’s picture

FWIW, I'm going to discuss why D7 does it the way it does in my BADcamp presentation - http://2011.badcamp.net/program/sessions/page-render-drill-down-drupal-7. We can discuss the tradeoffs at the end of my session.

jenlampton’s picture

tagging for learnability

xjm’s picture

xjm’s picture

#1390576: Remove empty HTML tags and #997408: $tabs is always set have been closed as duplicates of this issue.

JamesK’s picture

There are so many issues related to this topic, and I've tried my best to consume them all, but I may have missed it if this has already been mentioned.
BTW, I'm coming at this from the point of view of a PHP developer who spends a LOT of time doing theming.

Instead of adding more layers, why don't we:

  1. define a good template variable naming convention so that people know which variables are strings, which are rendered HTML output, and which are renderable arrays
  2. move all the code containing renderable arrays, like print render($page['content']); into theme_process_*() for core themes

So in the process function we create a new variable like $variables['rendered_page_content'] = render($variables['page']['content']); and in the .tpl.php we have just print $rendered_page_content;
IIRC, when the new render system was introduced, this was the recommended way to appease all the people who wanted to avoid having PHP functions in their .tpl.php files.

jwilson3’s picture

#120 sounds like the logical next step for abstraction after David Rothstein's suggestions in #68. I like it.

David_Rothstein’s picture

I think this was already discussed above - see #91, #92, and #99 which explain why this approach would have some difficulties (at least for Drupal 7).

David_Rothstein’s picture

Starting in the next couple of weeks or thereabouts, I'm hoping to have more time to dedicate to Drupal core, and I've been considering whether or not to pick up work on this issue again.

I believe the general trend of the comments since I posted my patch last June has been "lukewarm enthusiasm". So I kind of want to make sure that if I do work on it more, that will translate into the patch actually getting reviewed and tested. Although this change is conceptually simple, it will be a fair amount of work to convert and document all of core this way and then test to make sure we didn't break anything.

There's been a lot of enthusiasm recently (in other issues) for a major rewrite of the entire theme system and that's a good thing, but Drupal is a software project with hundreds of thousands or millions of websites actually using it, so there's something to be said for incremental improvements like this one too, especially ones that can definitely be backported to Drupal 7. So I hope some of the enthusiasm for improving the theme system carries over to this issue, because if I decide to commit a lot more time to this patch I don't want to be the only one working on it and then have it die :)

That said, I took a quick look through core now and maybe it's not quite as much work as I feared. In Drupal 8 currently, there are 42 tpl.php files. However, it looks like only 18 of them actually call render() or hide() anywhere within. So for this issue, we mostly would only have to touch those 18 files.

JamesK’s picture

Re #122: The previous comments regarding moving rendering to template.php (other than #99) are suggesting doing it in a 'destructive' way, but #99 is really exactly what I was thinking as well. Do it in a 'non-destructive' way so that the arrays can still be modified and re-rendered down the line if desired. A good variable naming structure would be absolutely necessary to keep this readable though.
I personally think that the extra code in template.php is still less messy than having render() in the tpl.php files for core themes.

jenlampton’s picture

Version: 8.x-dev » 7.x-dev

@David_Rothstein, I applaud your enthusiasm for this issue, and would love it if you start to have time to work on it :)

However, I propose that we move this issue to the D7 queue, since we are working on #1499460: [meta] New theme system for D8, so the point there may be moot. I'd still like to see a solution that will help people use drupal *today*, and I really like the patch in #105.

xjm’s picture

Version: 7.x-dev » 8.x-dev

In order to fix it in D7, we need to fix it in D8 first so there isn't a regression in the interim. Eventually #1499460: [meta] New theme system should replace it, but let's come up with a backportable solution for D7 here in any case.

JohnAlbin’s picture

Version: 8.x-dev » 7.x-dev
Issue summary: View changes
Status: Needs review » Needs work

And back to 7.x if anyone really wants to take that on. :-p

Otherwise, I'd close (duplicate) this.

Fabianx’s picture

Status: Needs work » Closed (duplicate)

I don't think anyone wants to take this on for core, so closing as duplicate of new theme system with Twig ...