Menu router items: Allow to pass PASS_THROUGH to t()

Fixing version. We added PASS_THROUGH in D7.

Also labeling as a regression and re-filing as a bug report, since this was possible in D5.

What about something like this?

Whether we use title options or not is up for discussion. I have amended the patch to support any title callback not just t.

I'd rather see more information in callback arguments - putting a leading "-" in the callback name just seems very unintuitive and odd.

I think in a sense we can alrady do that. Perhaps the problem really lies in this function:
http://api.drupal.org/api/function/_menu_item_localize/7

Some links are getting passed through a callback - but not all, plus the fact that we are later calling check_plain() on everything for the page title. Perhaps we should assume that any callback makes the text safe, and we should call check_plain in any case where there is not a callback?

Note that the definition does not use - in the callback name it's only an internal trick.

#8: title_pass.patch queued for re-testing.

Apologies for jumping in on this thread, but it looks like this might relate to my question posed at: http://drupal.org/node/725524

_menu_item_localize is responsible for translation of both title and description of a menu item, but looking through http://api.drupal.org/api/function/_menu_item_localize/7 , it seems that there is different functionality provided for each, namely:
- Menu titles can specify a custom 'title callback', and that callback (or the default callback, t()) can receive additional 'title arguments'.
- Menu item descriptions, however, always only use t(), and cannot have additional parameters supplied.

This discrepancy seems like a regression too, since previously it was possible to provide inline arguments to both menu item titles and descriptions, such as this:

