I just stumbled over this in template_preprocess_html() and recalled my countless WTF moments I had when I just wanted to replace the pipe (|) with a slash on my sites.

To be able to do that, you have to rebuild all the parts on your own, check for a title and sanitize it, get the site's name, get the site's slogan, ...

...when all you really only want is to replace the delimiter, or perhaps change the order...

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nadavoid’s picture

Status: Needs review » Reviewed & tested by the community

This seems like a logical and helpful improvement. RTBC!

Dries’s picture

I'd actually prefer to move the pipe to the actual template files. That is, we'd have to pass in two variables instead of an array.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Damien Tournoud’s picture

This is closely related to #449142-1: SA-CORE-2009-005 and SA-CORE-2009-006 - Drupal core - Cross site scripting. I would prefer introducing a proper theme function for the page title.

sun’s picture

Status: Needs work » Needs review

1) We cannot move this into the template file, because it contains of 3 string parts, from which 2 are dynamically combined depending on the current page context. Hence, the only way to override the resulting string is to implement a mytheme_preprocess_html() function.

2) The concatenated values in this array form the innards of the TITLE tag. Not anything before or after or around it. I fail to see how this simple change could be related to #449142: SA-CORE-2009-005 and SA-CORE-2009-006 - Drupal core - Cross site scripting.

3) I also fail to see how a theme function would be different this patch aside from an additional, forced function call on every site.

Please bear in mind that themers can already do this, but as of now, they need to know the internal variable names of the system or do any other weird code to change this.

sun’s picture

Really, I think my tiny little innocent patch got derailed with unrelated stuff.

I even made sure that this actually makes sense for themers (nadavoid is a themer) and actually makes things a little bit easier.

nadavoid’s picture

I agree with sun. The original intent of the patch was to simplify things, while maintaining existing functionality. It does not make sense to me to separate a single variable in html.tpl.php into 2 or more, with a pipe separator manually placed between them. That does not seem consistent since other occurrences of variables in html.tpl.php have a singular purpose ($styles, $page, $rdf_namespaces, etc.) and each seems pretty self sufficient.

In the end, it seems to me that the request to separate out the $page_title variable into separate variables in html.tpl.php would belong in a separate request.

Marking original patch RTBC because having the addition of an array for themers to have available without affecting existing functionality seems like a really good thing.

nadavoid’s picture

Status: Needs review » Reviewed & tested by the community

after all that, I forgot to actualy update the status. Updating status now.

Status: Reviewed & tested by the community » Needs review

Re-test of drupal.html-head-title-array.0.patch from comment @comment was requested by webchick.

sun’s picture

Status: Needs review » Reviewed & tested by the community
Pasqualle’s picture

Status: Reviewed & tested by the community » Needs work

there is one small problem: the original issue is not solved

just wanted to replace the pipe (|) with a slash

so after this patch try to create the code in the template which would make the title with '/' (for all the title options)

and the new variable needs to be documented in page.tpl.php

jide’s picture

Status: Needs review » Needs work

Maybe silly, but could not we use a theme function for this ?
It would let themers do whatever they want with the page title.

Edit : Yeah, sorry, this was already considered.

jide’s picture

Here is my try with a theme function. But maybe this has performance implications ?

jide’s picture

Status: Needs work » Needs review

.

jide’s picture

Status: Needs work » Needs review
FileSize
2.28 KB

Whoops. This one should work.

sun’s picture

I don't really care whether it's a theme function or not. With the previous/original patch you'd simply do:

function mytheme_preprocess_html(&$variables) {
  $variables['head_title'] = implode(' / ', $variables['head_title_array']);
}
webchick’s picture

Version: 7.x-dev » 8.x-dev

We're now well past API freeze, even by the original patch's date. The current situation doesn't represent a regression. Let's figure out the optimal solution for this in D8.

sun’s picture

Version: 8.x-dev » 7.x-dev
Component: theme system » markup

I have troubles to follow that reasoning. This patch does nothing else than to add a very helpful template variable. No API change involved and no existing template variables are changed. Changing APIs never was the intention of this issue. Such revamping solutions can be discussed in separate issues for D8.

sun’s picture

Status: Needs review » Needs work

The last submitted patch, drupal.html-head-title-theme.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
Issue tags: +Security improvements
FileSize
1.2 KB

Really. All I want is those chunks that make up the $head_title string in a separate array.

