This is a spin-off from #353208-36: Allow for runtime-conditional local tasks to make sure this patch gets in for D7. Please read over there why it's needed.

This is required to properly port CTools to D7.

Marking RTBC, because it was reviewed & approved already.

Files: 
CommentFileSizeAuthor
#46 drupal.menu-local-tabs.46.patch8.93 KBsun
PASSED: [[SimpleTest]]: [MySQL] 26,996 pass(es).
[ View ]
#34 menu-local-tabs-599706-34.patch7.35 KBJohnAlbin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch menu-local-tabs-599706-34.patch.
[ View ]
#30 menu-local-tabs-599706-30.patch5.86 KBJohnAlbin
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in template.php.
[ View ]
#29 drupal.menu-local-tabs.29.patch5.91 KBsun
PASSED: [[SimpleTest]]: [MySQL] 24,819 pass(es).
[ View ]
#27 menu-local-tabs-599706-27.patch4.47 KBJohnAlbin
PASSED: [[SimpleTest]]: [MySQL] 22,192 pass(es).
[ View ]
#24 menu-local-tabs-599706-24.patch4.46 KBJohnAlbin
PASSED: [[SimpleTest]]: [MySQL] 19,172 pass(es).
[ View ]
#17 drupal_599706.patch579 bytestobiasb
Passed: 13709 passes, 0 fails, 0 exceptions
[ View ]
#14 drupal.menu-local-tasks-alter.14.patch14.95 KBsun
Passed: 13707 passes, 0 fails, 0 exceptions
[ View ]
#9 drupal.menu-local-tasks-alter.9.patch13.97 KBsun
Failed: Failed to apply patch.
[ View ]
#5 drupal.menu-local-tasks-alter.5.patch13.98 KBsun
Passed: 13670 passes, 0 fails, 0 exceptions
[ View ]
#3 drupal.menu-local-tasks-alter.3.patch13.19 KBsun
Failed: 13478 passes, 186 fails, 688 exceptions
[ View ]
drupal.menu-local-tasks-alter.patch700 bytessun
Passed: 13683 passes, 0 fails, 0 exceptions
[ View ]

Comments

Status:Reviewed & tested by the community» Needs work

This is missing a system.api.php hunk. Is it worth adding a rudimentary test at the same time?

Issue tags:+D7 API clean-up

Tagging absolutely critical clean-ups for D7. Do not touch this tag. Please either help with this or one of the other patches having this tag.

Status:Needs work» Needs review
StatusFileSize
new13.19 KB
Failed: 13478 passes, 186 fails, 688 exceptions
[ View ]

You are right - that has been too easy ;)

So here's why this patch bumped from a few bytes to 14 KB:

1) menu_local_tasks() produced already rendered HTML string output, totally unsuitable for alteration. That is altered to use a renderable array structure, nicely leveraging the shiny new theme() awesomeness we have now. :)

2) theme_menu_local_tasks() is supposed to allow certain themes (like Garland) to override the rendered local tasks (Garland wants to have primary and secondary tabs separated instead of in one big blob). That (theming) feature is kept, but we now deal with renderable arrays instead of strings.

3) Due to 1) and 2), there is no location we could drupal_render() the local tasks, so they are passed along as renderable array (like many other variables in the meantime). The theme uses the new render() function to output them. This is especially required for 2), so themes are still able to output the different tab levels in different locations.

4) Pretty verbose documentation about hook_menu_local_tasks_alter().

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new13.98 KB
Passed: 13670 passes, 0 fails, 0 exceptions
[ View ]

ugh, not caused by this patch. The @todo I added is really really not nice. Extra points for the one who opens another highly critical API clean-up issue. :-/

Status:Needs review» Reviewed & tested by the community

Installed on my local Drupal install and sun's hook - working and looks great (I read the code too).

Should go in ASAP - ctools blocker

Status:Reviewed & tested by the community» Needs work

one little thing in a comment but sun's on it

Status:Needs work» Needs review
StatusFileSize
new13.97 KB
Failed: Failed to apply patch.
[ View ]

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

