Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

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?

sun’s picture

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.

sun’s picture

Status: Needs work » Needs review
FileSize
13.19 KB

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.

sun’s picture

Status: Needs work » Needs review
FileSize
13.98 KB

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

sun’s picture

dmitrig01’s picture

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

dmitrig01’s picture

Status: Reviewed & tested by the community » Needs work

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

sun’s picture

Status: Needs work » Needs review
FileSize
13.97 KB
dmitrig01’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

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),
effulgentsia’s picture

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.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work

Sorry, cross-posted with webchick.

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
14.95 KB

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

webchick’s picture

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.

sun’s picture

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

tobiasb’s picture

Status: Needs work » Needs review
FileSize
579 bytes

small follow-up

Dries’s picture

Status: Needs review » Fixed

Committed to CVS HEAD. Thanks.

moshe weitzman’s picture

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.

sun’s picture

Status: Needs work » Fixed

Scope-creep for this issue, anyway.

tobiasb’s picture

Status: Fixed » Needs work

Sorry Tha_sun :D

This needs documenting in the module upgrade guide.
catch’s picture

Priority: Critical » Normal
JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
4.46 KB

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

moshe weitzman’s picture

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.

sun’s picture

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!

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
4.47 KB

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:

  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.

sun’s picture

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

sun’s picture

Re-rolled against HEAD.

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

JohnAlbin’s picture

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

JohnAlbin’s picture

Arg. Stupid parse error in my patch.

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

sun’s picture

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

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
7.35 KB

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
sun’s picture

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

JohnAlbin’s picture

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.

sun’s picture

Status: Needs review » Reviewed & tested by the community

d'oh. :)

sun’s picture

Title: Allow to alter local tasks/actions » Allow to alter local tasks/actions [fix required]
sun’s picture

Status: Reviewed & tested by the community » Closed (won't fix)
Dave Reid’s picture

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.

sun’s picture

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

sun’s picture

Version: 7.x-dev » 8.x-dev
sun’s picture

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.

sun’s picture

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

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.

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
8.93 KB

Re-rolled against HEAD.

Jacine’s picture

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.

sun’s picture

#46: drupal.menu-local-tabs.46.patch queued for re-testing.

sun’s picture

#46: drupal.menu-local-tabs.46.patch queued for re-testing.

sun’s picture

Priority: Normal » Major
Issue tags: +API change

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

webchick’s picture

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!

rfay’s picture

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

Thanks!

JohnAlbin’s picture

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

jhodgdon’s picture

Status: Fixed » Needs work
Issue tags: -Needs documentation +Needs documentation updates

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?

hutch’s picture

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

sun’s picture

Assigned: sun » Unassigned
catch’s picture

Status: Needs work » Fixed
Issue tags: -Needs documentation updates
jhodgdon’s picture

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.