Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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!
Comment | File | Size | Author |
---|---|---|---|
#36 | 796692-term-descr.patch | 3.52 KB | andypost |
#34 | 796692-term-description.patch | 3.17 KB | andypost |
#19 | taxo_header_revert-796692-19.patch | 2.03 KB | yched |
#14 | taxonomy_description_796692_3.patch | 2.67 KB | dman |
#9 | taxonomy_description_796692_2.patch | 2.37 KB | dman |
Comments
Comment #1
BarisW CreditAttribution: BarisW commentedA month without review. Can anyone have a look at this?
Comment #2
cross CreditAttribution: cross commentedI 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.
Comment #3
BarisW CreditAttribution: BarisW commentedHi 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
Comment #4
cross CreditAttribution: cross commentedwhat is it for?
Add fixed version
Comment #5
furamag CreditAttribution: furamag commentedI 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.
Comment #6
AaronBaumanYou 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:
Re-roll of OP @ HEAD attached.
Comment #7
cross CreditAttribution: cross commented#5 true. Dunno how it pass tests.
Re roll patch with only whitespace fixed.
Comment #8
dman CreditAttribution: dman commentedPatch 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..
Comment #9
dman CreditAttribution: dman commentedWhoops, here's the roll.
Comment #10
BarisW CreditAttribution: BarisW commentedWorking great! :)
Thanks!
Comment #11
dman CreditAttribution: dman commentedBump it up a little.
Ready to go (I think)
Comment #12
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #13
yched CreditAttribution: yched commentedEr - 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 ?
Comment #14
dman CreditAttribution: dman commentedDang. :(
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.
Comment #15
yched CreditAttribution: yched commentedAs 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).
Comment #16
vito_swat CreditAttribution: vito_swat commentedsubscribe
Comment #17
sun.core CreditAttribution: sun.core commentedLet's start with a straight rollback of #9 first.
Comment #19
yched CreditAttribution: yched commentedPatch reverting #9
Comment #20
catchYes 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.
Comment #21
dman CreditAttribution: dman commentedYeah
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
Comment #22
webchickHm. 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.
Comment #23
codycraven CreditAttribution: codycraven commentedwebchick, 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.
Comment #24
robertom CreditAttribution: robertom commentedSubscribe.
Compile and hide description for show image is insane :P
Comment #25
robertom CreditAttribution: robertom commentedComment #26
robertom CreditAttribution: robertom commentedops.. sorry
Comment #27
codycraven CreditAttribution: codycraven commentedJust 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.
Comment #28
webankit CreditAttribution: webankit commented+1
Comment #29
renat CreditAttribution: renat commentedSubscribe.
It's really serious for sites actively using custom fields for taxonomy.
Comment #30
balintk CreditAttribution: balintk commentedSubscribe.
Comment #31
catchComment #32
yched CreditAttribution: yched commentedFor 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)
Comment #33
Dave ReidSubscribe...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.
Comment #34
andypostSuppose we just need a proper fix for that
Comment #36
andypostSeems 'content' should be initialized to fix notices
Comment #37
yched CreditAttribution: yched commentedLooks good, thks @andypost.
Comment #38
EclipseGc CreditAttribution: EclipseGc commentedWould 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. :-(
Comment #39
vito_swat CreditAttribution: vito_swat commentedI can confirm that it's working.
Comment #40
Dries CreditAttribution: Dries commentedCommitted to 7.x and 8.x. Thanks!