Display term description on pages - CHANGELOG.txt

The patch applies fine (though the relative path is off, it has to be applied inside modules/taxonomy, but it should be applicable from the root folder), but I'm not seeing a difference.

Namely, I created a new term with a description and am seeing its page at taxonomy/term/1. But there is no description there besides "There are currently no posts in this category." Only the corresponding feed page has the description...

Thanks for that info. After clearing the cache table, the description shows up as intended. So far so good. :)

I'm afraid this still doesn't happen on my test site though:

Equally to the feed output, only the first term's description is output if more than one term is requested.

When displaying more than one term, as in taxonomy/term/1+2, I see no description at all, regardless of the cache.

Having just one description on /1,2 and 1+2 seems like a bug to me.

Also I'm wondering if this should be a theme setting similar to the "submitted" option for node types - I probably have about ten dodgy descriptions of taxonomy terms out of 2,500 terms, so it'd at least need to be documented in theme upgrade etc. to stop nasty surprises.

+1 to no description for more than one term, makes sense to me.

An extra setting (per vocabulary?) on having the description display I think would be usability for D6 personally - it's arguably less of a change than this bug fix will be in terms of user experience.

Nice work.

I have applied this patch, and after clearing the cache tables, it does show the description on the page.
However i noticed some weird behaviour.

Steps i took:
- created a vocabulary, assigned to Story nodes and with a description
- created 3 terms within the vocabulary (World, Business, Sports) with a description for each term
- created 3 Story nodes, and assigned to first two to 'World', and the last to 'Sports'.
- applied the patch
- checked to see the description on the pages taxonomy/term/1 (and 2 & 3 afterwards) -> is fine
- Next i added the feed for Sports (taxonomy/term/3/0/feed) to my newsreader. It showed the 1 item in that category.
- Next i added the feed for Business (taxonomy/term/2/0/feed) to my newsreader. This should be empty, but shows all 3 content items instead!
Also when refreshing the admin pages i get the following error:

notice: Undefined variable: items in C:\xampp\htdocs\drupal6cvs\modules\taxonomy\taxonomy.pages.inc on line 62.

I'm not 100% sure if this is related to the patch, but when i reversed the patch, it seemed to work.

Hmm i must still be half asleep...
In fact i get this behaviour without having applied the patch, so this seems unrelated. You can forget my comment :)
Now i should try to find what causes this error, and if it's already been reported...

I like what this patch is doing: I agree, this is a long-overdue feature in the taxonomy module!

However, -1 to hard-coding the default input format for outputting the description. Even though it's in a themable function, and it's only intended as a temporary solution, it's still very ugly. I'd prefer to let this patch wait until D7, when we can add a new 'format' field to the 'term_data' db table, and thus allow the input format to be configurable per-term by the user. Of course, it's too late in the D6 release cycle for a schema change such as that.

Otherwise — as an alternate D6-ready solution — I'd prefer to pass the description through filter_xss_admin(), instead of check_markup(), thus outputting it safely while avoiding the filter system. According to the page on how to handle text in a secure fashion, filter_xss_admin() would seem to be the best outputting mechanism to use here. Plus, themers can always override the function, if they do actually want to use check_markup() and to force the default input format.

Also, I think that the output of theme_taxonomy_term_page() should wrap the description in some kind of special container, e.g. a div with class 'term-description'.

+1
I really like this small addition to Taxonomy module. Patch applies cleanly, works well, and seems OK to me. So setting RTBC.

Indeed, if Taxonomy should be used for categorization in Drupal by everyone (as quite a few people said to me that Category module is bound to be dropped one day), then we need to improve Taxonomy's navigational/user-experience greatly. This patch is an important part of that, which I believe is non-intrusive enough for 6.x.

I'm starting a new issue on the next part I believe is needed: http://drupal.org/node/185146 Obviously, that's a D7 stuff...

Indeed, t is quite unfortunate that we have quite a few textareas in Drupal, which don't have an input format associated. Taxonomy descriptions are of those kind. So using filter xss seems like the best we can do for now.

What's to do with this patch still IMHO:

