This is a spin-off from #353208-36: Allow for runtime-conditional local tasks (hook_menu_local_tasks_alter()) 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.
Comment | File | Size | Author |
---|---|---|---|
#46 | drupal.menu-local-tabs.46.patch | 8.93 KB | sun |
#34 | menu-local-tabs-599706-34.patch | 7.35 KB | JohnAlbin |
#30 | menu-local-tabs-599706-30.patch | 5.86 KB | JohnAlbin |
#29 | drupal.menu-local-tabs.29.patch | 5.91 KB | sun |
#27 | menu-local-tabs-599706-27.patch | 4.47 KB | JohnAlbin |
Comments
Comment #1
catchThis is missing a system.api.php hunk. Is it worth adding a rudimentary test at the same time?
Comment #2
sunTagging 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.
Comment #3
sunYou 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().
Comment #5
sunugh, 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. :-/
Comment #6
sunWe need to keep this @todo until #600974: theme() fails for theme functions taking one argument in renderable arrays is resolved.
Comment #7
dmitrig01 CreditAttribution: dmitrig01 commentedInstalled on my local Drupal install and sun's hook - working and looks great (I read the code too).
Should go in ASAP - ctools blocker
Comment #8
dmitrig01 CreditAttribution: dmitrig01 commentedone little thing in a comment but sun's on it
Comment #9
sunComment #10
dmitrig01 CreditAttribution: dmitrig01 commentedComment #11
webchickThis 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:
Comment #12
effulgentsia CreditAttribution: effulgentsia commentedNot 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.
Comment #13
effulgentsia CreditAttribution: effulgentsia commentedSorry, cross-posted with webchick.
Comment #14
sunDiscussed 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. :-/
Comment #15
webchickOk, 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.
Comment #16
suntobiasb 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...
Comment #17
tobiasbsmall follow-up
Comment #18
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #19
moshe weitzman CreditAttribution: moshe weitzman commentedI 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.
Comment #20
sunParsing help:
http://api.drupal.org/api/function/garland_menu_local_tasks/7
http://api.drupal.org/api/function/garland_preprocess_page/7
http://api.drupal.org/api/function/seven_preprocess_page/7
Comment #21
sunScope-creep for this issue, anyway.
Comment #22
tobiasbSorry Tha_sun :D
Comment #23
catch#653068: Update documentation is incomplete
Comment #24
JohnAlbinOk, 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').
Comment #25
moshe weitzman CreditAttribution: moshe weitzman commentedNice 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.
Comment #26
sun1) 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!
Comment #27
JohnAlbinIf 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:
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?
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.
Comment #28
sun#27: menu-local-tabs-599706-27.patch queued for re-testing.
Comment #29
sunRe-rolled against HEAD.
AFAICS, there's no substantial API change here --
print render($tabs);
still works the same.Comment #30
JohnAlbin@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. ;-)
Comment #32
JohnAlbinArg. Stupid parse error in my patch.
Also, I just noticed overlay module does stuff to the tabs too. Fixing now…
Comment #33
sunAm I missing something? Aren't those headers the same?
Powered by Dreditor.
Comment #34
JohnAlbinUh… yeah, brain fart when merging our patches. Fixed that now.
The patch also:
.overlay .primary { display: none; }
style from seven/style.css since it is overlay's job to remove the tabs.Comment #35
sunIf we don't need a condition for the second $tabs2, then we also don't need one for the first $tabs.
Powered by Dreditor.
Comment #36
JohnAlbinUnfortunately, we do. There's a
</div>
inside that conditional.Comment #37
sund'oh. :)
Comment #38
sunComment #39
sunComment #40
Dave ReidJust 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.
Comment #41
sunAlthough badly needed, this sounds like D8 material to me.
Comment #42
sunComment #43
sunSo it seems that bugs are still considered to be fixed for D7.
Comment #44
sun#34: menu-local-tabs-599706-34.patch queued for re-testing.
Comment #46
sunRe-rolled against HEAD.
Comment #47
JacineAs 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.
Comment #48
sun#46: drupal.menu-local-tabs.46.patch queued for re-testing.
Comment #49
sun#46: drupal.menu-local-tabs.46.patch queued for re-testing.
Comment #50
sunThe required follow-up fix is a small API change, so tagging accordingly.
Comment #51
webchickHm. 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!
Comment #52
rfayIt's marked as an API change. Does this break BC? Please give an explanation of the implication to developers.
Thanks!
Comment #53
JohnAlbin@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.
Comment #54
jhodgdonChanging 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?
Comment #55
hutch CreditAttribution: hutch commentedTwo 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
Comment #56
sunComment #57
catchAdded http://drupal.org/update/themes/6/7
Comment #58
jhodgdonThanks! Looks good to me.