Only show the term heading if the term has a description

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.

+  ¶

+  $build = array();

+  ¶

what is it for?

Add fixed version

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.

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.

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

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..

Whoops, here's the roll.

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

Committed to CVS HEAD. Thanks.

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 ?

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';
  }

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).

subscribe

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

Patch reverting #9

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.

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

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.

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.

Subscribe.

Compile and hide description for show image is insane :P

ops.. sorry

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.

+1

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

Subscribe.

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)

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.

Suppose we just need a proper fix for that

Seems 'content' should be initialized to fix notices

Looks good, thks @andypost.

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. :-(

I can confirm that it's working.

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