Vocabulary title HTML entities are double encoded

#1: taxonomy-vocab-title-encode-1555294-1.patch queued for re-testing.

Patch works for me. The property title is automatically escaped in hook_menu .

This needs to be rerolled for D8 first.

Same patch for D8

works for me

Makes sense, and matches the other title callbacks I've seen (user_page_title, node_page_title, menu_test_title_callback, filter_admin_format_title, menu_overview_title)

Thanks. Committed/pushed to 8.x.

I just tested the patch and it's working on D7. Thanks !

Here's the updated patch.

This is a public function that could be called from other contexts, so for Drupal 7 it's probably not safe to change its behavior in this way.

I'd suggest backporting it via a method like #1363358-6: Shortcut set titles are double-escaped with check_plain() instead - and while doing so, feel free to review/RTBC that patch :)

Hi David,

What's the difference between your method and the one here ?

Thanks.

My method does not change the output of an existing API(-ish) function; rather, it adds a new function.

True true... but is there a way to verify that this function is used by a module ? Or we just can't change the API now ?

Is this patch better ?

+++ b/modules/taxonomy/taxonomy.moduleundefined
@@ -382,6 +382,14 @@ function taxonomy_admin_vocabulary_title_callback($vocabulary) {
+ * Return the vocabulary name given the vocabulary object.
+ * This function is now used as a menu item title callback.

There are docs at http://drupal.org/node/1354 about how to document hook_menu callbacks.

Or, you could just mimic #1363358-6: Shortcut set titles are double-escaped with check_plain() because it does it properly.
Also mimic the marking of the old function as deprecated.

Right, exactly. It's a public function and we don't know who is using it or how, so it's best not to change its behavior in a stable release.

The patch looks like the right idea; I don't think "taxonomy_admin_vocabulary_set_title" is the right function name though (since it's not really setting anything); how about just "taxonomy_vocabulary_title_callback"? I also think we should add a note to the old function saying it's deprecated, like I did in the other issue; you can feel free to borrow the text I wrote there, or tweak/improve it as necessary.

Sorry, crosspost.

Thanks for your advices :-)

Patch updated !

Looks pretty good on a quick glance, except:

+ * Returns the sanitized title of a shortcut set.

Too much copy-paste :)

Also, this got me thinking:

+ * directly if you need a sanitized title. In Drupal 8, this function will be
+ * restored as a title callback and therefore will no longer sanitize its
+ * output.
  */
 function taxonomy_admin_vocabulary_title_callback($vocabulary) {
   return check_plain($vocabulary->name);

Frankly I think taxonomy_vocabulary_title_callback() is a better name, so it may make sense for Drupal 8 to just remove taxonomy_admin_vocabulary_title_callback() instead. We don't necessarily have to figure that out now except that it affects the code comments for the Drupal 7 patch... Perhaps that sentence isn't necessary and could just be removed anyway.

Updated patch.

I disagree here, hooks, page callbacks, form callbacks, etc. are *not* API functions. Here we just have the case of a menu title callback (explicitly documented as such) that has been implemented incorrectly. I don't see the harm in changing its behavior.

If another module is using this function for what it is not (ie. using it as something different then a menu title callback), it's a bug in that other module.

I would argue the opposite. If it's not prefixed with '_' and it's in the .module file, it's a public API due to our nature.

As an alternative here, why not use this instead, which would be perfectly backportable to D7 rather than adding a new function:

  'title callback' => 'entity_label',
  'title arguments' => array('taxonomy_vocabulary', 3),

I made the original patch mimic #973328: Special characters are encoded twice in taxonomy term title, which is a similar issue. Why was it okay to change that callback function (taxonomy_term_title) which is used in exactly the same way as this one, but not okay to change this one, where the function seems much more specific than the one that was already changed? Should that one not have been changed?

Hm, I would tend to think that other one was a mistake, yes. Although since it was committed only a couple weeks after Drupal 7.0 was released (when not that many people were actually using Drupal 7 yet on real sites), it was less of a risk than doing it now would be :)

As an alternative here, why not use this instead, which would be perfectly backportable to D7 rather than adding a new function:

'title callback' => 'entity_label',
  'title arguments' => array('taxonomy_vocabulary', 3),

Ah! That sounds like a very good idea.

Shame this isn't forwardportable, since entity_label is deprecated in D8.

Ah, shoot, now I'm wondering... is it actually safe to change 'title arguments' like that?

If anyone is using hook_menu_alter() to swap in a different title callback, then this is going to result in a totally different set of parameters being passed to their function. Sorry for not thinking of that earlier.

I don't know about you, but when I hook_menu_alter the arguments, I also explicitly duplicate the callback for that exact reason.

True, it's definitely best practice to alter both if you need to alter one.

Also, we're only talking about the title of an admin page here, so I guess it wouldn't be that big of a deal if it breaks for someone. I'm OK with going ahead with this approach.

I agree the patch #26 is nice approach... but I you think changing taxonomy_admin_vocabulary_title_callback was a problem because it can possibly break some website this patch can do it to.

That say they will break website differently. In the first patch it can introduce XSS. This one will just break the admin... for me it's acceptable as their is a very small chance than it's append and the consequences are not that bad (compared to the unreadable title we have now).

Let's commit this!

This is forward portable for D8. We have a valid use case here that I've already identified when we tried to completely remove entity_label() that we need title callback support. I think it's going to be renamed to entity_page_title() for title support.

#1615240: Remove entity_label() in favor of EntityInterface::label() went in, so entity_page_label can be used in D8

Do you think we can make it for 7.15 ?

Let's fix this in D8 first with entity_page_label().

Also, tests?

Still needs tests, but this removes taxonomy_admin_vocabulary_title_callback() (it will be marked deprecated in D7).

Here's a patch with tests that reverts the commit to prove it fails, and the combined patch.

Removing tag.

Looks good

Yes this looks good. Committed/pushed to 8.x, moving to 7.x for backport.

Default value for site name :/

The test failed because the behavior of 8.x when saving a new vocabulary is different from 7.x. 8.x takes you to admin/structure/taxonomy/%vocab_machine_name. 7.x takes you back to admin/structure/taxonomy.

I added a line to the test to go to admin/structure/taxonomy/%vocab_machine_name after creating the vocabulary in order to check the page title. The change caused the test to pass here on my local machine. We'll see what Testbot thinks.

Blah, changing status. That's the 3rd time today I've forgotten.

#45: vocabulary-title-1555294-45.patch queued for re-testing.

Fix from D8

Thanks for the fix, and for the test! Committed and pushed to 7.x.

This is tagged "Needs change notification" (for Drupal 8) but doesn't look like it ever got one? Could probably use something similar to the shortcut set one at http://drupal.org/node/1762604 ...

Also, I'm tagging this for the Drupal 7.17 release notes and adding it to CHANGELOG.txt.

http://drupal.org/node/1838750

Untagging. Please remove the "Needs change notification" tag when the change notice task is complete.

AIEEEEEEE