Garland's code style has been a point of embarrassment every time I've opened it up to use as an example. Over time, the case has become even worse as Garland has started to include duplicate functionality now provided by the theme system in Drupal 6. It even has a function that has absolutely no purpose in it. Here's the set of changes that needs to happen for D7's Garland to redeem itself in the light of a well-styled theme:

- Remove the phptemplate_menu_local_tasks() function. It's not used :P
- Rename all phptemplate_ prefixes to garland_. Garland owns these functions, not phptemplate.
- Remove phptemplate_body_class(). theme.inc now provides classes that are perfectly acceptable for use (sidebar-left, sidebar-right, and two-sidebars).
- Move logic from page.tpl.php to garland_preprocess_page() in template.php. Let's keep the logic in template.php, .tpl.php files are for templating markup.
- Adding consistency to the opening and closing DIV comments.
- Converting all function calls in page.tpl.php to variables in garland_preprocess_page().
- Adding comments to template.php functions.

Garland still doesn't make the "perfect" starting theme because of wrapper DIVs used to add the rounded corners, but at least we're not ignoring our own best practices in the default Drupal theme. People will use Garland as a starter theme no matter what, so we should set a good example in the first code people are likely to encounter in Drupal.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Stefan Nagtegaal’s picture

I'm certainly agreeing with you on several points. Garland isn't the perfect place to start a new theme of...
I was wondering if it would be possible to split the default page.tpl.php file (whith the autodiscovering/margin/padding of the sidebars) into several PHP-template files.

Like:
- page-sidebar-content.tpl.php;
- page-content-sidebar.tpl.php;
- page-sidebar-content-sidebar.php;
- page-content.tpl.php;

this way it would be much easier for users to extend/change their default theme. Also the CSS files would be much easier to understand or to grok..
Though, i'm not sure it would be the most ideal way of doing things but I would love to discuss this some more in here as this seems to be the right issue (according your arguments)..

So, any idea's?

Stefan Nagtegaal’s picture

The amount of CSS and wrapper-divs would become less, and would be much easier to understand..
If coded well...

dvessel’s picture

I think the patch makes perfect sense but for whatever it's worth, I'm on the fence about having multiple page templates. Certainly, it'll be easier to understand each page separated by layout but it's more to juggle around and could bring out inconsistencies from having to modify four templates just to change the header or whatever.. but I guess that could be considered other peoples problems. I almost never use more than one page.tpl.php file.

Anyone with a decent understanding of CSS should be able to grok how the layout classes work.

quicksketch’s picture

With the convention of "body_classes" now being part of the core theming system (this patch makes Garland use the core $body_classes rather than making it's own), I think the single page.tpl.php file makes the most sense still. I like switching the CSS based on the body class, but maybe just because I've been using the Zen theme for a while, which also uses this approach.

quicksketch’s picture

FileSize
10.62 KB

I didn't realize phptemplate_menu_local_tasks() was a theme over-ride, so that function is actually doing something. This version restores that function so that $tabs will again only contain the primary navigation and never the secondary nav (which is in $tabs2).

dvessel’s picture

Status: Needs review » Needs work

The maintenance-page.tpl.php has to be changed too. It gives a fatal about phptemplate_get_ie_styles not existing on install. Also, the patch isn't diff'd from root.

dvessel’s picture

The way Garland/Minnelli works with the install and upgrade code is very crappy. I'm responsible for that. What I think is needed is a dedicated theme for installs, updates and off-line modes. I think Gábor mentioned this before.. I'll try to work on that.

For now, perhaps the maintenance template can skip the IE styles and add in the body classes.

dvessel’s picture

Status: Needs work » Needs review
FileSize
13.31 KB

I wasn't paying attention, just needed to change the prefix for the ie style function. Changed that and added the body classes to page-maintenance-page.

All that messy logic is still in the maintenance template but I think it's fine for now. Hopefully we can separate the install & update pages from Minnelli to free it from the mess.

Fixed a few typos in your patch. You repeated "site_name" variable in the preprocessor. Also mirrored the preprocess function from template_preprocess_page with template_preprocess_maintenance. It was missing a $front_page var. They should always have the same vars generated.

Did a quick test from install and checking the layout. Seems fine.

[edit: It seems prefixing the preprocessor to garland is preventing them from running when minnelli is running. It shouldn't matter since the base theme's preprocessors should always run. It's giving variable undefined notices.]

Dries’s picture

I've committed what you guys have for now. It looked like a good first pass and it all seemed to work. Feel free to follow-up on this, or to mark the issue as fixed.

quicksketch’s picture

Status: Needs review » Fixed

Thanks Dries! I've got some more changes that might allow us to cleanup the maintenance page also. But it'll be a little weird because we need to fire off the preprocess hooks for theme_maintenance_page() manually. I'll post more about it in a followup issue. Thanks again!

dvessel’s picture

Status: Fixed » Active

I didn't state the problem of prefixing garland_ properly. It prevents minnelli from running garlands preprocessor. I always thought the base themes preprocessors was supposed to always run but in checking with 6, it does the same thing.

It needs to be changed back.

Stefan Nagtegaal’s picture

Yup, that was why we used phptemplate_ as a prefix...

The status of this patch was CNR, right?

dvessel’s picture

I'm not sure if the base theme's preprocessor not working is a bug itself. I believe it is. Normal theme functions can be prefixed to garland just fine.

A separate post to get to the bottom of this.. http://drupal.org/node/252430

quicksketch’s picture

Looks like a bug to me also. The code seems to indicate the base theme is supposed to be processed. I've posted a patch to correct the problem in the dvessel's new post: http://drupal.org/node/252430.

starbow’s picture

subscribing

Cristian.Palmas’s picture

Version: 7.x-dev » 6.2

Hi all,

I tried to apply the patch on Drupal 6.2 installed on my laptop with XAMPP/Win installation.
It failed to apply.

I copied the garland_page_cleanup_2.patch into the root directory and I used the UnxUtils to run the following:

patch -p0 < garland_page_cleanup_2.patch --verbose --binary

Is the problem about the fact that this patch doesn't apply to D6?
I read an article at http://www.lullabot.com/articles/theming-best-practices-garland-gets-a-c... and it seemed to me that the patch were suitable for my Drupal version...

Cristian Palmas

StevenPatz’s picture

Version: 6.2 » 7.x-dev

Please don't move version.

quicksketch’s picture

Status: Active » Fixed

Sorry the patch alone won't work in D6. The entire theme system was rewritten in D6, allowing such a change to take place. Apologies that the article may have indicated that this could be done in Drupal 6.

Anonymous’s picture

Status: Fixed » Closed (fixed)

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

JohnAlbin’s picture

Quicksketch, you missed a couple cleanup items. ;-) #295895: Garland overrides overrides template with theme function

But its much easier to spot errors like this now that Garland is so much cleaner! yayz!