Hook_menu_alter() not supported by potx

Well, it is called at around the same time as hook_menu() from http://api.drupal.org/api/function/menu_router_build/6, so you should not use t() directly on the titles and descriptions. However, the structure of hook_menu_alter() is way more fluid compared to what we can enforce for hook_menu(), so I'm not sure what pattern can we look for to extract the translatable strings. Any idea?

Ps. an immediate workaround is to do code like this:

function mymodule_menu_alter(&$items) {
  $items['someitem']['title'] = 'Modified title';
  return;  
  // potx has no direct support for this hook.
  t('Modified title');
}

This is ugly code, but once again, to solve it, we need to have a generic pattern to identify titles and descriptions in alter hooks.

Yeah, well yours is one way to modify items. I've seen some modules using the way outlined in #2, as in specifying array keys one by one. There are most probably all kinds of other creative ways people use there. Not sure how to get that in order.

#969172: Hook_menu_alter() not supported in potx has been marked as duplicate of this issue

Here is a patch that:

1. Add support for $items['node/%node/something']['title'] = 'Title'; type of title and description formats to hook_menu() additionally to the existing support.
2. Add support for $items = array('node/%node/something' => array('title' => 'Title'); support to hook_menu_alter(), as well as support for the there more likely format in (1).

So this extends hook_menu() format awareness to less common formats (though we've seen those used in core in some repeatedly added menu patterns). Plus it also adds the same level support for hook_menu_alter() covering the two common cases for that as discussed above. I tested this on Drupal 7 with the comment.module code. Before the patch it did not extract the hook_menu_alter() items, after it did:

#: comment.module:295
msgid "Administer content and comments."
msgstr ""

#: comment.module:299
msgid "Comment fields"
msgstr ""

#: comment.module:301
msgid "Comment display"
msgstr ""

Reviews welcome.

Heh. :) @Gábor Hojtsy, I've really tried to, but this is kind of impossible to review. ;)

Fundamentally, the code looks good (despite various coding style issues and missing inline comments); ideally we'd prove with unit tests, I guess. :)

Yeah, well, while this is a Drupal 7 module patch, the include file in fact tries to be language independent (for easier patching across Drupal versions, because all potx versions should be able to parse any Drupal version code). So the code style applied is on the D6 style. Indeed, we should probably expand our existing unit test coverage to cover this as well, right.

Here is a quick update with tests expanded. Could not run the tests on my machine for some I think unrelated reason (DatabaseConnectionNotDefinedException: The specified database connection is not defined: simpletest_original_default in Database::openConnection()....). Will try to come back to running tests (possibly reinstalling my dev site for this) later. Unless someone beats me to verifying the tests pass :)

Uhm, the error was due to a bad setUp() method. Here are the tests actually fixed so it should pass tests now (it does on my machine).

+++ potx.inc	24 Jan 2011 15:51:51 -0000
@@ -1083,32 +1083,57 @@ function _potx_find_context($tf, $ti, $f
+  ¶
...
+          ¶

Trailing white-space.

+++ potx.inc	24 Jan 2011 15:51:51 -0000
@@ -1083,32 +1083,57 @@ function _potx_find_context($tf, $ti, $f
+          if ($_potx_tokens[$tn][0] == T_CONSTANT_ENCAPSED_STRING && in_array($_potx_tokens[$tn][1], $keys) && $_potx_tokens[$tn+1][0] == T_DOUBLE_ARROW) {
...
+                $_potx_tokens[$tn+2][2]
...
+              $tn+=2; // Jump forward by 2.
...
+          if (is_string($_potx_tokens[$tn]) && $_potx_tokens[$tn] == '[' && $_potx_tokens[$tn+1][0] == T_CONSTANT_ENCAPSED_STRING && in_array($_potx_tokens[$tn+1][1], $keys) && is_string($_potx_tokens[$tn+2]) && $_potx_tokens[$tn+2] == ']') {
...
+                $_potx_tokens[$tn+4][2]
...
+              $tn+=4; // Jump forward by 4.

Missing space before and after operator.

+++ potx.inc	24 Jan 2011 15:51:51 -0000
@@ -1083,32 +1083,57 @@ function _potx_find_context($tf, $ti, $f
+              potx_status('error', t('Invalid menu %element definition found in %hook. Title and description keys of the menu array should be literal strings.', array('%element' => $_potx_tokens[$tn][1], '%hook' => $filebase . $hook .'()')), $file, $_potx_tokens[$tn][2], NULL, 'http://drupal.org/node/323101');
...
+              potx_status('error', t('Invalid menu %element definition found in %hook. Title and description keys of the menu array should be literal strings.', array('%element' => $_potx_tokens[$tn+1][1], '%hook' => $filebase . $hook .'()')), $file, $_potx_tokens[$tn+1][2], NULL, 'http://drupal.org/node/323101');

1) Would be good to use multi-line array syntax for the t() token replacements here.

2) Missing space before and after operator.

+++ tests/potx.test	24 Jan 2011 15:51:51 -0000
@@ -17,10 +17,8 @@ class PotxTestCase extends DrupalWebTest
+    parent::setUp('locale', 'potx');

Modules should be passed in an array().

+++ tests/potx_test_7.module	24 Jan 2011 15:51:51 -0000
@@ -19,9 +19,23 @@ function potx_test_7_menu() {
+function potx_test_7_menu_alter(&$items) {
+  $items['translate/test-empty']['description'] = 'Test menu item description altered';
+  $items['translate/test-empty'] = array_merge(
+    $items['translate/test-empty'],
+    array(
+      'title' => 'Test menu item title altered'

Let's add another alter that uses the double-quote syntax.

Powered by Dreditor.

Ok, concentrating on functionality stuff for now, we should do a more thorough operator whitespace fix for the whole file. These $tn+3 issues are all around :)

For the setUp() method, looks like core is not in agreement with itself... I have Drupal 7.0 and aggregator and menu uses the argument list syntax, while field_ui module for example uses arrays. Now what?

I'm glad to add yet another item to the menu_alter() test, sure, will post once we figure out this setUp() question :)

Passing multiple string arguments to setUp() dates back to SimpleTest for D5 or something. The new and recommended way is to pass a single array containing the list of modules to enable. DrupalWebTestCase::setUp() contains a @todo for D8 to remove support for the old way. SimpleTest for D6 has been updated to support the single array, too.

Ok, here is a patch updated with one more description item in the alter and the setup fixed. Trailing whitespace also fixed. Other whitespace to be fixed in a different issue. Committing this to Drupal 7.x version.

Ok, committed to 7.x-1.x, needs to be ported and committed to 6.x-3.x.

Ok, bacported to Drupal 6.

If using the array() syntax for setUp() (using the latest 6.x-2.11 simpletest), it gets me 5 exceptions saying "array_flip() [function.array-flip]: Can only flip STRING and INTEGER values! (in install.inc line 313)". So looks like the backport of this is not yet in there. So keeping the non-array syntax, which makes the tests pass in their entirety.

Now committed to Drupal 6 too. About to roll out on l.d.o. (Old releases will not be reparsed, but I'll kick off a Drupal 7.0 reparse manually). Contrib modules will take advantage of this feature with their next release parsed on l.d.o.

For enterprising individuals, you can look at #1038286: String context not identified in hook_menu and hook_menu_alter title_arguments but that has a workaround suggested, and given core did not use contexts for menus yet, we can popularize the workaround for now and make our lives much easier :)

Thank you, @Gábor Hojtsy!

Follow-up: #1542144: Mark strings as localizable/translatable (new t()-alike string function that isn't t(), only for potx)