- doesn't taxonomy already has a themeable function group, where this function could go?
- a comment would be great at both places with something like: // Only display the description if we have a single term, to avoid clutter and confusion.

These are not major things though.

- Themable functions group: No, there isn't one. I checked - 'taxonomy.pages.inc' is a tiny file with only just 3 functions in it (including the one added by this patch), and there's no explicit functions grouping in the whole Taxonomy module. I also searched for other themable functions, and found only one (theme_taxonomy_term_select() in taxonomy.module - i.e. other file), which is no more than a wrapper around another theme() call. So our new function is the only one in the file, and only *real* one in the whole module, so it's only natural to have it near it's usage IMO.

- Along the way, I noticed that the file taxonomy.pages.inc had an incorrect comment at the top (someone probably pasted it accidentally from taxonomy.admin.inc while splitting the module) - these page callbacks are certainly *not* administrative. It's so tiny issue (one word in a comment), so I added it to this patch too.

- Suggested comments are now added. Really useful IMO.

New patch attached, with NO changes done to the code, only comments changed. Still, I tested it successfully.

Oh, I nearly committed this patch, but then decided to look up what happens with 'description' after all.

- node_feed is called with $channel (http://api.drupal.org/api/function/node_feed/6)
- node_feed passes on the description to format_rss_channel (http://api.drupal.org/api/function/format_rss_channel/6)
- which strips all HTML and escapes what's remaining

So after all it was not bad that the description was not filtered for XSS here when passing on for feed display. When passing on for page display, it needs to be filtered though. So seems like we are fine by removing the filtering and adding a comment there that HTML will be removed from the description later.

Marking as needs work for that comment.

I did the change, but as the attached file is just untested edit to the previous patch file, it needs a bit review again. (Also sorry about the extension - I'm currently sitting on my boss' outdated Windows, unable to change settings here.)

Third hunk doesn't apply, "malformed patch".

Hmm, perhaps it's not OK to add lines to a patch file by just editing it, or the Windows CR/LF environment is a problem... Sorry about that, but unfortunately I've not access to my patch-rolling setup now (until evening).

OK, I've just returned to my home machine, and re-created the change. Attaching the same patch again, but rolled properly now. I've also tested it (including the feed output), but another review would be nice, obviously.

Patch applies ok but I get a WSOD on all taxonomy/term pages with or without a description entered after applying.

- Re-rolling, applied with fuzz today.

- catch: Please explain a bit more, I have no clue what is "WSOD" supposed to mean (I'm rather new to these abbreviations, yes). I tested the patch again, and works fine for me. But all my testing is on fresh 6.x-dev installs with just few terms/nodes created (my production site doesn't run Taxonomy).
Thanks in advance for more info.

Oh, after a bit more testing I seem to get it at last: You mean 'White Screen Of Death', right? ;-)

I managed to reproduce that, but only by targetting it specifically:
- Did an unpatched 6.x-dev install, created a few terms and nodes. All OK.
- Applied the patch to this existing install, WITHOUT flushing cache.
- Now, indeed, all Term pages show just white screen. There's a new themable function in the patch, which needs to register in the new theme system properly.
- Flushed the 'cache' table.
- Now all fine, and patch works as expected.

So setting back to 'Needs Review', because the above is not a fault of the patch. If your problem was different, feel free to change status again.

Agreed.

Thanks, committed.

sun, in that case I'm setting back to active so this can be added to CHANGELOG.txt, where such announcements are usually drawn from. I agree that it'll make a big difference to new drupal users - we've used things like sticky nodes to have something up top (which means they always appear at the top of feeds as well, not great).

here's a patch.

Well next to it is:

- Added support for OpenID.
- Added support for triggering configurable actions.

These will also need translating into plain English, possibly with links to more information, when they get into the announcement itself. If we could keep it suitably short though I don't see why not.

While I don't think this warrants its own CHANGELOG item, I am not strongly against it either. Anyway, we already have a taxonomy versioning item, so we can fold it under this "heading" to make some use of this being a list.

Well here's a patch with that change anyway.

OK, modified rss to be RSS, fixed wrapping and wrapped first item as it was past 80 columns. Committed. Now let's move on to fixing bugs :)