Drupal theming nightmares part 2

Welcome to the second part of the Drupal theming nightmares series. Not really surprised by the feedback I've got in the previous post. Most of you were enough lucky to stumble on the same problems. While the post was focused on the theming mistakes, it raised a discussion about unfinished jobs too. So if you haven't read it, it is right here: Drupal theming nightmares part 1.

That day, when I found out what I will be working with, I wasn't able to fall asleep (and it wasn't because of the litres of green tea I had). I was thinking about the person/company that wrote it, whether they are haunted in theirs dreams, do they even care? They should, you should, we all should. Take some responsibility for what you are doing. Do it right. I tend to ask people: "Would architect build a house ignoring physics?" Of course not, if he did, people might die. In our binary world we say kittens might die. Familiar right? But this could be another blog post. Lets move on.

Opening template.php wasn't the best idea to do right in the morning. I felt like entering the Camp Blood on Friday 13th. Literally. Uh, is that a db_query() tearing performance apart? Yay, a garland function (garland_preprocess) hanging around? What the hell are you doing there? Ummm, actual html input elements hiding in the corner, waiting to be called as a replacement for login form? Now I need a chainsaw...

Copy, paste and think

Copying and pasting others code into yours is a very common cause of white screen of death. If you really have to copy and paste the code in yours think what you have done. Try to understand the code line by line. There are hundreds of snippets all over the internet, and they don't have to be right. They might even pose a security issue. Even if it is from drupal.org, it doesn't mean it is always correct.

In our template.php there are copied functions. And there are functions like garland_xxx even though our theme name is elmstreet. In fact these are never going to be called, but what surprised me, they are being called manually from page.tpl.php. Now there are some real fundamentals missing (more on this topic d.o handbook page [Overriding themable output]).

There are also other functions, called like groupdetails(), get_user(),... The name itself doesn't say much and we like to keep it clean. Won't it be better to rename them to _elmstreet_groupdetails? The underscore there is to indicate they are being used internally (in our theme) and to avoid collision with other function names(hooks).

SQL queries

No surprise there! I must admit that I did expect this. Adding sql queries in your theme layer is just evil. It will affect the performance, it will be hard to track, it has nothing to do in theme layer at all. That is what modules are for. No more to say about this. Never ever do this!

Form API misuse

In template.php there is a function elmstreet_mysearchform. It is a FAPI syntax and returns $form array. Hmm, but where is it being called from? Is it a theme override for theme_mysearchform? That will be strange. No, can't see that anywhere. Uh, it is being called from page.tpl.php in the footer region via drupal_get_form.

A little bit too late, no? The page has already printed headers ($scripts, $styles,...), etc... so there is no way for Drupal to attach other behaviours, such as jQuery if needed (collapsible fieldsets,...). Anyways, forms shouldn't be in template.php, that is a module business.

Same applies for drupal_set_message() and functions alike. We have one that is being called somewhere else in node.tpl.php! The actual message will be displayed on the next page reload.

Includes again

In the first part there were some includes already. But this is even worse.
page_xxx everywhere
Now you would think, ok, this is just that they needed a whole page template (including html head,...) for each of the specific pages. I guess everyone of us would be looking for a structure these pages can share and will try to make it consistent. In fact the layout on every page was the same, only few changes to the content area.

The code tells the truth. Top of the page.tpl.php:

<?php $nodenid = $node->nid; ?>
<?php if ($nodenid == '142') { include('page_mpffe.tpl.php'); return; } ?> 
<?php if ($nodenid == '131') { include('page_sca.tpl.php'); return; } ?>
<?php if ($override == 'mpffe') { include('page_mpffe_other.tpl.php'); return; } ?>
<?php if ($override == 'sca') { include('page_sca_other.tpl.php'); return; } ?>
<?php if ($override == 'cppce') { include('page_cppce_other.tpl.php'); return; } ?>
<?php if ($override == 'ankpg') { include('page_ankpg_other.tpl.php'); return; } ?>
and it continues... about 120 lines with includes! ;(

Absolute disaster. There are several ways how to do this right if you need different template files. Either better structure - if possible as mentioned above - or we have this template_files array (or template_file var) in preprocess function. Which allows you to tell drupal which suggestions/files to look for. You can read more about it in the handbook page: Using the theme layer.

Wrapping it up for today

The more I am getting into this theme, the more frustrated I feel. It kind of feel insulting as well. The way they are using the theme layer makes me wonder, whether they asked themselves "Is this the right way to do things in Drupal?" They must think that we are stupid and who would use such a system that works like this? Or is the handbook documentation not enough? Why did everyone of us seen such mistakes?

Anyways, see you in the 3rd part. And please, do share your nightmares. :)

Need Drupal Theming? Hire us