This seems reasonable, but:

1) Where's the test?

2) Let's get some benchmarks to ensure running this extra bit through the render system doesn't harm performance.

3) There is no way in hell I'm committing something with this hunk in it:

-      'arguments' => array('link' => NULL),
+      // @todo drupal_render() passes $elements instead of theme function
+      //   arguments in #properties when there is a single theme argument only.
+      'arguments' => array('link' => NULL, 'dummy' => NULL),

Status:Needs work» Reviewed & tested by the community

Not changing this to "needs work", but just offering up my opinion that we should change the signature to take a renderable array rather than an extra dummy argument: http://drupal.org/node/600974#comment-2136448. Feel free to take this opinion or leave it.

Status:Reviewed & tested by the community» Needs work

Sorry, cross-posted with webchick.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new14.95 KB
Passed: 13707 passes, 0 fails, 0 exceptions
[ View ]

Discussed lengthy in IRC and also explored other options (meh)...

1) This does not need a test, because we would be testing drupal_alter().

2) These are (most often) 3 links in a simple, two-dimensional renderable array, which are now rendered via drupal_render(). The entire page is rendered via drupal_render(), so there won't be any measurable benchmark result.

We also cannot use hook_page_alter() for this, because that would mean we have to move the template variable into the hook_page_build(). Our core themes, however, are directly invoking the builder functions for local tasks to be able to render them separately in different regions/variables. So they would not catch up any altered data from the page array, which remedies the entire approach.

3) That hunk is now removed, because I went ahead with effulgentsia's proposal (which chx also recommended earlier in IRC).

This patch is absolutely required to move forward with #473268: D7UX: Put edit links on everything, and I'm already a bit stressed, because I now spent hours on this no-brainer instead of aforementioned edit-anywhere patch. :-/

Status:Reviewed & tested by the community» Needs work
Issue tags:-D7 API clean-up+Needs Documentation

Ok, that's fair enough. Thanks very much for looking into this more. I'm much more satisfied with passing in $element instead of $dummy. :P

Committed to HEAD (with minor adjustment to PHPDoc) to keep things moving.

This needs documenting in the module upgrade guide.

tobiasb spotted a PHPDoc mismatch in the introduced API docs here... caused by the change into a theme function that takes a renderable array, it still says $element, but now takes $variables...

Status:Needs work» Needs review
StatusFileSize
new579 bytes
Passed: 13709 passes, 0 fails, 0 exceptions
[ View ]

small follow-up

Status:Needs review» Fixed

Committed to CVS HEAD. Thanks.

Status:Fixed» Needs work

I can't completely parse what we mean by "We also cannot use hook_page_alter() for this, because that would mean we have to move the template variable into the hook_page_build(). Our core themes, however, are directly invoking the builder functions for local tasks to be able to render them separately in different regions/variables. So they would not catch up any altered data from the page array, which remedies the entire approach."

Can't we use render() in those regions/variables?

For D8, I'd like to see this move to hook_page_build(). I think it is slightly inelegant to build renderable structures after hook_rendr_page() has been called.

Status:Needs work» Fixed

Scope-creep for this issue, anyway.

Status:Fixed» Needs work

Sorry Tha_sun :D

This needs documenting in the module upgrade guide.

Priority:Critical» Normal

Status:Needs work» Needs review
StatusFileSize
new4.46 KB
PASSED: [[SimpleTest]]: [MySQL] 19,172 pass(es).
[ View ]

Ok, so I was doing the docs for this and realized that theme_menu_local_tasks() is violating our theme function API. It no longer takes in variables (or render elements) and outputs a string.

Instead theme_menu_local_tasks() is now a content generation function which returns a render element. This is bad.

The simplest way to fix this it to create a function whose purpose is to return a render element and then have theme_menu_local_tasks() theme it like a normal theme function. So this patch adds a menu_local_tabs() function which is called by template_preprocess_page() instead of theme('menu_local_tasks').

Status:Needs review» Reviewed & tested by the community

