Problem/Motivation
Core's page.tpl.php
uses the following to print tabs:
<?php if ($tabs): ?><div class="tabs"><?php print render($tabs); ?></div><?php endif; ?>
However, the $tabs
variable is never empty and the div is always printed. $tabs
is set in template_preprocess_page() to the return value of menu_local_tabs():
function menu_local_tabs() {
return array(
'#theme' => 'menu_local_tasks',
'#primary' => menu_primary_local_tasks(),
'#secondary' => menu_secondary_local_tasks(),
);
}
This is never empty; it always includes all three keys and '#theme'
is always a non-empty string. (This issue is a specific, targeted case of #953034: [meta] Themes improperly check renderable arrays when determining visibility.)
theme_menu_local_tasks() has a related issue.
Proposed resolution
There are a few possible approaches:
-
Add logic to the theme template to render the tabs first and then check the rendered value.
(A patch using this pattern is in #24).
This is not an ideal solution for several reasons:
- It muddies the separation between logic and presentation.
- It is harder for non-programmers to understand and not good for themer experience (see http://drupal4hu.com/node/290 and #1158090: Separate usage of hide(), render(), and other template features from print statements to enable non-developers to create themes .
- It is not backportable to Drupal 7 because it alters a core template, and existing themes are already built on them.
-
Check the values of
$tabs['#primary']
and$tabs['#secondary']
rather than just$tabs
.This has the same issues for TX as #1. Additionally, the same pattern will not work for rendering content and so is not a good choice.
-
Change menu_local_tabs() to return empty if no primary or secondary tabs are set.
This is cleaner than #1 or #2, but is an API change. Existing code may try to use the expected keys of
menu_local_tabs()
directly. -
Check for non-empty primary or secondary tabs in template_preprocess_page().
As in #3, existing code may try to check the keys of
$tabs
directly.
Remaining tasks
A patch was applied to D8 using solution #1 above and then later rolled back since it was not backportable. (See #24, #26, and #41).
A new patch is needed using solutions #3 or #4 in the list above. Patch in comment #4 is the last working patch using this type of solution. At a minimum, that patch needs to be changed so menu_local_tabs()
always returns at least an empty array.
Any solution should be consistent with #953034: [meta] Themes improperly check renderable arrays when determining visibility. Fixing that issue would include resolving this one as well.
User interface changes
Potential template or markup changes, depending on the solution chosen.
API changes
Potential API change, depending on the solution chosen.
Original report by @Jeff Burnz
In core page.tpl.php we use this:
<?php if ($tabs): ?><div class="tabs"><?php print render($tabs); ?></div><?php endif; ?>
However the DIV's are always printed (i.e. in teaser mode). I'm not sure where to fix this.
Setting to major to get some fast attention, we should really fix this.
Comment | File | Size | Author |
---|---|---|---|
#36 | 997808-rollback.patch | 3.3 KB | catch |
#24 | drupal-997408-24.patch | 3.3 KB | tim.plunkett |
#19 | 997408-19-tabs-div.patch | 3.27 KB | JohnAlbin |
#4 | menu_local_tabs.patch | 834 bytes | Jeff Burnz |
#2 | tabs-always-set-997408-2.patch | 990 bytes | erikwebb |
Comments
Comment #1
Jeff Burnz CreditAttribution: Jeff Burnz commentedTo test use Bartik or Stark.
Relates to and causes the issue in #997108: Sticky not aligned with sidebars
Comment #2
erikwebb CreditAttribution: erikwebb commentedBecause of the way tabs are aggregated, does it make more sense to -
I've attached a patch for option #2, which I think is the better choice. An empty array means no options, a NULL/unset variable could mean many things.
Comment #3
erikwebb CreditAttribution: erikwebb commentedAlso, option #2 doesn't cause an API change.
Comment #4
Jeff Burnz CreditAttribution: Jeff Burnz commentedCan we just put a check inside menu_local_tabs?
Comment #5
erikwebb CreditAttribution: erikwebb commentedI think, logically, that an empty array means there are zero tabs, while a NULL value could mean different things. I think we need some other opinions, because both make sense really.
Not sure this is really 'Major', but it would constitute a practical change for themers, so it's major in that way.
Comment #6
bleen CreditAttribution: bleen commentedI personally prefer #4's approach ... but if we are going to go with #2, can we do this instead:
if (!empty($tabs)) { ... }
Comment #7
Jeff Burnz CreditAttribution: Jeff Burnz commentedI have tested with both count() and empty() and neither appear to work, because afaict there is always an item in the array - even if nothing is actually being returned form menu_primary_local_tasks() or menu_secondary_local_tasks, menu_local_tabs() still returns '#theme' => 'menu_local_tasks'
I still think this is rather major, although it can be worked around (see Seven or Garland) this is actually how we are doing this in core page.tpl.php and it does not work - this what we are presenting to the world as the example that everyone should follow. I agree its not "that" major but still a bad ass bug to have in core.
Comment #8
willmoy CreditAttribution: willmoy commentedI agree that this is major, because it leaves a pointless and usually pretty conspicuous page element on a lot of pages for any logged in user. A small but significant WTF (which was enough to put me off doing a migration to D7 tonight).
I have an omega subtheme with this (the 2.2 test):
endif;
?>
It was rendering the tabs div even on the home page where there are none. Jeff Burnz's patch in #4 fixed it. Applies with a few lines offset.
Can also confirm that patch works when using if ($tabs).
I am not a theming expert, not even a novice, but if someone else agrees, I think this is RTBC.
Comment #9
willmoy CreditAttribution: willmoy commented#4: menu_local_tabs.patch queued for re-testing.
Comment #10
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe correct way is:
That's how we do it elsewhere.
Comment #11
bleen CreditAttribution: bleen commentedrelated to #10 ... #953034: [meta] Themes improperly check renderable arrays when determining visibility
Comment #12
Jeff Burnz CreditAttribution: Jeff Burnz commentedShould we really be doing this sort of logic in the templates? I don't recall ever seeing this sort of thing in any Drupal template, I could be wrong but I am really racking my brain to think of an example.
Comment #13
willmoy CreditAttribution: willmoy commentedI agree with Jeff Burnz, and so apparently does webchick (on the thread bleen18 linked to):
I did have a look at core themes. Bartik does seem to use if ($tabs) in D7, so I'm not sure where the 'elsewhere is', but as I say this isn't my area so forgive me if I'm missing something.
Comment #14
JacineUgh, this is an annoying problem.
I don't like #10 at all. I ran into this myself, and decided to do this:
Comment #15
swentel CreditAttribution: swentel commented10 is actually good, because there might be an access check which still returns an empty string - ok most likely this will be fine for tabs, but for content this is really a different case, so we need to make sure we ducument this right.
Comment #16
Jeff Burnz CreditAttribution: Jeff Burnz commentedCorrect, for content it does not work, I have used if (render($page['region'])) with regions (for menu blocks with access checks) which seems to work.
Comment #17
JacineAh. Well, I guess that's the way it has to be then. Sigh.
Comment #18
JohnAlbinThere are two ways to fix this that I see, but both would required changes to the default page.tpl.php and would require all d7 themes (core and contrib) to be updated.
1.) Move the "
2.) Do what was suggested in #10 above. Though I would prefer to re-use the $tabs variable (instead of creating a $tabs_rendered variable) since we are rendering it and won't need it again.
Comment #19
JohnAlbinIf we can't use
!empty($tabs['#primary'])
in page.tpl, then our theme_menu_local_tasks() is wrong too, for the same reason.http://api.drupal.org/api/drupal/includes--menu.inc/function/theme_menu_...
Comment #21
tstoecklerPlease see #953034: [meta] Themes improperly check renderable arrays when determining visibility for reasoning why this is not on the table for D7. Tempted to mark this won't fix, but leaving at needs work for now.
Comment #22
barraponto CreditAttribution: barraponto commented#19: 997408-19-tabs-div.patch queued for re-testing.
Comment #23
bleen CreditAttribution: bleen commentedAn interesting read that is related to this issue at a very high level: http://drupal4hu.com/node/290
Comment #24
tim.plunkettThis was won't fixed for D7 by webchick here #953034-13: [meta] Themes improperly check renderable arrays when determining visibility.
Here's a git reroll since the testbots are changing at the end of the month.
This should stay as its own issue since its a targeted fix, and doesn't include all of the complexities of the other issue.
Comment #25
moshe weitzman CreditAttribution: moshe weitzman commentedAgreed that this is a targetted, harmless fix.
Comment #26
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks. Moving to 7.x for @webchick to evaluate.
Comment #27
webchickSorry, we can't make such changes to core templates in 7.x, so the proposed 8.x patch is a no-go for D7.
A reasonable approach for D7 would be a change further up the chain, like #4. I don't really understand why that fix isn't appropriate here. The template logic is correct, it's the theme function generating output incorrectly that's the issue.
(It's worth also noting the reason I won't fixed that, aside from the markup freeze aspect, is because it makes template files even more scary to non-PHP programmers/designers and even more logic-filled, when template.php is the place we've tried to tell people such logic should exist. But that's really for the other issue.)
Comment #28
willmoy CreditAttribution: willmoy commentedAgree with webchick and using #4 happily since January, so if it's the right approach IMO it's good to go.
Comment #29
sunLet's move the fix into http://api.drupal.org/api/drupal/includes--menu.inc/function/menu_local_... instead.
Powered by Dreditor.
Comment #30
sunsorry for the noise.
Comment #31
c4rl CreditAttribution: c4rl commentedSubscribe
Comment #32
sunoh, this went into D8 already? So that's probably the reason for why Garland always outputs
<h1 class="with-tabs">
in D8 but not in D7 -- as you can probably imagine, that leads to some funky page output ;)Comment #33
Devin Carlson CreditAttribution: Devin Carlson commentedsubscribing
Comment #34
catchSo do we need to roll back the 8.x patch and start again then?
Comment #35
klonos...subscribing.
Comment #36
catchThis is a rollback of #24 for D8, there's no point having a stop-gap fix in D8 that will never be applied to D7.
Comment #37
sunAgreed.
Comment #38
catchComment #39
xjmTagging.
Comment #39.0
xjmUpdated issue summary.
Comment #39.1
xjmFixing markup.
Comment #40
xjmSummary added.
The patch in #4 or similar is probably the cleanest solution. However, doesn't it still constitute an API change? We should at least return an empty array rather than
NULL
.What about doing the check midway between
menu_local_tasks()
andpage.tpl.php
, when we set up the variables intemplate_preprocess_page()
? I don't think that has been discussed yet.Also, we should make sure to be consistent with #953034: [meta] Themes improperly check renderable arrays when determining visibility. This issue kinda feels like a duplicate. However, if we come up with a backportable fix here, maybe the same pattern would be useful there.
Hopefully we can get this rolled back soon so we can look at the other possibilities.
Comment #40.0
xjmMissed a parenthesis.
Comment #41
Dries CreditAttribution: Dries commentedI rolled back this patch in Drupal 8, using the patch in #36 provided by catch. Moving this to 'needs work' so we can work on the proper solution.
Comment #42
klonos...since the rollback is done.
Comment #42.0
klonosBetter word choice.
Comment #42.1
xjmUpdated issue summary.
Comment #43
xjmUpdated the summary.
Comment #43.0
xjmUpdated issue summary.
Comment #44
xjmSimilar issue: #1390576: Remove empty HTML tags.
Comment #45
tim.plunkettThis was originally a separate issue from #953034: [meta] Themes improperly check renderable arrays when determining visibility, but since this approach was rolled back and abandoned, and there has been no new ideas here, I think this would be best closed to come up with a real solution over there.
Comment #46
sachbearbeiter CreditAttribution: sachbearbeiter commentedas a workaround in D7:
Comment #47
barraponto CreditAttribution: barraponto commentedAre you sure rendering tabs twice is the way to do it? I didn't test it, but I think it should be:
Comment #47.0
barraponto CreditAttribution: barraponto commentedUpdated issue summary.