And yet again, took a job that someone else started long time ago, although I did say that I won't do this ever again. "It is almost finished, it needs only few tweaks there and there and we are done." they are saying. I am staying calm, due to time pressure (and the fact that I had to log in to a windows xp machine through vnc - actual server), throwing down some estimates without looking at the site build. Easy tasks you would say, add a border there, fix margin, login box there and mostly styling issues. Nothing I haven't done before.
They wanted to launch the site as soon as possible and of course, only these little things needed to be done. It's ok, shouldn't take me longer than half a day and surely it will help them. So they signed off the budget and we were ready to fix few simple issues and get it out. But...
What the... ???
It was a quiet morning, that morning when you have your breakfast first and then turn on your computer (in that order - not many of these days I have). Relaxed, looking at the bug list, logging into VNC, downloading the site build, installing the site on my local machine, hitting ctrl+U in firefox to see which theme we are using and where it is located.
Slapped for the first time - theme in /themes
Nothing I am not used to, theme - lets call it elmstreet - is in /themes folder, right in the root of the Drupal installation. Thinking that this company might have its own Drupal stack like we have, with common base themes used, but this doesn't look like a common base theme, but site specific. So why is it bad to have a theme in the /themes folder in the Drupal root?
Simply put, upgrades. Would you like to overwrite your theme every time when there is a new Drupal x.y version? Hell NO! We have a place where themes are being stored - sites/all/themes, sites/default/themes,... If you don't know anything about this, go on and read something about Drupal's multi-site approach: Drupal.org | Setup of /sites directory for multi-site.
Yikes, the moment of truth
Browsing through the file system and locating the theme in a command line, entering the folder, typing ls command (to see what files are in). Wheeeeeeewww. What was that? The contents of the ls command result was almost 5 pages long. That is probably first time I have noticed there is something wrong. The site was a simple site right? Did they forget to tell me something?
Just so you can imagine, I made a picture in Nautilus. Does this look right to you (btw there are about 103 files)?
That was the moment, when you slowly start to panic and think, oh, either I don't want to be involved in this or I will have to rewrite it. So I went back to the client, there is no way to convince them to pay extra money on rewriting the whole theme which does and I repeat does work, that's what they've said. Common answer in these cases. Contract signed off, shall I just do what they are asking for and leave it as is?
Nope, I'll rewrite it and use it as a reference/examples for blogposts/presentation. Good plan. Maybe these who wrote the code will read this and learn, and next time, I or some other themer won't have to deal with such a crippled theme implementation, it will save time, energy, electricity, nerves, etc... Everyone will be happy. Job done in half a day.
Nightmare no.1 - multiple libraries
Lets start in .info file. I am not really against using multiple scripting libraries at once, eg. prototype, mootools, jQuery, etc... And never had to do it. I would consider it ok to use multiple libs when you have a functionality that you had previously before migrating to Drupal and it is a matter of time to convert it to jQuery (if possible). But using several libraries to achieve an effect that you can quite easily do with jQuery or there might be a plugin already? Look at this (the text after dashes are my notes):
located in: elmstreet.info
scripts = jquery.pngFix.js - jQuery plugin
scripts = mootools.r598.js - mootools library
scripts = yamgo-menu.js - mootools file
scripts = jmenuopen.js - jQuery file
scripts = jsearchbox.js - normal js file
scripts = jquery.js - jQuery library!?
So, why are we using mootools here? Because we need a drop down menu, kind of suckerfish style. I must say, suckerfish would work very well. Maybe better. Can we just remove the mootools library and replace yamgo-menu with suckerfish? Of course. And if there is another themer coming to this project, it is more likely that they will know how to work with jQuery. No need to learn another js library.
In the jsearchbox.js file, there are a lot of lines written in normal js. We surely can make this simplified and more friendlier to others using jQuery. And yes, in this case it seems that someone just copied a piece of code to achieve something. Does it make sense to have a js function getElementsByClassName?
Ouch, they are even including jQuery at the end! Believe it or not it is the same version as Drupal 6 ships with!
Nightmare no.2 - hardcoded "link" & "script" elements
Hello there, not a first time I have seen this in a theme (page.tpl.php). So, $scripts variable in page.tpl.php prints out all scripts that were added by drupal throughout the process of rendering a page. Same for $styles variable to print out all stylesheets.
How about this:
<?php print $head ?>
<title><?php print $head_title ?></title>
<link type="text/css" rel="stylesheet" media="all" href="<?php echo base_path() . path_to_theme();?>/common.css?G" />
<link type="text/css" rel="stylesheet" media="all" href="<?php echo base_path() . path_to_theme();?>/style.css?G" />
<link type="text/css" rel="stylesheet" media="all" href="<?php echo base_path() . path_to_theme();?>/pc-menu.css?G" />
<?php print $scripts ?>
No $styles variable? Nothing in .info file where this theme css files should be specified for inclusion? How about modules css files? We don't want them to give us some basic styling?
Ok, this is not good and simply by not printing the $styles variable you will print only yours styles hardcoded in head and you won't be able to apply Drupal's css optimization.
In case you want to remove some of the styles that are put into the $styles by Drupal and its modules, you can simply say that in the .info file. eg.
stylesheets[all] = system-menus.css
This will remove the system-menus.css from the styles. Or you can use another approach in preprocess function.
Must admit, we do sometimes add custom link element for conditional stylesheets, but we never remove the $styles or $scripts vars.
Nightmare no.3 - $closure replaced with require_once
$closure variable is important. It is usually being printed right before the body closing tag. Modules can send scripts into this variable via drupal_add_js. Most of the wysiwygs won't work if you forget to print out the $closure var.
Now what striked me, that there already was the google analytics module, all set and ready. But apparently it wasn't doing anything for them. It would work well if $closure was there. Instead, they made another file called google_analytics.php and placed the html/js (generated by google analytics) there. How neat is that?
Found in: page.tpl.php
<?php require_once($_SERVER['DOCUMENT_ROOT']."/themes/elmstreet/google_analytics.php"); ?>
Why I call them nightmares? I can't sleep when I know there is a code like this lurking somewhere on the production sites. And yet suprised/sad that I am still seeing people not using it properly. How about you?
Some evil stuff coming next (5 nightmares like sql in tpl.php, fapi and more...). See you soon in part 2.
1 2 Freddy's coming for you...