Nice cleanup. If you wanted to do as garland does and always generate a separate tabs2 variable, i'd support that. but thats beyond the goal here so this one is rtbc.

Status:Reviewed & tested by the community» Needs work

+++ includes/menu.inc
@@ -1985,22 +1985,33 @@ function menu_tab_root_path() {
+  if (!empty($variables['primary'])) {
+    $output .= '<ul class="tabs primary">';
+    $output .= render($variables['primary']);
+    $output .= '</ul>';
   }

1) render() is only supposed to be used in templates.

2) Previously, the $tabs variable contained a renderable array. Can't we return a renderable array here, like before?

107 critical left. Go review some!

Status:Needs work» Needs review
StatusFileSize
new4.47 KB
PASSED: [[SimpleTest]]: [MySQL] 22,192 pass(es).
[ View ]

2) Previously, the $tabs variable contained a renderable array. Can't we return a renderable array here, like before?

If you look at the patch, you'll see that $tabs does contain a renderable array; its set by the return value of menu_local_tabs:

<?php
 
return array(
   
'#theme' => 'menu_local_tasks',
   
'#primary' => menu_primary_local_tasks(),
   
'#secondary' => menu_secondary_local_tasks(),
  );
?>

Its just that theme_menu_local_tasks should not be the function that returns the renderable array. theme_menu_local_tasks() should be returning a string. Period.

I put the #prefix and #suffix stuff as plain strings inside theme_menu_local_tasks() instead of inside the render element. I don't see any advantage in putting it in the render element. The classes for primary and secondary ULs are not easily modifiable by both modules and themes no matter where we put them. We'd have to create new $primary_classes/$secondary_classes variables (etc.) and add a template_preprocess_theme_menu_local_tasks() for that; which seems a bit beyond what we should be doing this far past code freeze.

Or did you want to make theme_menu_local_tasks's parameter be a render element instead of variables?

What are your thoughts?

1) render() is only supposed to be used in templates.

Oh! You mean I should use drupal_render(). Yep, you're correct! Re-rolled with this change, but I'll wait for your answer to my question above too.

#27: menu-local-tabs-599706-27.patch queued for re-testing.

StatusFileSize
new5.91 KB
PASSED: [[SimpleTest]]: [MySQL] 24,819 pass(es).
[ View ]

Re-rolled against HEAD.

AFAICS, there's no substantial API change here -- print render($tabs); still works the same.

StatusFileSize
new5.86 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in template.php.
[ View ]

@sun. Oh, I like your changes to theme_menu_local_tasks().

I would RTBC it, except that you missed the accessible headings on garland's tabs. This new patch adds them in.

Also, I just noticed that Seven was rebuilding the tabs to do something similar to Garland. So I've fixed that; it now uses the same technique of copying already-built tabs instead of starting from scratch.

That makes this patch a very slight performance improvement on admin pages. ;-)

Status:Needs review» Needs work

The last submitted patch, menu-local-tabs-599706-30.patch, failed testing.

Arg. Stupid parse error in my patch.

Also, I just noticed overlay module does stuff to the tabs too. Fixing now…

