Hi Zen,

we solved one shortcoming of the current quote.module and changed the quote rendering code so a theme-function is used for the *rendering* of quotes (if available). This means, with the attached patch the quote.module's output of one quote is now themed via the call

... theme('quote', $quote_content, $quote_author) ...

of a theme-function like:

function phptemplate_quote($quote_content, $quote_author) { ... }

If no such theme-function is defined, the module's default rendering will be used.

The patch was created against: quote.module,v 1.29.2.8 2007/08/11 19:51:20 karthik
It should be ready to be commited against the current dev-version, too.

Please try and commit the patch :)

CommentFileSizeAuthor
#5 quote.module.patch2.37 KBdanielnolde
quote.module.patch1.48 KBdanielnolde

Comments

Zen’s picture

Zen commented
Status: Reviewed & tested by the community » Needs work

Why not just make _quote_filter_process a themeable function?

Please don't set patches to "ready to be committed" directly - chalk it up for review first. Please also follow Drupal's coding standards.

Thanks :)

danielnolde’s picture

danielnolde commented

> Why not just make _quote_filter_process a themeable function?

_quote_filter_process includes some of the modules *logic*, that has nothing to do with theming but with handling the [quote]-parsing - that's not theming. Logic and Theming should be seperated as good as possible and/or suitable, and in this case the way the patch achieves it is suitable and get's the module to the goal of logic/theming seperation.

> Please don't set patches to "ready to be committed" directly - chalk it up for review first.

Okay.

> Please also follow Drupal's coding standards.

I can modify the code to adhere to the standards if i get a little hint where it lacks those... (would be the fastest way).

junyor’s picture

junyor commented

A couple suggestions for the coding style:

* Use two spaces for indenting (don't use tabs)
* Use meaningful variable names ($s isn't meaningful)
* Use "{" and "}" in if statements, even if they're only one line

The full coding standards are available at http://drupal.org/node/318.

junyor’s picture

junyor commented

Also, it would say that the logic and themeing aren't suitably separated. I'd rather see theme_quote() and theme_quote_author() added and used within _quote_filter_process(). I think that would mean not using regular expression replacement, though.

danielnolde’s picture

danielnolde commented
Version: 5.x-1.3 » 6.x-1.x-dev
Status: Needs work » Needs review
StatusFileSize
new quote.module.patch2.37 KB

Here's a reworked patch for current 6.x-1.x-dev release of quote.module (2008-Oct-07), including:

- proper adherence to code drupal standards
- D6 specific adustments (theme function is declared)
- the abstraction of now themable quote rendering form the quote parsing
- a reworked and simplyfied quote parsing method with a different, simplyfied and solid regexp (source given in code)
- nested quotes are handled correctly via recursive parsing

One thing: Rendering filter output through a theming function enables to smoothly seperate the filter's function from the rendering of its output - on a single theme site. On a multi-theme-site, this will be working correctly only, if the theme('quote', ... ) theming-function would be synchronized/identical throughout the different themes used.
However, since many many drupal projects involve heavy customizing via a single custom theme, the introduction of themabe output for the quote.module is a great help for themers/developer and at the moment the only way to avoid hacking this module for achieving custom quote rendering. So, anyway, the patch should be adding significant value.

Hope this patch gets tested by others, too, and will be finalley committed.

thanks,

daniel

NorthPort’s picture

NorthPort commented

Sorry, noob question, but how to import the patch or replace the existing file?

cheers,
David.

danielnolde’s picture

danielnolde commented

http://en.wikipedia.org/wiki/Patch_(Unix)

http://drupal.org/node/60108

Kripsy’s picture

Kripsy commented

This tests well for me.

Zen’s picture

Zen commented
Status: Needs review » Needs work

COmmitted slightly modified patch to 6-1.

Would it be possible to improve the regex to validate the quote tags more rigorously? Currently, [quotemenot] and [quote[quote=zen] both pass validation. Checking for the = and loose whitespace around it would also be good.

Setting to patch needs work. I'm also looking to backport this to D5.

Many thanks for your work on this, Daniel :)

Cheers,
-K

Zen’s picture

Zen commented

This has also been backported to D5.

Zen’s picture

Zen commented
Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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