Add a "description callback" property to menu items

Here's a copy/paste based attempt at this.

D7 still has function_exists, uploading this as well.

Possibly wishful thinking.

Let's see how this works.

Oh, forgot a hook_update_N. At least #4 tests-only shows that the tests fail, now just to get them all to pass.

Ugh, sorry @David_Rothstein and other mystery follower, I'm not trying to flood your tracker.

Okay, I reran those failing tests locally and I think this will work.

Adding #1490418: Convert node/add/TYPE to use %node_type as a D8-only follow-up.

Updated D7 backport, just so I can use it.

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

1. +++ b/core/includes/menu.incundefined
   @@ -699,11 +699,29 @@ function _menu_item_localize(&$item, $map, $link_translate = FALSE) {
   +  if (!empty($item['description_arguments']) || !empty($item['description'])) {
   

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

2. +++ b/core/modules/simpletest/tests/menu.testundefined
   @@ -180,6 +180,16 @@ class MenuRouterTestCase extends DrupalWebTestCase {
   +    $this->assertText(t('Menu Item Description Text'));
   +    $this->assertText(t('Menu Item Description Arguments'));
   
   +++ b/core/modules/simpletest/tests/menu_test.moduleundefined
   @@ -22,6 +22,28 @@ function menu_test_menu() {
   +    'title' => 'Menu Item Title',
   ...
   +    'description' => 'Menu Item Description Parent',
   ...
   +    'title' => 'Menu Item with a regular description',
   ...
   +    'description' => 'Menu Item Description Text',
   ...
   +    'description arguments' => array('Menu Item Description Arguments'),
   

   Y SO MANY CAPITALS

3. +++ b/core/modules/system/system.installundefined
   @@ -1109,6 +1109,20 @@ function system_schema() {
   +        'description' => 'A function which will alter the description. Defaults to t()',
   
   @@ -1761,6 +1775,28 @@ function system_update_8004() {
   +    'description' => 'A function which will alter the description. Defaults to t()',
   

   This should probably have a period after t().

4. +++ b/core/modules/system/system.installundefined
   @@ -1761,6 +1775,28 @@ function system_update_8004() {
   + * Adds description_callback and description_arguments fields to {menu_router}.
   

   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

5. +++ b/core/modules/system/system.installundefined
   @@ -1761,6 +1775,28 @@ function system_update_8004() {
   +function system_update_8005() {
   

   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.

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

Whoops, looks like system_update_8005 was added already.

webchick condirmed that she thinks this is OK for backport. So the correct way to handle the update hook would be:

1. Roll a D8 patch with no update hook.
2. Get the D8 patch in, or at least to RTBC to avoid parallel rerolls.
3. Create a D7 patch with an update hook, 7.0->7.x upgrade path tests, and probably 6.x->7.x upgrade path tests.
4. Review the D7 patch (and probably manually test the upgrade path).
5. Get the patch in D7.

Regarding the @todo:

+++ b/core/includes/menu.incundefined
@@ -699,11 +699,33 @@ function _menu_item_localize(&$item, $map, $link_translate = FALSE) {
+  // @todo.
+  if ($link_translate && isset($original_description) && isset($item['options']['attributes']['title']) && $item['options']['attributes']['title'] == $original_description) {
+    $item['localized_options']['attributes']['title'] = $item['description'];

So, putting this in English:

• If the link is being translated,
• and it has a description,
• and it has a title,
• and the title is equal to the description

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.

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

@dcrocks, that code is already in there. It's just that its finally being documented as part of this addition.

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

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

xpost; 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.)

As per #19 part 1.

So #24 illustrates an interesting chicken-and-egg problem:

• The patch assumes the added db fields already exist in the database when upgrading from D7 to D8, because we are planning to backport this to D7.
• However, the fields do not actually exist yet in D7, because this has not been backported yet.
• Furthermore, the D7 dumps for the 7->8 upgrade path tests don't contain the field either (because they'd need to be updated once D7 includes this field).
• So, in order for the 7->8 upgrade path tests above to pass, this fix would need to be included in D7 first, violating our backport policy.

I talked to @catch, @tim.plunkett, and @swentel about this issue this morning, and we came up with the following "solution":

1. Update the D8 patch to include the update hook form #16, but wrap the two additions in a if (!db_field_edists()).
2. Commit to D8.
3. 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.
4. Commit to D7.
5. Create another D8 patch to update all the 7.x dumps in D8 with this new schema.
6. Potentially remove the update hook from D8 in the same patch.
7. Commit to D8.

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.

Okay, with !db_field_exists() now.

Yep!

Hi kgoel,

Are you still looking into this issue?

Sorry for taking long. I have worked on all the minor updates but unclear about hook_menu documentation as where to add.

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

Thanks Tim Plunkett! I will check the link.

Rerolled with the feedback from #29.

Didn't notice that there were 3 db updates added since the last reroll, so system_update_8006() was defined twice.

Looks good, couldn't find anything to complain about. Could use another look though, before RTBC.

Good point.
Made both changes.

The 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:

+++ b/core/modules/system/system.api.phpundefined
@@ -691,6 +691,10 @@ function hook_menu_get_item_alter(&$router_item, $path, $original_map) {
+ *   - "description arguments": Arguments to send to t() or your custom callback,

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.

+++ b/core/modules/system/tests/modules/menu_test/menu_test.moduleundefined
@@ -22,6 +22,28 @@ function menu_test_menu() {
+    'title' => 'Menu Item Title',
...
+    'title' => 'Menu Item with a regular description',
...
+    'title' => 'Menu Item with a description set with a callback',

More silly capitals. :P

+++ b/core/modules/system/system.api.phpundefined
@@ -693,9 +693,9 @@ function hook_menu_get_item_alter(&$router_item, $path, $original_map) {
- *   - "description callback": Function to generate the description; defaults to
+ *   - description callback: Function to generate the description; defaults to
  *     t(). If you require only the raw string to be output, set this to FALSE.
- *   - "description arguments": Arguments to send to t() or your custom callback,
+ *   - description arguments: Arguments to send to t() or your custom callback,

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

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

#41: menu-description-687842-41.patch queued for re-testing.

Conflicted with the addition of system_update_8009 and system_update_8010. Rerolled.

I gave this another lookover and it looks good.
Was RTBC before so must be RTBC now. :)

#48: drupal-687842-48.patch queued for re-testing.

Moving the test around post-PSR-0. Same exact diffstat, leaving RTBC.

Renumbering the hook_update_N function

Thanks for keeping this hot, @tim.plunkett!

Fixed a couple of very minor things. Back to RTBC.

system_update_8012() was added in #1496542: Convert site information to config system, rerolled, still RTBC.

Thanks. 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 :)

Rerolled.

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

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

I don't have the time to work on backporting this. Leaving it open though.

This is REALLY not worth the effort, #26 illustrates how complex it would be.