+++ includes/menu.inc
@@ -2045,22 +2045,35 @@ function menu_tab_root_path() {
+    $variables['primary']['#prefix'] = '<h2 class="element-invisible">' . t('Primary tabs') . '</h2>';
...
+    $variables['secondary']['#prefix'] = '<h2 class="element-invisible">' . t('Secondary tabs') . '</h2>';
+++ themes/garland/page.tpl.php
@@ -47,8 +47,8 @@
+          <?php if ($tabs): ?><h2 class="element-invisible"><?php print t('Primary tabs'); ?></h2><?php print render($tabs); ?></div><?php endif; ?>
+          <?php if ($tabs2): ?><h2 class="element-invisible"><?php print t('Secondary tabs'); ?></h2><?php print render($tabs2); ?><?php endif; ?>

Am I missing something? Aren't those headers the same?

Powered by Dreditor.

Status:Needs work» Needs review
StatusFileSize
new7.35 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch menu-local-tabs-599706-34.patch.
[ View ]

Uh… yeah, brain fart when merging our patches. Fixed that now.

The patch also:

  • Fixes overlay_preprocess_page() so that it removes the #primary tabs.
  • Removes a .overlay .primary { display: none; } style from seven/style.css since it is overlay's job to remove the tabs.
  • Removes unneeded markup around the tabs in seven/page.tpl.php

+++ themes/garland/page.tpl.php
@@ -47,8 +47,8 @@
+          <?php if ($tabs): ?><?php print render($tabs); ?></div><?php endif; ?>
+          <?php print render($tabs2); ?>

If we don't need a condition for the second $tabs2, then we also don't need one for the first $tabs.

Powered by Dreditor.

If we don't need a condition for the second $tabs2, then we also don't need one for the first $tabs.

Unfortunately, we do. There's a </div> inside that conditional.

Status:Needs review» Reviewed & tested by the community

d'oh. :)

Title:Allow to alter local tasks/actionsAllow to alter local tasks/actions [fix required]

Status:Reviewed & tested by the community» Closed (won't fix)

Assigned:sun» Unassigned
Status:Closed (won't fix)» Reviewed & tested by the community

Just because someone is upset doesn't mean we have to mark something as won't fix. Someone else is more than welcome to pick up work on a ticket.

Although badly needed, this sounds like D8 material to me.

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

Version:8.x-dev» 7.x-dev
Assigned:Unassigned» sun
Category:task» bug

So it seems that bugs are still considered to be fixed for D7.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs Documentation, +API clean-up

The last submitted patch, menu-local-tabs-599706-34.patch, failed testing.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new8.93 KB
PASSED: [[SimpleTest]]: [MySQL] 26,996 pass(es).
[ View ]

Re-rolled against HEAD.

As JohnAlbin said in #30 "theme_menu_local_tasks() is violating our theme function API. It no longer takes in variables (or render elements) and outputs a string." :(

This really should be fixed IMO.

Priority:Normal» Major
Issue tags:+API change

The required follow-up fix is a small API change, so tagging accordingly.

Title:Allow to alter local tasks/actions [fix required]Allow to alter local tasks/actions
Status:Reviewed & tested by the community» Fixed

Hm. Yeah, this introduced a pretty big WTF here. To be the only theme_X function without $variables and returning a string is bad news.

There are a lot of other changes in this patch that look severe, but I wasn't able to spot any visual changes in clicking around in different themes. And in terms of the $tabs variable, it looks to be outputting exactly what it did before.

So, committed to HEAD. Thanks!

It's marked as an API change. Does this break BC? Please give an explanation of the implication to developers.

Thanks!

@rfay:

Previously, theme_menu_local_tasks() took no $variables, so preprocess functions on it were impossible. Modules can now alter the primary and secondary tabs using preprocess or process functions.

Themes that overrode the old definition of theme_menu_local_tasks() will need to update their theme declaration to mirror the changes to the default implementation in includes/menu.inc.

Status:Fixed» Needs work
Issue tags:-Needs Documentation+Needs Update Documentation

Changing tag/status to reflect that this (I think) needs to be added to the Theme Update 6/7 guide.

I'm assuming that the actual function/template documentation was updated in the patch itself, so we're not needing any in-code documentation at this point?

Two of my modules were hit by this change so I dug around in includes/menu.inc, here is the cure:

These modules use iframes which have their own template.
In template_preprocess for the iframe I changed

$variables['tabs'] = theme('menu_local_tasks');

to

$tabs = menu_local_tabs();
$variables['tabs'] = theme('menu_local_tasks', array('primary' => $tabs['#primary']));

and my tabs reappeared.

dd() is my friend ;-)

Hope this helps someone

Assigned:sun» Unassigned

Status:Needs work» Fixed
Issue tags:-Needs Update Documentation

Thanks! Looks good to me.

Status:Fixed» Closed (fixed)
Issue tags:-API change, -API clean-up

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