Currently the term page ALWAYS outputs a header, even if the term doesn't have a description. This is a problem when the description needs styling. I mentioned this on my blog. See the attachments to understand the problem.

This is the code outputted:

<div class="term-listing-heading">
  <div class="taxonomy-term vocabulary-tags clearfix" id="taxonomy-term-6">
    <div class="content">
      <div class="taxonomy-term-description"></div>
    </div>
  </div>
</div>

My suggestion would be to only show this piece of HTML if the term has a description. See the patch!

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

BarisW’s picture

A month without review. Can anyone have a look at this?

cross’s picture

I searched you blog but didn't find the description of problem. Can you provide direct link?
What you mean "term page"? page with path like taxonomy/term/1 ? If so i can't notice the bug.

BarisW’s picture

Hi cross,

I applied the patch on my blog so the error is fixed. See my code example: empty tags are generated if a term doesn't have a description. This is unnecessary - and ugly when you apply styling to .term-listing-heading or .taxonomy-term-description. See the screenshots.

And indeed; I mean urls like taxonomy/term/1

cross’s picture

+  ¶

+  $build = array();

+  ¶

what is it for?

Add fixed version

furamag’s picture

I have reviewed this bug and tested patch http://drupal.org/files/issues/taxonomy_description_4.patch. I think this patch is a good fix for it.

cross, when I tried to use your version of patch I had error:
Fatal error: Unsupported operand types in C:\sites\drupal7\modules\taxonomy\taxonomy.pages.inc on line 45
I don't know why we need
$build = array();
but without this piece of code I can see only php error message.

AaronBauman’s picture

You absolutely need $build = array();, otherwise, when there is no term description (a prerequisite for this issue) you're trying to do shorthand array addition with an uninstantiated variable.

The patch in #4 results in a fatal error when the term does not have a description -- try it on a clean d7 install:

Fatal error: Unsupported operand types in modules/taxonomy/taxonomy.pages.inc on line 41 
Call Stack:
index.php: menu_execute_active_handler() 
index.php: call_user_func_array() 
includes/menu.inc: taxonomy_term_page() 
includes/menu.inc:0

Re-roll of OP @ HEAD attached.

cross’s picture

#5 true. Dunno how it pass tests.
Re roll patch with only whitespace fixed.

dman’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
0 bytes

Patch is fine as far as it goes, but different themers make different assumptions about whether blank placeholders are appropriate in the page or not. Sometimes that space is desired.
I generally think the themer should take steps to deal with that, and that the code should not output blank elements, so I'm happy with this 'fix'.

Anyway. This a pretty good fix.
HERE is a re-roll for HEAD
and a test that proves the problem and then proves that the fix works
So I'll flag this as tested (the patch from cross is good)

And I'm adding the simpletest, to make the fix eligible for core..

dman’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.37 KB

Whoops, here's the roll.

BarisW’s picture

Working great! :)
Thanks!

dman’s picture

Status: Needs review » Reviewed & tested by the community

Bump it up a little.
Ready to go (I think)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

yched’s picture

Status: Fixed » Active

Er - that logic might have been valid in D6, but not anymore in D7.
Terms can have fields. A term can have non-empty content while its description is empty.

Can we revert this patch ?

dman’s picture

Category: bug » feature
FileSize
2.67 KB

