I don't know if this is implemented in Drupal 7, but in Drupal 6 there is no HTTP-Header Content-Language which indicates the language of the current viewed page to the client. The HTTP-Header isn't really from interest to the user per se but it would help Browsers, Robots and all kind of other automated clients to determine faster which language is used on the page.
I implemented it in drupal_init_language and it's working perfectly:
/**
* Choose a language for the current page, based on site and user preferences.
*/
function drupal_init_language() {
global $language, $user;
// Ensure the language is correctly returned, even without multilanguage support.
// Useful for eg. XML/HTML 'lang' attributes.
if (variable_get('language_count', 1) == 1) {
$language = language_default();
}
else {
include_once './includes/language.inc';
$language = language_initialize();
}
// Send appropriate HTTP-Header for browsers and robots (Google).
header('Content-Language: '.$language->language);
}
I assigned it to Drupal 6.x-dev, but it would be something from interest for every new Drupal version. For a perfect i18n support things like that are important in my opinion.
Kindest regards, Richard
Comment | File | Size | Author |
---|---|---|---|
#86 | 673020-86.patch | 1.57 KB | Zoltán Balogh |
#81 | 673020-80.patch | 1.58 KB | Zoltán Balogh |
#77 | 673020-77.patch | 1.54 KB | Zoltán Balogh |
#75 | 673020-75.patch | 1.54 KB | Zoltán Balogh |
#72 | 673020-72.patch | 1.63 KB | Zoltán Balogh |
Comments
Comment #1
Gábor HojtsyI think this is a great idea, however, in Drupal 7 there are multiple languages that can be used for the page. The interface and content can be different. So they still need to figure out between each other the right language to output there. I'd argue probably the interface language is the right one to include here, and then the content items should be marked up with language codes proper. That part is discussed at #1164926: Nodes need to have languages specified separately for accessibility.
Comment #2
mgiffordGabor, this sounds like the right approach to me.
Any idea why drupal_init_language() was dropped in D7? Should it be added back in with D8?
The language info certainly doesn't seem to be sent to the browser.
Date: Wed, 15 Jun 2011 17:48:15 GMT
Server: Apache/2.2.9 (Debian) PHP/5.2.6-1+lenny10 with Suhosin-Patch mod_ssl/2.2.9 OpenSSL/0.9.8g
X-Powered-By: PHP/5.2.6-1+lenny10
Expires: Sun, 19 Nov 1978 05:00:00 GMT
Last-Modified: Wed, 15 Jun 2011 17:48:15 +0000
Cache-Control: public, max-age=0
Etag: "1308160095-0"
Link: ; rel="shortlink",; rel="canonical"
X-Generator: Drupal 7 (http://drupal.org)
Vary: Cookie,Accept-Encoding
Content-Encoding: gzip
Content-Length: 7700
Keep-Alive: timeout=5, max=100
Connection: Keep-Alive
Content-Type: text/html; charset=utf-8
200 OK
Comment #3
plachThe approach in #1 looks reasonable to me too. Can we have a patch?
@mgifford:
In d7 we have
drupal_language_initialize()
:)http://api.drupal.org/api/drupal/includes--bootstrap.inc/function/drupal...
Comment #4
mgifford@platch - How do we get it when you go to this page:
http://api.drupal.org/api/drupal/includes--bootstrap.inc/function/drupal...
There is some link that indicates that the function's name was changed in 7 and is now
http://api.drupal.org/api/drupal/includes--bootstrap.inc/function/drupal...
@Fleshgrinder - I don't know how many people actually use the HTTP-Header Content-Language - I couldn't find an example in Google, Flickr or the w3c. Who is using this? I don't doubt that it would be faster, but I don't know who is actually looking for it either. I'd like to know if there's a movement behind adopting this and what devices are using it.
That being said, here's a D8 patch.
Comment #5
Gábor HojtsyCode style is not 100%, D7+ uses a space after and before concatenation. Otherwise looks good.
Comment #6
plach@mgifford:
It should have gone into: http://drupal.org/update/modules/6/7
I added a comment on the D6 API page.
Comment #7
mgifford@Gabor - Forgot about the spacing, thanks! And to confirm, this will insert the interface language into the header and not the content's language which is the agreed best practice & consistent with what we do in the meta tags.
@plach - Thanks!
Comment #8
attiks CreditAttribution: attiks commentedsmall cosmetic change, remove empty line before closing }
Comment #9
plachRelated issue: #1222194: Rename global $language to $language_interface.
Comment #10
Gábor HojtsyCleaner title.
Comment #11
Gábor HojtsyActually look good to land IMHO. Very simple and useful.
Comment #12
webchickIs there a reason we shouldn't backport to D7 as well?
Comment #13
Gábor HojtsyI think it good for D7 too. This is information we already put on the
<html lang="" and xml:lang="">
attributes, but doing it over HTTP makes us even more flexible/compatible.Comment #14
attiks CreditAttribution: attiks commentedPatch applies cleanly to D7
Comment #15
webchickSounds good!
Committed and pushed to 8.x and 7.x. Thanks!
Comment #16
moshe weitzman CreditAttribution: moshe weitzman commentedIs there a reason this does not use drupal_add_http_header()?
Comment #17
moshe weitzman CreditAttribution: moshe weitzman commentedFurthermore, this warns for all non http clients like drush scripts and drush.sh scripts. Please wrap like so
Comment #18
Gábor HojtsyOn #16: here is a quick patch.
On #17: I looked some random samples of the 23 places where drupal_add_http_header() is called and none of the have it wrapped like this. Is this a wider systemic problem or why would it only appear here? http://api.drupal.org/api/drupal/includes--bootstrap.inc/function/drupal...
Comment #20
Gábor HojtsyShould help if I use the API proper.
Comment #22
attiks CreditAttribution: attiks commentedfixed typo
Comment #23
attiks CreditAttribution: attiks commentedPatch for moshe
Comment #24
plachI share Gabor's perplexity: if non-HTTP clients should not get HTTP headers, why warnings are not throwed elsewhere? And if they do, why don't we simply move the test inside the
drupal_add_http_header()
function?Comment #25
moshe weitzman CreditAttribution: moshe weitzman commented@gabor - the other standard header() calls only happen inside of drupal_deliver_html_page(). This call should also be moved there. That would resolve the CLI issue that I mentioned.
Comment #26
attiks CreditAttribution: attiks commentedAs explained in #25, new patch attached
Comment #27
attiks CreditAttribution: attiks commentedcleaner patch, removed spaces
Comment #28
attiks CreditAttribution: attiks commentedOne remark: the header isn't added during install, but i guess it isn't that much of a problem?
Comment #29
Gábor Hojtsy@attiks: you also need to roll back the global $language from the first hunk then.
Comment #30
attiks CreditAttribution: attiks commentedas requested
Comment #31
Gábor HojtsyLooks complete. What do you think Moshe?
Comment #32
moshe weitzman CreditAttribution: moshe weitzman commentedLooks good.
Comment #33
plachI guess we need a side issue providing some test coverage for non-HTTP clients.
Comment #34
chx CreditAttribution: chx commentedLooks like a bugfix to me and as such needs tests.
Comment #35
attiks CreditAttribution: attiks commented@chx, I don't mind adding a test, but what do you want to test, the presence of the header inside the response? Or are you talking about tests for the non-http clients?
Comment #36
Fleshgrinder CreditAttribution: Fleshgrinder commentedSorry for the extremely late reply. Wasn't around for quite a while 'cause I was busy with a lot of other projects.
I also don't know of any special use cases of the Content-Language attribute. Like an other user stated before, it's mainly to be more flexible. The W3C encourages people to use the HTTP header field or the lang attribute on the html tag. By using both Drupal is prepared for any use case. [1]
Personally I think this can come in handy when dealing with search engines and this is the reason why I started to use it. Think of the following scenario. The bot indexes a new URL and saves it to the "crawl" repository and needs to get some basic data. For sure the bot will not download the whole page, but by just doing a HEAD request the bot can get all those basic information. If there is a standard compliant Content-Language field the bot directly knows which language is used within the page.
I know that this is an invented use case, but it's possible. Personally I think that there are for sure other use cases where this thing comes in handy. For instance for content negotiation [2] related things.
Comment #37
chx CreditAttribution: chx commentedYes: create a test module with a different delivery callback and assert that Drupal added no http headers. Going to be tricky because the webserver also adds headers, right? Maybe try requesting an HTML page with drupalGet, store the name of the headers and ensure that a non-html delivery callback does not add more.
Comment #38
idflood CreditAttribution: idflood commentedThis issue breaks in some way drush (site-install command), see #1314392: drush site-install on Drupal 7.9 fails with a fatal error "Call to undefined function cache_get()"
I would love to help providing tests but I'm not comfortable with those and this one looks a bit harder to do. Any idea of similar tests which could be used as example?
Comment #39
attiks CreditAttribution: attiks commented@idflood you get the headers while using drush, as far as I can tell this should not happen, but true we need some kind of test for this.
Comment #40
moshe weitzman CreditAttribution: moshe weitzman commentedNow this bug is in the D7 Drupal release and breaking Drush. We should be reverting features when we immediately notice that they introduce a bug. Please revert this feature or commit the fix in #30. Both are safe and will not break any tests. To revert, run:
Comment #41
attiks CreditAttribution: attiks commentedPatch in #30 is applied, see f429c2716ee22754e3fcccc91ad1818b9fda3785
Edit: This is the D7 one other the D8
Comment #42
moshe weitzman CreditAttribution: moshe weitzman commented@attiks - that's a prior patch in this issue. #30 moves the header call, which is a fix to the prior commit. so once again, please apply #30 or revert as per #40
Comment #43
attiks CreditAttribution: attiks commentedclarified on irc and by actually looking at the code, please apply #30
Comment #44
webchickCrap, sorry. :( This wasn't tagged "Release blocker," nor was it critical, so I missed it in my final commits prior to 7.9.
However, I'm super confused because I didn't get that error locally when I
drush si
ed 7.9 (that's part of the standard routine I do to check a release). Does this only happen in certain versions of Drush or am I just lucky?[Edit: Yes, it occurs only on Drush 5.x, not Drush 4.x.]
Anyway, committed/pushed #30 to 8.x and 7.x. Leaving needs work for tests.
Comment #45
e2thex CreditAttribution: e2thex commentedThis is a modification of #30 for use with drush make (removing the a/ and b/'s).
Note That I do not think this pass the test because of these change so this is only for use by those of use using drush make.
Comment #46
e2thex CreditAttribution: e2thex commentedMy apologizes that patch was not converted correctly
This is a modification of #30 for use with drush make (removing the a/ and b/'s).
Note That I do not think this pass the test because of these change so this is only for use by those of use using drush make.
Comment #47
andypostJust to test #46
Comment #49
good_man CreditAttribution: good_man commentedSmall test, anything more to add?
Comment #50
Gábor HojtsyLooks generally good. Here are some comments:
Why does this need to be set?
Comment does not reflect what is happening right below it.
The message can start with 'Content-Language', that is the HTTP name of the header no?
The 'language' key changed to be 'langcode' in D8. The 'default' property should be TRUE if we want to make it the default. locale_language_save() is now language_save().
Same case fix as above.
-25 days to next Drupal core point release.
Comment #51
Gábor Hojtsy@good_man: can you help move this forward? I think I have some clear comments here. Thanks a lot!
Comment #52
Gábor HojtsyTagging for base language system
Comment #53
rvilarI'll work on this!
Comment #54
Zoltán Balogh CreditAttribution: Zoltán Balogh commentedA new version is attached.
Comment #56
Zoltán Balogh CreditAttribution: Zoltán Balogh commentedComment #58
Zoltán Balogh CreditAttribution: Zoltán Balogh commentedOne more try.
Comment #59
Gábor HojtsyLet's say "Tests Content-language headers generated by Drupal." (3rd person verb to start; also only testing the language header, so better be specific).
It is not default anymore, so let's just say "Add French language." or something along those lines.
Let's add a line of comment here like above. "Request front page in French and check for matching Content-language" or somesuch.
Comment #60
kalman.hosszu CreditAttribution: kalman.hosszu commentedPatch updated based on Gábor's comments.
Comment #61
kalman.hosszu CreditAttribution: kalman.hosszu commentedThe test run successfully, so I think we should change the status to RTBC.
Comment #62
Gábor HojtsyI agree this looks good now. We debated a bit as to whether put this in with locale or language module, but it looks fine with the general http header tests AFAIS.
Comment #63
Dries CreditAttribution: Dries commented#60: 673020-60.patch queued for re-testing.
Comment #64
Dries CreditAttribution: Dries commentedI asked for a re-test to make this is still okay now that #1416392: Clean up language (types) bootstrap function naming and documentation and #1410096: Convert comment language code schema to langcode went in.
Comment #65
Gábor HojtsyStill passes. This modifies a common test for HTTP headers so no interference with other language changes. Still RTBC :)
Comment #66
Gábor HojtsyAlso retitle to make it clear we are only doing tests here, the feature is in already.
Comment #68
Dries CreditAttribution: Dries commentedI thought it was going to be ok to commit, but the tests do pass langcodes in the URL so I wan't 100% sure. Better safe than sorry. Glad to tests still passed. Committed the tests to 8.x and marking this 'fixed'. Another D8MI patch bites the dusts. I'm happy with all the D8MI progress.
Comment #69
Gábor HojtsyAs per @webchick in #44, tests need to go to D7 too. So reopening as a backport. Might apply 1-1 to Drupal 7.
Comment #70
Zoltán Balogh CreditAttribution: Zoltán Balogh commentedPorted to 7.x.
Comment #71
Zoltán Balogh CreditAttribution: Zoltán Balogh commentedOh, it will be bad, there is not langcode in 7.x :)
Comment #72
Zoltán Balogh CreditAttribution: Zoltán Balogh commentedCorrected
Comment #74
Zoltán Balogh CreditAttribution: Zoltán Balogh commentedComment #75
Zoltán Balogh CreditAttribution: Zoltán Balogh commentedComment #77
Zoltán Balogh CreditAttribution: Zoltán Balogh commentedComment #79
Zoltán Balogh CreditAttribution: Zoltán Balogh commentedComment #80
Gábor Hojtsy@Zoltán: the patch in #75 was better, you need to pass on a 'full' language object to url(). So after locale_add_language(), clear the language_list cache and get the list of languages to have the language object. Something like:
(Not tested).
Comment #81
Zoltán Balogh CreditAttribution: Zoltán Balogh commentedIt was ok in the local environment.
Comment #82
Zoltán Balogh CreditAttribution: Zoltán Balogh commented@Gábor, locale_add_language('fr', 'French'); causes a PDOException in D7:
PDOException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'native' cannot be null:
I switched to German, to avoid the use of special characters in the native name.
Comment #83
Gábor Hojtsy@Zoltán: right, it needs more arguments in Drupal 7. Making the new language the default works too like in your last patch. Backport looks good.
Comment #84
xjmTrailing whitespace. :)
Also, the assertion messages need not be translated, but that already went in 8.x, so we shouldn't fuss with the backport.
Comment #85
Gábor HojtsyRrrright :)
Comment #86
Zoltán Balogh CreditAttribution: Zoltán Balogh commentedThanks @xjm, I corrected. 7.x in first round.
Comment #87
Gábor HojtsyLooks good again. Lacks line ending whitespace now :)
Comment #88
webchickGreat!
Committed and pushed to 7.x. Thanks!
Comment #89
Gábor HojtsyDone with it being on focus then, thanks.
Comment #90
Zoltán Balogh CreditAttribution: Zoltán Balogh commentedNo need to remove t () in 8.x from the assertion messages?
Comment #91
Gábor Hojtsy@Zoltán: if you look at modules/simpletest/tests/common.test in D8, you'll see it is chock full of t() on assertions. Unifying things like these would indeed be great, the coding standards for tests don't seem to have guidelines either way: [#325974]. Anyway, I think cleaning that up would be its own issue.
Comment #92
xjmhttp://drupal.org/simpletest-tutorial-drupal7#t
but also note that #500866: [META] remove t() from assert message is still NW, so while it's nice to fix that before patches go in, I think webchick will hurt me if I slow a patch over it. :)