Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
dashboard_permission()
is using the following code
return array(
'access dashboard' => array(
'title' => t('View the administrative dashboard'),
'description' => t('Customizing the dashboard requires the !permission-name permission.', array(
'!permission-name' => l(t('Administer blocks'), 'admin/people/permissions', array('fragment' => 'module-block')),
)),
),
);
which is contrary to what reported in the documentation for t().
- Incorrect HTML in t():
$output .= t('<p>Go to the @contact-page.</p>', array('@contact-page' => l(t('contact page'), 'contact')));
- Correct HTML in t():
$output .= '<p>' . t('Go to the <a href="@contact-page">contact page</a>.', array('@contact-page' => url('contact'))) . '</p>';
Comment | File | Size | Author |
---|---|---|---|
#44 | 997884-44.patch | 11.88 KB | jhodgdon |
#40 | 997884-40-gut-t-doc.patch | 11.68 KB | jhodgdon |
#35 | 997884-35-fix-t-doc.patch | 15.02 KB | jhodgdon |
#7 | 997884-7-t-cleanup.patch | 15.82 KB | jhodgdon |
Comments
Comment #1
jhodgdonGood point. So either we should fix the example in t(), or we should fix the way the Dashboard module is doing things. I'm inclined to say Dashboard should be fixed.
Comment #2
jhodgdonThis is also a string fix so it needs to be done before release!!!!
Comment #3
Dave ReidNeeds to be fixed.
Comment #4
jhodgdonbenjamin_agaric just pointed this out in IRC:
"The reason we're supposed to use tags in t() is so that the linked text is in context. There's a pretty good case in this context that 'Administer blocks' should be translated independently and the same regardless of context"
"Maybe a code comment saying we want it to be the same string as http://api.drupal.org/api/drupal/modules--block--block.module/function/block_permission/7 "
So I think we should do that -- put in a code comment, and also mention this case in the t() documentation, rather than changing the translatable string in this case.
Comment #5
sunsubscribing
Comment #6
jhodgdonRe-title issue so it's clear we are fixing up t() doc. Which is somewhat of a mess. I'm taking a first stab.
Comment #7
jhodgdonThat t() page is a mess. We're about to have @section tags in the API module, and this seems to be a good place for them. #996242: Support @ref, @section and @subsection tags
So, here's a preliminary patch, which rewrites and reformats the t() documentation, and adds a code comment to dashboard_permission().
Comment #8
apadernoThe same could be said for the example reported in
t()
document and, in general, in all the cases where a link is used in a string passed tot()
. contact page could be a string that has been passed separately tot()
.Considering strings like Database port and Database port must be a number., to be sure that the first string is translated in the same way in the second sentence, I should wrote code like the following:
I think that the reason to use a string like
'Go to the <a href="@contact-page">contact page</a>.'
is to give to the translator users the context to translate the sentence. For example, in German there is more than one definite article; to understand which one to use, I would need to see the word that follow the article, and in a string like'Customizing the dashboard requires the !permission-name permission.'
I cannot understand which article to use. To make the task harder, the used placeholder doesn't suggest which word follow the article.Comment #9
jhodgdonI disagree with both of the examples in #8.
contact page example: That particular example is using "contact page" as a lower-case string that is prose inside a sentence as well as being a link. I don't think we want to separate it out from the sentence.
database port example: DEFINITELY not. We want to keep the whole sentence together. Besides which it is missing a space, and we can't assume that you use an ASCII space character to separate words in all languages (doesn't apply to Chinese, for example, as far as I know).
But this comment is right: generally we want to give translators the context, so we want to pass in the whole sentence.
The one exception to that is that the permission for "Administer blocks" is a specific permission name on a specific page, and we want to make sure it is translated exactly the same so that the user will be able to look for that exact permission name on the Permissions page.
So... I think the examples in the current patch are correct...
Comment #10
jhodgdonI do see your point about multiple defininite articles, but just as a note, the noun here is "permission" and the permission name is really a modifier on this noun, so you should use the definite article appropriate for the word "permission", independent of what the permission is.
Comment #11
apadernoThat is true for English grammar, but there could be languages where the grammar roles for choosing the definite article are different, or there could be languages where the definite article is replaced by suffix that is added to the word following the article.
Comment #12
jhodgdonWho knows -- there are a lot of languages out there, but we still would want the permission name to match what is shown on the permission page, wouldn't we? That was the whole point here.
So kiamlaluno: are you advocating that we should go change the dashboard_permission() text, and remove this exception from the t() examples completely?
The change in dashboard would be to make it:
instead of:
(where currently !permission_name is doing l() to return the whole A element).
Comment #13
apadernoFollowing what reported in the documentation, the string should be
I made a test, and I tried translating two sentences like Customizing the dashboard requires the Create new books permission. and Customizing the dashboard requires the Administer blocks permission.; in some languages, the word order in the two sentences is different, or the word used for the (requires the) is different.
Comment #14
tstoecklerWhat happened to "[foo] should be translated consistently regardless of context"?
The first example in #8 is fine as it is (no double t()) and the second should be:
Comment #15
jhodgdonActually, #14 is still not OK. Some languages would need to have different noun forms depending on where "database port" is used in the sentence (subject, object of verb, object of preposition, etc.).
So I'm inclined to agree with #13 - that we should recommend all the words be put in there, use substitution for the URLs and such things that don't want to be translated, and just hope the translators translate them correctly.
Comment #16
tstoecklerWell #15 is only true if you don't necessarily need the same translations, in which case the double t() is worthless anyway. Perhaps the problem is simply that the example is not really a good one, whereas the one with the permission name is. Maybe we should just document the pros and cons of both methods. It seems there isn't really a golden rule.
What we do with the string in dashboard module, I don't really care.
Comment #17
sunRegardless of language, this should not affect the unique/fixed string that is injected. The premise for extracting a term/phrase into an own t() string is that the term/phrase is used like a proper noun within the sentence.
Therefore, we indeed have a difference between
and
Comment #18
jhodgdonI think kiamlaluno is correct -- even proper nouns can change, and the form of things like "the" is definitely dependent on what is around it. So I think we do want to go back to recommending including the whole sentence/paragraph no matter what, and live with the fact that translators could be inconsistent (although in my limited experience with the Spanish translation team, they do try to maintain wording consistency as much as possible, via having a glossary page and having people review the translations).
Comment #19
Cinque CreditAttribution: Cinque commentedI agree with kiamlaluno and jhodgdon. Translations and articles can and do change, depending on the language and context.
Comment #20
sunAnd that's actually one of the main reasons for extracting a proper noun into a string token.
If we happen to decide to have to rename the "Administer blocks" permission label in D7.1 or D8 or whatnot, why should translators have to redo all of the affected translations? There's no reason for that. In this usage, it's a proper noun, and should be treated like that.
I've done and ensured similar tweaks to many other strings in D7 that contain comparable references to strings that users expect to see elsewhere. For me, that's the primary and counter-argument-killing reason for extracting such strings into tokens. Whenever we're referencing to a definite and unique label/string that appears elsewhere, it should be extracted into a t() token.
Again, the situation is different when not referencing to a definite label/string. For example, if we would mention
then this clearly does not reference the proper noun and unique permission name "Administer blocks".
However, whenever we do
then we are referencing to a certain thing/label/string in the system, and thus, we should do our best to ensure that users will actually see what we are pointing to.
Comment #21
jhodgdonShould it then be enclosed in quotes? I guess making it a link is some kind of "enclosure", but since we're using Sentence case for our Proper nouns in Drupal, as you can see from this sentence (which I think was a Stupid idea), there's not a clear demarkation between the Proper noun and the rest of the sentence.
Comment #22
sunNormally, we put such terms in %placeholders, without any surrounding markers/punctuation.
Comment #23
jhodgdonI realize that, but I'm not sure it's a good idea from a writing style perspective, if they are to be considered proper nouns.
Comment #24
sunWell, it's what we do for a long time and almost everywhere in D7. Changing that style would be a D8 topic (and t() or drupal_placeholder() itself would ideally have to be changed to ensure that we have a consistent output; i.e., always
the "Label" thing
and notthe 'Label' thing
or any other possible variation).Comment #25
apaderno@sun: Strings like
'The @object-name is blu.'
create problems because in Swedish (but it is also true for Danish, and Norwegian) phrases like the apple is blu, the sky is blu, and the carrot is blu are translated in äpplet är blu, himlen är blu, and moroten är blu. (The words for apple, sky, and carrot, taken alone are respectively äpple, himmel, and morot.)In Greek, phrases like customizing the dashboard requires the Administer blocks permission and customizing the dashboard requires the Create new book pages permission are translated with προσαρμογή του ταμπλό απαιτεί την άδεια διαχείριση μπλοκ and προσαρμογή του ταμπλό απαιτεί τη Δημιουργία νέας άδειας βιβλίο σελίδες (I hope the Greek characters are visible). A string like
'Customizing the dashboard requires the !permission-name permission.'
creates problem to translators, as in Greek the definite article (the one after Customizing the dashboard requires) could be την, or τη depending on the word that follows it.Comment #26
jhodgdonOK, agreed.
So... Back to the patch at hand.
It seems to me that the pattern used in dashboard_permissions, where the entire link rather than the link title is put into a placeholder, is not entirely recommended, but I don't think it's bad enough that we want to try to put in a string change at this point.
It also seems to me that the patch in #7 (which adds a code comment to dashboard_permissions() and updates the doc for t() considerably) still needs review. I guess the only questionable part is this recommendation:
I still think it's OK to put this in as a recommendation, in spite of all the discussion above, but we can remove it if it's too objectionable. As noted by sun, it does formalize a pattern used all over D7 core. And maybe we should file a D8 bug to revisit this pattern, but it's definitely too late to revise it all over D7 core.
A review of the rest of the t() doc patch is also requested...
Comment #27
jhodgdonre: #25 -- Yeah, actually we have the same thing with the indefinite article in English -- a vs. an depending on the following word (not depending on the actual noun it is modifying). So I see kiamlaluno's point here. Finally.
So we should probably remove that recommendation cited in #26 from t(). Can someone review the rest of the patch, as I'm sure there are some other issues (typos, etc.) and I can then roll a new patch without that chunk and fixing the other issues...
And I'm actually surprised we haven't heard complaints from the translation teams about these issues, since this pattern is supposedly all over D7. Do we need to revisit changing the pattern?
Comment #28
jhodgdonAdding tags in case the i18n people haven't seen this... Should we ping someone?
Comment #29
apadernoThe used placeholder is different from the one used in another example, which is reported to be correct.
For the rest, the patch seems fine to me.
Comment #30
apadernoI apologize. Removing the tags was not intentional.
Comment #31
jhodgdonI don't understand #29. The first example is for an email message that is sent out, so we want to insert the bare URL and make sure it is not changed. The second is for output on a page, and is going into an HTML tag attribute, so it needs to be check_plain() just in case. I think?
Comment #32
sunE-mail bodies are considered to be HTML in D7. Therefore, usage of the @url placeholder like in any other HTML context should be correct.
Comment #33
jhodgdonOK, then we need a different example for when to use the ! placeholder (that was the example for the !placeholder section in the t() doc). Any ideas? That section also says:
So when should we say !variable is useful?
Comment #34
sunThe
!placeholder
is useful, when the injected string comes from another source and is already escaped. A bad example would be l(). A better example isComment #35
jhodgdonOK. Here's a new patch. Removes bad recommendation as discussed at length above, and puts in a new example/explanation for the ! placeholder.
Comment #36
apadernoIt seems I am late. :-)
I was going to suggest the following example:
Comment #37
Dave ReidLet's use a good example. :)
Comment #38
Gábor HojtsyMy overall thinking is that we are trying to fit a whole documentation *section* not even just a page into the t() docs. There are all kinds of things that we could document still. I'm not happy that it says you should always call t() but then has no mention of format_plural(), format_size(), etc. which do translation themselves, or exceptions like hook_menu(). Also, the text has a section on "providing context" talking about long paragraphs and keeping sentences intact, but no mention of the D7 'context' feature, which is the only way to provide context for shorter strings (core has two special contexts now). All-in-all I think this tries to be short version of http://drupal.org/node/322729 and instead would ideally be shorter and point to more information on drupal.org (which should in fact be more living and actively maintained). The new database API is not documented in code documentation either.
Comment #39
jhodgdon#38: That is a very good point.
I will tackle getting some of the hints and stuff out of here and into the Handbook, though probably not today.... We do need to document how to use the !, @, and % variable placeholders in the t() function, but possibly not at such length.
Comment #40
jhodgdonIt looks to me as though the Localization Guide http://drupal.org/node/322729 has all the information we want it to have (although it could use a little polishing here and there, I think it's fairly good).
So I've made a new patch referring to this rather than trying to repeat it. It pretty much gets t() down to a reasonable amount of doc, which I think is a Good Thing. The page is pretty much unreadable as it is now.
Comment #41
jhodgdon#40: 997884-40-gut-t-doc.patch queued for re-testing.
Comment #42
jhodgdonI just filed this issue against D8 to address the problem illustrated in #25 and the above discussion:
#1021852: Variable substitution in UI strings - only where absolutely necessary
We still could use a review of the patch in #40.
Comment #43
Gábor HojtsyLooks generally good. I think it would be a good idea to keep mentioning that t($user_data) is a bad idea in terms of security as well. The removed comments had this portion: ... input strings of uncertain origin. which might or might not make it clear. Because t('@stuff') usually does escaping, some people transition this assumption to t($stuff), which is however, obviously not true. So maybe making this clear would be a good idea. Otherwise looks great.
Comment #44
jhodgdonGood thought. Here's a new patch, with stronger-worded language about not doing t($variable).
Comment #45
jhodgdonbetter title...
Comment #46
Gábor HojtsyNice, it all looks good to me know.
Comment #47
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.