$items['menuitem1'] = array(
    'title' => t('@module management', array('@module' => MODULE_NAME)),
    'description' => t('View @module configuration', array('@module' => MODULE_NAME)),
    ...

So, is there a case for adding a 'description callback' and 'description arguments' to menu items as well? Should I file that as a new regression bug report? (Sorry, I've not done much to drupal core before... trying to follow protocol!)

That's a feature. We're focused only on this bug here.

Here's a reroll. Not sure how useful this is.

Re-roll, #16 was just missing the setUp() call to enable menu_test.module.

Not sure if I like the approach, using a constant as a key in hook_menu() is uncommon, all other keys are just strings. Also, the - trick seems quite hacky but I can't provide a better solution.

As above, I don't think that's the right approach. I'd do something more like this.

Needs a test or two.

Added test changes, and changed path.inc

#22: title_pass_556910-22.patch queued for re-testing.

I think those DBLog tests happen to fail sometimes... let's try again.

Edit: Oh, pwolanin already tried that. Maybe not then :)

#22: title_pass_556910-22.patch queued for re-testing.

Ok fixed the tests. Couldn't repeat the file failures but the db log I did. Little scary how this works properly. I haven't figured out why this passes but it does.

Test bot please test.

This will never be too pretty but works.

Indeed - looks like we need some cleanup:

to be fixed:

http://api.drupal.org/api/function/_aggregator_category_title/7
http://api.drupal.org/api/function/filter_admin_format_title/7
http://api.drupal.org/api/function/menu_overview_title/7
http://api.drupal.org/api/function/node_type_page_title/7
http://api.drupal.org/api/function/user_page_title/7

already ok:

http://api.drupal.org/api/function/comment_count_unpublished/7
http://api.drupal.org/api/function/shortcut_set_title/7
http://api.drupal.org/api/function/taxonomy_term_title/7
http://api.drupal.org/api/function/taxonomy_admin_vocabulary_title_callb...

unclear - should this really be using t()?

http://api.drupal.org/api/function/field_ui_menu_title/7

Fixed:
http://api.drupal.org/api/function/_aggregator_category_title/7
http://api.drupal.org/api/function/filter_admin_format_title/7
http://api.drupal.org/api/function/menu_overview_title/7
http://api.drupal.org/api/function/node_type_page_title/7
http://api.drupal.org/api/function/user_page_title/7

Can someone explain why

$ grep "node_page_title" . -rn
./modules/node/node.module:1990:function node_page_title($node) {

is not used somewhere? I guess this function needs to be check_plained, too.

Regarding field_ui_menu_title, its not translated for example in field_ui_field_overview_form

      'label' => array(
        '#markup' => check_plain($instance['label']),
      ),

Straight re-roll. Will work on fixing the tests now.

Let's see what the bot says.

I just noticed that format_username() (which is called by user_page_title()) is used very inconsistently throughout core currently. Even though the documentation says it should be check_plain'ed, in a lot of places it's not. Bascially in this patch we have the choice of moving the check_plain into format_username() or into user_page_title(). I'm not really an expert on this, but from a practical standpoint in makes more sense to alleviate all security implications centrally (like in this patch) and deal with double check_plain'ing in a different issue, than keeping it the way it is and fixing all the security issues in a different issue. Doesn't really matter either way, though.

format_username() is supposed to be used for non-html contexts - i.e. a plain text email, so I don't think it makes sense to check_plain() by default.

This one should work. (Broken due to #692044: A site with statistics module enabled is much slower in serving cached pages in D7 than in D6)

Just for the reference: I have no clue, why the bot couldn't install. I tried installing manually and it worked fine.
Running the SimpleTests with the patch applied fixed almost all problems (my machine is too slow, for me to actually run them all, but I tested about 10% and of those 95% were green).

Rerolled patch for #41 (#42 was a crosspost). No other changes. It would be ridiculous if this one doesn't break, but setting to 'needs review' just for the fun of it.

...

I think we agreed on the approach already and tests pass.

#45: drupal_556910-45.patch queued for re-testing.

Ugh. I really, really don't like this approach. This introduces a huge inconsistency into the API:

- When you call drupal_set_title('string'); it's automatically check_plain()ed for you. (a really great security improvement added in D7)
- When you define a menu title callback, you are now responsible for check_plain()ing it. (this was NOT the case in D6 (see menu_overview_title() for example), so we've just *lost* built-in security here.)
- Not to mention we're making API changes a bazillion years after code freeze, that have very strong security implications if they are not strictly followed.

Avoiding built-in security checks should be opt-in, IMO, same as it is when calling drupal_set_title() directly.

Let's discuss some other approaches.

I am not sure I understand this bug. Is it that we want to allow html tags in the the titles - or is it that the placeholders dont work? I cant really replicate the placeholders not working, but I also may just not get the issue.

Using the example from http://api.drupal.org/api/function/node_page_edit/7 the placeholders work fine if I do something like this.

$items['node/%node/edit'] = array(
  'title callback' => 'node_page_edit_title_callback',
  'title arguments' => array(1),
  'page callback' => 'node_page_edit',
  'page arguments' => array(1),
  'access callback' => 'node_access',
  'access arguments' => array('update', 1),
  'theme callback' => '_node_custom_theme',
  'weight' => 0,
  'type' => MENU_LOCAL_TASK,
  'context' => MENU_CONTEXT_PAGE | MENU_CONTEXT_INLINE,
  'file' => 'node.pages.inc',
);

function node_page_edit_title_callback($node) {
  $type_name = node_type_get_name($node);
  return t('Edit @type @title', array('@type' => $type_name, '@title' => $node->title));
}

Is it the html-tag/placeholder mix-in we want or can someone clarify? I removed the em-tags - they will be checkplained, but placeholders work.

The fact that drupal_set_title() automatically performs sanitation of the passed in string is definitely not something we should advance on. It's a constant source of problems and leads to developers making false assumptions about security.

I disagree. Where developers consistently are Doing It Wrong (tm), I think it makes a lot of sense to make the defaults secure. We do this in lots of places, actually. drupal_set_title() in Drupal 7, theme variables, @ and % placeholders in t(). All of which were added for the very reason that people never remember to check_plain() their stuff, and the security team gets overloaded with XSS reports.

I support adding some sort of mechanism to opt-out of this automatic, default security, but I don't support regressing backwards to making it the developer's responsibility. Especially at this point in the release cycle.

I'm a bit torn here. title callback are way less common that drupal_set_title (though could that in part be due to this bug?).

As the instegator of the API change for drupal_set_title(), I'm much less bothered by requiring title callbacks to return safe text.

So, what so we have left? The hack chx suggests would work and avoids a schema change. Hell, we could even backport that to 6 if desired. It's ugly, but I guess only those peering into the innards of menu.inc would be offended.

To nitpick webchick's comment:

Where developers consistently are Doing It Wrong (tm)

In my most humble opinion, if a developer is consistently doing it wrong then there are much larger problems at play.

Security is great, but this is security that takes away needed functionality. If the consensus is the approach taken in this patch is the right way to do it, then any other approach will likely be a hack or just plain WTF. Any fix should solve the problem at its root which it looks like this patch does.

In any case, the bottom line to me is that sometimes you have to sacrifice some security for usability and functionality. Security gets sacrified all the time on the internet, if that weren't true, there would be no internet, as no one would run web servers or any other service because any service is a security risk. There is no such thing as a 100% secure service. And there are so many different ways a Drupal developer can open up their code to vulnerabilities if they don't learn to follow best security practices.

It looks to me like webchick doesn't think this issue is important enough to sacrifice any security, so this should just be a won't fix instead of some ugly hack that someone might *eventually* (who wants to write an ugly hack?) conjure up.

This is the clean solution and we agreed with webchick that security trumps API and schema changes. Note that the API change is permissive: everything works as it did before but if you add a passthrough now you get new behaviour.

There is a fake entry for the home page in the active trail and that had no passthrough defined.

And with less debug.

#59: menu_passthrough.patch queued for re-testing.

Probably not the right person to RTBC but here goes anyway.

Personally I think 'passthrough' is a terrible variable name. Passthrough what?

What about calling it 'escape' or something?

see: http://api.drupal.org/api/function/drupal_set_title/7

I suggested to chx in IRC that the element/column be named something like 'title_output', and that we use the same 2 constants we defined for drupal_set_title, which are CHECK_PLAIN and PASS_THROUGH.

@sun - I'm not sure the options array makes sense in this context - since in many cases the title is not a link (not processed via l()).

@sun also already you need to alter title/title callback/title arguments together, now we add title output.

So this is how we can allow PASS_THROUGH.

Erm, that's some patch mixup. Here's a better one.

In the schema would it make sense to write:

+        'default' => CHECK_PLAIN,

It would.

That's a followup issue isnt it?

Yes, I don't think this patch needs to add such a helper function - though I agree it would make the code nicer throughout menu.inc.

I looked at this patch and while the code is easy to understand and follow, it is a big ugly and, as webchick explained, inconsistent.

I'd like to better understand the use case. We didn't have this in Drupal 6, and as far as I can tell, we did fine without it. Menu titles can also overwritten by drupal_set_title(), and we have the option of using menu title callbacks (i.e. 'title callback' option in hook_menu()).

In other words, I'd like to better understand why this is a critical bugfix versus a feature request. I might be missing something but this looks like it actually might be a non-critical feature request. I'm setting this back to 'needs review' so we can discuss the critical use cases a bit more as that would help me, and other reviewers, understand the issue better.

Well, webchick review's was for another approach. this patch defaults to CHECK_PLAIN. Also, no matter what you do with a title callback you still will be check_plain'd in D6 and HEAD: $title = check_plain(menu_get_active_title()) in drupal_get_title. As for inconsistent, rather it becomes more consistent with the new argument of drupal_get_title -- it even reuses the same constants.

Why critical, it's a regression from D5, and a long standing one because we did not fix this in D6.

This forces page callbacks to manually invoke drupal_set_title(), although manually overriding the page title should be rarely needed.

Calling drupal_set_title() from a page callback is not a crime, as far as I'm concerned, and adding 'title_option' to the menu system is a bit ugly and less transparent.

It would be good to see what page callbacks we'd change. Let's include some conversions in this patch, please. This will provide reviewers some actual real-world examples.

Based on chx's and sun's response, this sounds like a 'major' issue at most, and certainly not a 'critical' issue. I'll wait with demoting this patch for now so we can discuss it some more.

trying to do a couple example conversions locally, but getting an unexpected warning for node/add/X
# Warning: htmlspecialchars() expects parameter 1 to be string, array given in check_plain() (line 1500 of /Users/Shared/www/drupal-7/includes/bootstrap.inc).

@@ -1870,7 +1870,9 @@ function node_menu() {
     $type_url_str = str_replace('_', '-', $type->type);
     $items['node/add/' . $type_url_str] = array(
       'title' => $type->name,
-      'title callback' => 'check_plain',
+      'title callback' => 't',
+      'title arguments' => array('Create @name', array('@name' => $type->name)),
+      'title output' => PASS_THROUGH,
       'page callback' => 'node_add',
       'page arguments' => array(2),
       'access callback' => 'node_access',

Relying on drupal_set_title() during the page callback will give incorrect titles in Shortcut module AFAIK.

Downgrade as per Dries in #79

This issue is a real problem for modules like Shortcut and others that are trying to present menu links with a human-readable meaning without having a menu tree at hand. We have a technically ready patch, but the actual conversion of (un)certain router items seems to trigger failing tests.

I'm going to make an issue summary first, once I have a clear insight on this issue I believe I shall tackle it :)

Ok I believe that it needs more work but it will have to do for now.

Updating Issue summary with details from comments

Updated issue summary.

Well done on the issue summary micnap.

Reroll for D8...

Well, a start anyway. Also attached the .rej files I did not get finished yet in case I don't get back to it in time.

Might as well get the failing tests out of the way too, but should go back to needs work when the bot is done with it.

This isn't a true regression if it wasn't in either of the two current releases of Drupal.

This tries to do a lot in one patch, how about just making the fix and one test usecase of it, the individual conversions are what keep breaking the patch, and they could be done elsewhere.

might be novice to do #94

This might be more of a novice task if I could tell what was part of just making the fix and what was part of the conversion work.

@ZenDoodles seemed like this might be up for grabs. :) unassigning it.

Assigment from Core Office Hours.

Issue is probably not novice anymore.
patch is way to old to apply, hook_menu and drupal_get_title() no longer exist.
Also this might be possible somehow now, because Drupal\Core\Controller\TitleResolver::getTitle() has some code in there that supports variables in page titles.

Menu router items don't support %-style or @-style placeholders for custom title callbacks using t(). Thus check_plain()ed is always used. This doesn't support the kind of functionality needed.

This is not true any longer, the title resolver actually has support for it now.