Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Following some conversation the security list it seems clear that this function is confusing. It also includes a comment that doesn't follow standard.
I've tried to fix both but admit that an explanation of what is going on could be improved.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | 427648_clarify_drupal_get_title_filtering.patch | 912 bytes | greggles |
| better_comments_for_drupal_get_title.patch | 1006 bytes | greggles |
Comments
Comment #1
Crell commentedIt's not just the comments. node_page_view calls drupal_set_title() as well, which is therefore redundant with the title callback and confusing. That's what originally made me think there was a security hole there.
Comment #2
gregglesI consider them to be separate issues. #428144: remove duplicated drupal_set_title.
Comment #3
mcrittenden commentedI'd stick a comma after "convention", but other than that it looks good. Definitely makes more sense with the extra comment, follows commenting standards, etc.
Changing to "needs work" for the comma...feel free to change back if you think it's unnecessary. I'm was no English major :)
Comment #4
caktux commentedI can do without the comma ;)
Comment #5
webchickHm. While I agree that the docs here are an improvement, most people are not going to be looking at the innards of a function to know how to use it; they're going to be looking at the PHPDoc, which comes up in their IDE, on api.drupal.org, etc.
Could we possibly expand this single-line comment into more of sort of a "do / do not" list or whatever when using this function, up in the PHPDoc where people will see it and we have more room?
Comment #6
Tor Arne Thune commentedStill a valid issue.
Comment #7
gregglesI think the confusion was about why the function needed the check_plain at all, so having it inline makes sense to me. That said, I agree this could be more explicit that the return is already sanitized for the browser context.
Comment #21
quietone commenteddrupal_get_title was removed from core in #2209595: Remove drupal_set_title(), drupal_get_title() and associated tests in 2014. The documentation on Drupal\Core\Controller\TitleResolverInterface::getTitle has been updated a few times so I reckon this is now outdated.