Implement theme templates [#271287]

Following on from http://drupal.org/node/221608, I'm looking at getting templates into these modules.

First impressions of getting the theme layer into image.module:
There's too many nested theming calls at the moment.
We have:
- theme_image_teaser/body
-- calls image_display
--- calls theme_image_display

Nesting theming calls seems weird to me. Especially if we switch to using a tpl.php template.

I can change it, but it involves taking a hatchet to a lot of code -- probably replacing all those different theming calls with a single theme_image_display and a preprocess function that sorts out all the data.
What do you think, drewish?

Comment	File	Size	Author
#3	image theming layer.patch	6.18 KB	joachim
#3

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

drewish CreditAttribution: drewish commented 16 June 2008 at 20:32

i'd say take a hatchet to it. a lot of that code is still in the whole 4.6 frame of mind. lets pretend we're doing it right the first time... i would really like to end up with .tpl files to make it easier for themers.

Comment #2

joachim CreditAttribution: joachim commented 17 June 2008 at 22:16

Hatchet it is then!

I'm thinking there should be one template file for images. That way all images can easily be themed in one place.
At the same time, this should allow to distinguish between images in full nodes / teasers / block / view / gallery / attached to other nodes as full node or teaser, etc.

I'm going to have a stab at doing it the way I've described here: http://groups.drupal.org/node/11780

Comment #3

joachim CreditAttribution: joachim commented 5 July 2008 at 13:32

File	Size
image theming layer.patch	6.18 KB

Here is a patch for image.module.
It adds a template file for displaying both teasers and body images.

Here's an explanation of the changes to theming functions:

The existing flow is like this:
- theme_image_{teaser/body} -- optionally wraps a link around the call to...
-- image_display -- gets image info, build CSS classes, gets sizes
--- theme_image_display -- bare wrapper
---- theme_image -- core function -- returns an tag

New flow:
- theme_image_body -- calls a preprocessor function...
-- template_preprocess_image_body -- build up data in preprocessor:
* get image info
* build CSS classes
* get sizes
* create IMG tag from core theme_image
--- hand all this to the template file image.tpl.php

theme_image_teaser works the same way:
- theme_image_teaser -- calls a preprocessor function...
-- template_preprocess_image_teaser -- build up data in preprocessor
* call the main preprocessor, template_preprocess_image_body
* wrap the IMG in a link element, while storing the original IMG snippet
in case a themer wants to have access to it.

image_display() remains, even though it's not used by this module's theming code any more -- some submodules call it and so does views support in this module.
The next job will be to clean that up.

I've also moved a couple of functions in the code so all the theming-type stuff is together, and in the order of flow.

Comment #4

drewish CreditAttribution: drewish commented 6 July 2008 at 05:10

Status:

Active

» Needs work

for template_preprocess_image_body() i'd check isset($variables['size']) before calling empty($variables['node']->images[$variables['size']])

the rest looks pretty reasonable. seem like it's still a work in progress.

Comment #5

joachim CreditAttribution: joachim commented 6 July 2008 at 08:11

Right. That check I copied from image_display(). Consider it Frankencode, as TBH I'm not entirely sure what case it's checking for.

I'm starting a new job tomorrow and I really don't know how much time I'll have for working on this, if any.
Can I hand this on to you and the coding sprint you have planned (unless that's already happened... in which case, image code spring in Szeged? I'd really like to revive that plan of walkah's that's on there somewhere.)