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...
Comment | File | Size | Author |
---|---|---|---|
#60 | drupal.html-head-title-array.60.patch | 4.22 KB | sun |
#27 | drupal.html-head-title-array.27.patch | 2.21 KB | sun |
#22 | drupal.html-head-title-array.22.patch | 1.2 KB | sun |
#16 | drupal.html-head-title-theme.patch | 2.28 KB | jide |
#14 | drupal.html-head-title-theme.patch | 2.28 KB | jide |
Comments
Comment #1
nadavoid CreditAttribution: nadavoid commentedThis seems like a logical and helpful improvement. RTBC!
Comment #2
Dries CreditAttribution: Dries commentedI'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.
Comment #3
webchickComment #4
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis 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.
Comment #5
sun1) 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.
Comment #6
sunReally, 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.
Comment #8
nadavoid CreditAttribution: nadavoid commentedI 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.
Comment #9
nadavoid CreditAttribution: nadavoid commentedafter all that, I forgot to actualy update the status. Updating status now.
Comment #11
sunComment #12
Pasquallethere is one small problem: the original issue is not solved
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
Comment #13
jide CreditAttribution: jide commentedMaybe 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.
Comment #14
jide CreditAttribution: jide commentedHere is my try with a theme function. But maybe this has performance implications ?
Comment #15
jide CreditAttribution: jide commented.
Comment #16
jide CreditAttribution: jide commentedWhoops. This one should work.
Comment #17
sunI don't really care whether it's a theme function or not. With the previous/original patch you'd simply do:
Comment #18
webchickWe'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.
Comment #19
sunI 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.
Comment #20
sundrupal.html-head-title-array.0.patch queued for re-testing.
Comment #22
sunReally. 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.
Comment #23
sunIt seems like $[name]_array variables are not documented in @file doblocks of templates. So no additional documentation required.
Comment #24
sunAforementioned patch that lead to those sanitation of values: #461938: Core should consistently filter_xss_admin() on $site_slogan and check_plain $site_name
Comment #25
aspilicious CreditAttribution: aspilicious commentedI think this is RTBC.
Comment #26
sunI guess you forgot to change the status ;)
Comment #27
sunerr, just recalled my follow-up on the other issue - missed the maintenance page variables here.
Comment #28
aspilicious CreditAttribution: aspilicious commentedI need to learn being slowly on putting to RTBC, so I leave it to others with more knowledge ;).
Comment #29
markabur CreditAttribution: markabur commentedThis 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.
Comment #30
meba CreditAttribution: meba commentedLooks good to me.
Comment #31
JacineMany 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.
Comment #32
Dries CreditAttribution: Dries commented#27: drupal.html-head-title-array.27.patch queued for re-testing.
Comment #33
aspilicious CreditAttribution: aspilicious commented#27: drupal.html-head-title-array.27.patch queued for re-testing.
Comment #34
aspilicious CreditAttribution: aspilicious commented*Retesting old patches*
Comment #36
sun#27: drupal.html-head-title-array.27.patch queued for re-testing.
Comment #37
sunComment #38
sun#27: drupal.html-head-title-array.27.patch queued for re-testing.
Comment #39
JacinePatch still applies. Seems like a no-brainer so RTBC +1.
Comment #40
Frando CreditAttribution: Frando commentedYeah, 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.
Comment #41
sun#27: drupal.html-head-title-array.27.patch queued for re-testing.
Comment #43
sun#27: drupal.html-head-title-array.27.patch queued for re-testing.
Comment #44
sunTestbot hiccup, back to RTBC.
Comment #45
bleen CreditAttribution: bleen commentedjust played with this for a couple minutes ... looks excellent.
RTBC++
Comment #46
Jeff Burnz CreditAttribution: Jeff Burnz commentedSeems link a nice incremental improvement +1
Comment #47
sun#27: drupal.html-head-title-array.27.patch queued for re-testing.
Comment #48
sun#27: drupal.html-head-title-array.27.patch queued for re-testing.
Comment #49
sunComment #50
Dave ReidJust 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.
Comment #51
sunAlthough badly needed, this sounds like D8 material to me.
Comment #52
sunSo it seems that bugs are still considered to be fixed for D7.
Comment #53
sun#27: drupal.html-head-title-array.27.patch queued for re-testing.
Comment #54
sun#27: drupal.html-head-title-array.27.patch queued for re-testing.
Comment #55
sun#27: drupal.html-head-title-array.27.patch queued for re-testing.
Comment #56
sun#27: drupal.html-head-title-array.27.patch queued for re-testing.
Comment #57
sunComment #58
sun#27: drupal.html-head-title-array.27.patch queued for re-testing.
Comment #59
webchickI 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.
Comment #60
sunSure.
Comment #61
JacineLooks good to me. :)
Comment #62
webchickThat "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!