Problem/Motivation
Currently, the title of a menu item has the ability to be determined dynamically via a callback function, but the description does not.
Allowing it to be dynamic would be useful for, e.g., contextual links, where it would be nice to have the link say "Configure the 'XYZ' block" rather than just "Configure this block".
Proposed resolution
Add a 'description callback' and 'description arguments' to hook_menu()/hook_menu_alter()
Remaining tasks
As outlined in #26:
Update the D8 patch to include the update hook form #16, but wrap the two additions in aif (!db_field_edists())
.Commit to D8.Create a D7 backport with the same update hook, renamed, and add 7.0->7.x (and possibly 6->7) upgrade path test coverage for the update.- Commit to D7.
- Create another D8 patch to update all the 7.x dumps in D8 with this new schema.
- Potentially remove the update hook from D8 in the same patch.
- Commit to D8.
User interface changes
N/A
API changes
API Addition: Add a 'description callback' and 'description arguments' to hook_menu()/hook_menu_alter()
Original report by David_Rothstein
Currently, the title of a menu item has the ability to be determined dynamically via a callback function, but the description does not.
Allowing it to be dynamic would be useful for, e.g., contextual links, where it would be nice to have the link say "Configure the 'XYZ' block" rather than just "Configure this block".
Also, it would allow fixing up the following code that is currently in node_menu():
// @todo Remove this loop when we have a 'description callback' property.
// Reset internal static cache of _node_types_build(), forces to rebuild the
// node type information.
drupal_static_reset('_node_types_build');
foreach (node_type_get_types() as $type) {
$type_url_str = str_replace('_', '-', $type->type);
$items['node/add/' . $type_url_str] = array(
'title' => $type->name,
'title callback' => 'check_plain',
'page callback' => 'node_add',
'page arguments' => array(2),
'access callback' => 'node_access',
'access arguments' => array('create', $type->type),
'description' => $type->description,
'file' => 'node.pages.inc',
);
}
Comment | File | Size | Author |
---|---|---|---|
#57 | drupal-687842-57.patch | 11.86 KB | tim.plunkett |
#55 | drupal-687842-55.patch | 12.22 KB | tim.plunkett |
#54 | drupal8.menu-description-callback.54.patch | 12.22 KB | sun |
#54 | interdiff.txt | 3.04 KB | sun |
#53 | drupal-687842-53.patch | 12.3 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettHere's a copy/paste based attempt at this.
D7 still has function_exists, uploading this as well.
Comment #2
tim.plunkettPossibly wishful thinking.
Comment #4
tim.plunkettLet's see how this works.
Comment #6
tim.plunkettOh, forgot a hook_update_N. At least #4 tests-only shows that the tests fail, now just to get them all to pass.
Comment #8
tim.plunkettUgh, sorry @David_Rothstein and other mystery follower, I'm not trying to flood your tracker.
Comment #10
tim.plunkettOkay, I reran those failing tests locally and I think this will work.
Comment #11
tim.plunkettAdding #1490418: Convert node/add/TYPE to use %node_type as a D8-only follow-up.
Comment #12
tim.plunkettUpdated D7 backport, just so I can use it.
Comment #13
xjmI like this patch. The overall approach looks good to me, it adds useful parity and functionality, it has tests, and it's a straightforward API addition.
Nitpicks:
(And subsequent lines) Some inline comments would be nice with all the logic in this hunk (especially for the last line which has sufficient issets and square brakets to be pretty hard to read).
Y SO MANY CAPITALS
This should probably have a period after
t()
.Dumb thing: I think we settled on imperative/infinitive form for the verbs for the update hooks (since they're user-facing). See under: http://drupal.org/node/1354#hookimpl
We probably should add an upgrade path test for this update. However, if we are backporting this, we should kill this update hook and instead put the update hook and tests only in the backport.
Leaving at NR for, like, stuff.
Comment #14
tim.plunkettAddressed 1, 2, 3, and 4.
I left in system_update_8005, but it can be easily removed if need be. I'm not sure how that works with the backport policy.
+++ b/includes/menu.incundefined
@@ -712,11 +712,29 @@ function _menu_item_localize(&$item, $map, $link_translate = FALSE) {
+ if ($link_translate && isset($original_description) && isset($item['options']['attributes']['title']) && $item['options']['attributes']['title'] == $original_description) {
+ $item['localized_options']['attributes']['title'] = $item['description'];
I added an @todo for this, because I actually have no idea what its doing. Its just a carry-over from the current code.
Comment #16
tim.plunkettWhoops, looks like system_update_8005 was added already.
Comment #17
tim.plunkettComment #19
xjmwebchick condirmed that she thinks this is OK for backport. So the correct way to handle the update hook would be:
Regarding the
@todo
:So, putting this in English:
then make the localized title the same as the translated description.
How about:
// If the title and description are the same, use the translated description as a localized title.
I dunno why there's no else case to try to translate the title if they're different, but I'm no translator.
Comment #20
dcrocks CreditAttribution: dcrocks commentedSmall niggle. I would expect the case in #19 to be very rare. Recognizing that Drupal's 'title' field is actually used as the 'text' part of an 'a' element and Drupal's 'description' field is placed in the 'title' attribute of an 'a' element, title and description shouldn't be considered to be the same thing, as they are separately styled parts of an element. I use rather verbose descriptions, because, after all, they are Descriptions, as an example. I think their would be unexpected results if you do what was suggested in #19.
Comment #21
tim.plunkett@dcrocks, that code is already in there. It's just that its finally being documented as part of this addition.
Comment #22
xjmHuh? This is not a suggestion. This is a line that's already in core, and we are just adding a comment explaining it because we're adding additional logic before it.
Comment #23
xjmxpost; Re-tagging for tests for the D7 tests. (The D8 patch just needs the update hook removed and I guess my suggested comment added in place of the @todo, and then I think it's RTBC.)
Comment #24
tim.plunkettAs per #19 part 1.
Comment #26
xjmSo #24 illustrates an interesting chicken-and-egg problem:
I talked to @catch, @tim.plunkett, and @swentel about this issue this morning, and we came up with the following "solution":
if (!db_field_edists())
.This workflow is... a little crazy. It should work fine, but I feel like we should at least document this somewhere, and maybe brainstorm some other ways of dealing with backported schema changes.
Comment #27
tim.plunkettOkay, with !db_field_exists() now.
Comment #28
xjmYep!
Comment #29
David_Rothstein CreditAttribution: David_Rothstein commentedSorry, I meant to review this earlier but dropped the ball :)
I found one main issue to set this back to needs work, which is that the hook_menu() documentation needs to be updated for these new parameters.
Other than that, a couple minor things:
Typo ("desscription").
Based on this hook_menu() entry:
First, they probably shouldn't be asserting with t() if the callback never actually used t()? (Probably doesn't matter in practice though.)
Second, the test might be improved slightly if the description text were something where check_plain() actually affected it? For example,
<strong>Menu item description arguments</strong>
and then assert that the HTML tags are actually escaped in the final page output...Otherwise, all looks great to me!
Comment #30
kgoel CreditAttribution: kgoel commentedComment #31
xjmHi kgoel,
Are you still looking into this issue?
Comment #32
kgoel CreditAttribution: kgoel commentedSorry for taking long. I have worked on all the minor updates but unclear about hook_menu documentation as where to add.
Comment #33
tim.plunkettLook in core/modules/system/system.api.php, line 811.
See http://api.drupal.org/api/search/8/hook_menu
If you're still unclear, just post what you have so far and I'll see if I can help.
Comment #34
kgoel CreditAttribution: kgoel commentedThanks Tim Plunkett! I will check the link.
Comment #35
tim.plunkettRerolled with the feedback from #29.
Comment #37
tim.plunkettDidn't notice that there were 3 db updates added since the last reroll, so
system_update_8006()
was defined twice.Comment #38
tstoecklerLooks good, couldn't find anything to complain about. Could use another look though, before RTBC.
Comment #39
David_Rothstein CreditAttribution: David_Rothstein commentedWouldn't this be better switched to
$this->assertRaw(check_plain('<strong>Menu item description arguments</strong>'));
?Comment #40
tim.plunkettGood point.
Made both changes.
Comment #41
no_commit_credit CreditAttribution: no_commit_credit commentedComment #42
xjmThe patch looks good to me, so I think it's RTBC (after a couple other nitpicks) unless David finds something else wrong with it. ;)
The above reroll adjusts the following:
81 characters. We could fix it by omitting the quotes, which would be consistent with our list formatting standards (http://drupal.org/node/1354#lists) but inconsistent with the rest of the docblock. I vote for the latter, as there's a docs issue somewhere to clean up the existing list formatting.
More silly capitals. :P
Comment #43
tim.plunkettI think I made this change, wrapped the lines, and then added the quotes in to match without rewrapping. I agree this should be done here.
Comment #44
David_Rothstein CreditAttribution: David_Rothstein commentedI skimmed it over, and the patch looks good to me also.
Comment #45
catchThis makes sense to me. I'm wondering a bit how dynamic titles and descriptions will work with #916388: Convert menu links into entities looking at it, but we already have dynamic titles for menu links so this doesn't make it any worse on that front. Haven't actually reviewed the most recent patch yet.
Comment #46
catch#41: menu-description-687842-41.patch queued for re-testing.
Comment #48
tim.plunkettConflicted with the addition of system_update_8009 and system_update_8010. Rerolled.
Comment #49
tstoecklerI gave this another lookover and it looks good.
Was RTBC before so must be RTBC now. :)
Comment #50
sun#48: drupal-687842-48.patch queued for re-testing.
Comment #51
tim.plunkettMoving the test around post-PSR-0. Same exact diffstat, leaving RTBC.
Comment #53
tim.plunkettRenumbering the hook_update_N function
Comment #54
sunThanks for keeping this hot, @tim.plunkett!
Fixed a couple of very minor things. Back to RTBC.
Comment #55
tim.plunkettsystem_update_8012() was added in #1496542: Convert site information to config system, rerolled, still RTBC.
Comment #56
catchThanks. Committed/pushed this to 8.x.
Moving back to 7.x for backport, we'll see if David still thinks it's a good idea :)
Comment #57
tim.plunkettRerolled.
Comment #58
tim.plunkettI updated the issue summary, since we have a rather confusing path forward.
Next is "add 7.0->7.x (and possibly 6->7) upgrade path test coverage for the update."
Comment #59
sunI'm not sure whether it is a good idea to backport this. It looks like it might be possible, but backporting schema changes and especially menu/router system changes is always hairy.
It looks more important to me to continue with #1490418: Convert node/add/TYPE to use %node_type
Comment #60
tim.plunkettI don't have the time to work on backporting this. Leaving it open though.
Comment #61
tim.plunkettThis is REALLY not worth the effort, #26 illustrates how complex it would be.
Comment #62.0
(not verified) CreditAttribution: commentedUpdated issue summary.