Dang. :(
Looking at taxonomy_term_view() I don't see any meaningful way we can run an empty() on the $build return value there.

Does this mean that the renderer naturally creates a lot of placeholder divs for null content in many other cases too?
Hm. Would be a lot of work to deal with that well. And potentially unpredictable. We may have to live with horrible layers of Drupal div-itis for a few more years if there is no clean way to say "don't print wrappers around empty content".

Or maybe all $build functions just have to be a lot more careful about what they build...
Sorta like this: Only fill in the $build array with rendering detail if there is genuinely content to render.
This means empty($build) can still be used further up the chain.

  $build = array();
  
  if (!empty($term->description)) {
    $build['description'] = array(
      '#markup' => check_markup($term->description, $term->format, '', TRUE),
      '#weight' => 0,
      '#prefix' => '<div class="taxonomy-term-description">',
      '#suffix' => '</div>',
    );
  }
  $build += field_attach_view('taxonomy_term', $term, $view_mode);
  
  if (! empty($build)) {
    // Only return markup if there is content to be rendered.
    $build = array(
      '#theme' => 'taxonomy_term',
      '#term' => $term,
      '#view_mode' => $view_mode,
    );
    $build['#attached']['css'][] = drupal_get_path('module', 'taxonomy') . '/taxonomy.css';
  }
yched’s picture

Title: Only show the term heading if the term has a description » [please revert] Only show the term heading if the term has a description
Category: feature » bug
Priority: Normal » Major

As I stated in #13, the patch that got committed is absolutely wrong, this needs to be reverted.
Reported in #953928: taxonomy/term/X only displays the term's fields if term has a description (marked as dupe).

vito_swat’s picture

subscribe

sun.core’s picture

Status: Active » Reviewed & tested by the community

Let's start with a straight rollback of #9 first.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, taxonomy_description_796692_3.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
2.03 KB

Patch reverting #9

catch’s picture

Status: Needs review » Reviewed & tested by the community

Yes the original patch doesn't make any sense for D7 at all, This is a straight revert, let's get it in, then open a new issue to discuss empty divs, which is not an issue specific to taxonomy module at all.

Also term description would very likely have been a field in D7 if we'd had a bit more time, so adding even more special casing for it only makes that process harder in D8.

dman’s picture

Yeah
I'm ok with a revert. Although the issue should still be addressed, my patch had some d6 assumptions which don't hold up under new testing

webchick’s picture

Hm. I'm a bit torn here because this is a UI change well after UI freeze. Insanely, utterly past.

And yet, it can't be denied that the way this was implemented originally wasn't correct, and this point was first brought up < 24 hours after the patch went in. And a field API maintainer thinks it should be rolled back, as well as one of the people who originally worked on it... hmmm...

I'm going to let this sit overnight and look at it again in the morning.

codycraven’s picture

webchick, any further thought on this? It seems utterly broken to require data within the description field to display other fields which do have data in them.

robertom’s picture

Subscribe.

Compile and hide description for show image is insane :P

robertom’s picture

Priority: Major » Normal
Status: Reviewed & tested by the community » Needs work
robertom’s picture

Priority: Normal » Major
Status: Needs work » Reviewed & tested by the community

ops.. sorry

codycraven’s picture

Just as a note, I've created a module which addresses this problem since it looks like it will not be fixed in core for D7 taxonomy_display.

webankit’s picture

+1

renat’s picture

Subscribe.
It's really serious for sites actively using custom fields for taxonomy.

balintk’s picture

Subscribe.

catch’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
yched’s picture

For the record : this is indeed a no-brainer for D8, but webchick had some concerns about committing this change in D7 (although I stand by the fact that the current D7 behavior is plain broken)

Dave Reid’s picture

Subscribe...this affects contib modules like Metatags that want to use hook_entity_view_alter() with all entities since terms with no descriptions never have the hook invoked.

andypost’s picture

Title: [please revert] Only show the term heading if the term has a description » Only show the term heading if the term has a description
Status: Reviewed & tested by the community » Needs review
FileSize
3.17 KB

Suppose we just need a proper fix for that

Status: Needs review » Needs work

The last submitted patch, 796692-term-description.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
3.52 KB

Seems 'content' should be initialized to fix notices

yched’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, thks @andypost.

EclipseGc’s picture

Would love to see some movement on this, and hopefully have it present in 7.1 considering how broken term pages are at the moment without this. :-(

vito_swat’s picture

I can confirm that it's working.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x and 8.x. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -Needs backport to D7

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