In the meantime, it seems like some other security patch has been applied. Now look at how those chunks are built! Do you really expect that anyone will get this right when having to entirely redo this manually?

$[name]_array is the way we provide non-flattened template variables for themers in D7. I fail to see why we can't simply provide the HTML head title as array, too.

sun’s picture

It seems like $[name]_array variables are not documented in @file doblocks of templates. So no additional documentation required.

sun’s picture

aspilicious’s picture

I think this is RTBC.

sun’s picture

Status: Needs review » Reviewed & tested by the community

I guess you forgot to change the status ;)

sun’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.21 KB

err, just recalled my follow-up on the other issue - missed the maintenance page variables here.

aspilicious’s picture

I need to learn being slowly on putting to RTBC, so I leave it to others with more knowledge ;).

markabur’s picture

Status: Needs review » Reviewed & tested by the community

This looks good. The head_title site_name on the maintenance page ought to get the check_plain treatment, but that is taken care of in #461938.

meba’s picture

Looks good to me.

Jacine’s picture

To be able to do that, you have to rebuild all the parts on your own, check for a title and sanitize it, get the site's name, get the site's slogan, ...

Many will not know they need to sanitize this, so this patch is a good improvement IMO. I remember struggling trying to figure out how to change this back in the newbie days. I think it should go in.

Dries’s picture

Issue tags: -Security improvements
aspilicious’s picture

Issue tags: +Security improvements
aspilicious’s picture

*Retesting old patches*

Status: Reviewed & tested by the community » Needs work
Issue tags: -Security improvements

The last submitted patch, drupal.html-head-title-array.27.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
Issue tags: +Security improvements
sun’s picture

Status: Needs review » Reviewed & tested by the community
sun’s picture

Jacine’s picture

Patch still applies. Seems like a no-brainer so RTBC +1.

Frando’s picture

Yeah, stumbled over that a few times as well. The seperate head_title_array variable is very helpful and makes it much easier for themers (less copy/paste needed). RTBC+1.

sun’s picture

Issue tags: -Security improvements

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal.html-head-title-array.27.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
Issue tags: +Security improvements
sun’s picture

Status: Needs review » Reviewed & tested by the community

Testbot hiccup, back to RTBC.

bleen’s picture

just played with this for a couple minutes ... looks excellent.

RTBC++

Jeff Burnz’s picture

Seems link a nice incremental improvement +1

sun’s picture

Issue tags: -Security improvements
sun’s picture

Issue tags: +Security improvements
sun’s picture

Status: Reviewed & tested by the community » Closed (won't fix)
Dave Reid’s picture

Assigned: sun » Unassigned
Status: Closed (won't fix) » Reviewed & tested by the community

Just because someone is upset doesn't mean we have to mark something as won't fix. Someone else is more than welcome to pick up work on a ticket.

sun’s picture

Version: 7.x-dev » 8.x-dev

Although badly needed, this sounds like D8 material to me.

sun’s picture

Version: 8.x-dev » 7.x-dev
Assigned: Unassigned » sun

So it seems that bugs are still considered to be fixed for D7.

sun’s picture

Issue tags: -Security improvements
sun’s picture

sun’s picture

sun’s picture

Issue tags: +Security improvements
sun’s picture

Title: HTML HEAD title cannot be output differently » HTML HEAD title cannot be safely output differently
Issue tags: +API addition
sun’s picture

webchick’s picture

Status: Reviewed & tested by the community » Needs work

I moved this to D8 once already, and still believe it's too late (the last appropriate time to add new theme variables like this was December 2009).

However, only because there's some very tricky strip_tags/check_plain() stuff that needs to happen to make the individual parts of this variable safe to use, I think we can squeeze it in before RC1, if it's ready.

But if you add a variable, you need to document it. In both page.tpl.php and maintenance page.tpl.php.

sun’s picture

Status: Needs work » Needs review
FileSize
4.22 KB

Sure.

Jacine’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me. :)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

That "maximally" thing was a bit weird, so changed to:


The key/value pairs may contain one or more of the
* following, depending on conditions:

...because that's both a bit more clear English-wise, and also more future proof in case we ever decide to derive the title from more than just those values.

(I also wrapped the line above this at 80 characters because it was annoying me. :P)

Committed to HEAD. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -Security improvements, -API addition

Automatically closed -- issue fixed for 2 weeks with no activity.