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:

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

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

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

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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jeff Burnz’s picture

To test use Bartik or Stark.

Relates to and causes the issue in #997108: Sticky not aligned with sidebars

erikwebb’s picture

FileSize
990 bytes

Because of the way tabs are aggregated, does it make more sense to -

  1. Check for an empty array in the menu system and unset the variable for the theme
  2. Add an extra check to the theme to check for an empty array

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.

erikwebb’s picture

Status: Active » Needs review

Also, option #2 doesn't cause an API change.

Jeff Burnz’s picture

FileSize
834 bytes

Can we just put a check inside menu_local_tabs?

erikwebb’s picture

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

bleen’s picture

I personally prefer #4's approach ... but if we are going to go with #2, can we do this instead:

if (!empty($tabs)) { ... }

Jeff Burnz’s picture

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

willmoy’s picture

I 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):

if (isset($tabs) && count($tabs) > 0): 

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.

willmoy’s picture

#4: menu_local_tabs.patch queued for re-testing.

Damien Tournoud’s picture

The correct way is:

<?php if ($tabs_rendered = render($tabs)): ?><div class="tabs"><?php print $tabs_rendered; ?></div><?php endif; ?>

That's how we do it elsewhere.

bleen’s picture

Jeff Burnz’s picture

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

willmoy’s picture

I agree with Jeff Burnz, and so apparently does webchick (on the thread bleen18 linked to):

Uh?!? No. Way. Those templates have had that markup in them since 2009. They're staying put.

Moving to D8, but this is probably even won't fix. That logic doesn't belong in tpl.php files.

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.

Jacine’s picture

Ugh, this is an annoying problem.

I don't like #10 at all. I ran into this myself, and decided to do this:

<?php if (!empty($tabs['#primary'])): ?>
  <div class="tabs"><?php print render($tabs); ?></div>
<?php endif; ?>
swentel’s picture

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

Jeff Burnz’s picture

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

Jacine’s picture

Ah. Well, I guess that's the way it has to be then. Sigh.

JohnAlbin’s picture

There 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 "

into the theme_menu_local_tasks() theme function. It would be trivial to check if there is output before adding the wrapper. This method would cause issues with Garland though. :-p

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.

<?php if ($tabs = render($tabs)): ?>
  <div class="tabs"><?php print $tabs; ?></div>
<?php endif; ?>
JohnAlbin’s picture

FileSize
3.27 KB

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

Status: Needs review » Needs work

The last submitted patch, 997408-19-tabs-div.patch, failed testing.

tstoeckler’s picture

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

barraponto’s picture

Status: Needs work » Needs review

#19: 997408-19-tabs-div.patch queued for re-testing.

bleen’s picture

An interesting read that is related to this issue at a very high level: http://drupal4hu.com/node/290

tim.plunkett’s picture

Version: 7.x-dev » 8.x-dev
FileSize
3.3 KB

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

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Agreed that this is a targetted, harmless fix.

Dries’s picture

Version: 8.x-dev » 7.x-dev

Committed to 8.x. Thanks. Moving to 7.x for @webchick to evaluate.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

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

willmoy’s picture

Agree with webchick and using #4 happily since January, so if it's the right approach IMO it's good to go.

sun’s picture

+++ b/includes/menu.inc
@@ -2188,17 +2188,13 @@ function menu_local_tabs() {
 function theme_menu_local_tasks(&$variables) {
   $output = '';
...
   return $output;

+++ b/modules/system/page.tpl.php
@@ -122,7 +122,7 @@
-        <?php if ($tabs): ?><div class="tabs"><?php print render($tabs); ?></div><?php endif; ?>
+        <?php if ($tabs = render($tabs)): ?><div class="tabs"><?php print $tabs; ?></div><?php endif; ?>

+++ b/themes/bartik/templates/page.tpl.php
@@ -192,9 +192,9 @@
-      <?php if ($tabs): ?>
+      <?php if ($tabs = render($tabs)): ?>
...
-          <?php print render($tabs); ?>
+          <?php print $tabs; ?>

+++ b/themes/garland/page.tpl.php
@@ -46,7 +46,7 @@
-          <?php if ($tabs): ?><?php print render($tabs); ?></div><?php endif; ?>
+          <?php if ($tabs = render($tabs)): ?><?php print $tabs; ?></div><?php endif; ?>

Let's move the fix into http://api.drupal.org/api/drupal/includes--menu.inc/function/menu_local_... instead.

Powered by Dreditor.

sun’s picture

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

sorry for the noise.

c4rl’s picture

Subscribe

sun’s picture

oh, 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 ;)

Devin Carlson’s picture

subscribing

catch’s picture

So do we need to roll back the 8.x patch and start again then?

klonos’s picture

...subscribing.

catch’s picture

Status: Needs work » Needs review
FileSize
3.3 KB

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

sun’s picture

Status: Needs review » Reviewed & tested by the community

Agreed.

catch’s picture

Title: $tabs is always set » (rollback) $tabs is always set
xjm’s picture

Tagging.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Fixing markup.

xjm’s picture

Summary 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() and page.tpl.php, when we set up the variables in template_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.

xjm’s picture

Issue summary: View changes

Missed a parenthesis.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

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

klonos’s picture

Title: (rollback) $tabs is always set » $tabs is always set

...since the rollback is done.

klonos’s picture

Issue summary: View changes

Better word choice.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Updated the summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

tim.plunkett’s picture

Status: Needs work » Closed (duplicate)

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

sachbearbeiter’s picture

as a workaround in D7:

<?php if ($tabs = render($tabs)): ?>
    <div class="tabs">
    <?php print render($tabs); ?>
    </div>
<?php endif; ?>
barraponto’s picture

Are you sure rendering tabs twice is the way to do it? I didn't test it, but I think it should be:

<?php if ($tabs = render($tabs)): ?>
    <div class="tabs">
    <?php print $tabs; ?>
    </div>
<?php endif; ?>
barraponto’s picture

Issue summary: View changes

Updated issue summary.