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>';
      
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Title: Documentation problem with dashboard_permission » dashboard_permission() is violating our standards for t()
Component: documentation » dashboard.module

Good 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.

jhodgdon’s picture

Priority: Normal » Major
Issue tags: +String freeze

This is also a string fix so it needs to be done before release!!!!

Dave Reid’s picture

Needs to be fixed.

jhodgdon’s picture

Title: dashboard_permission() is violating our standards for t() » dashboard_permission() is violating our standards for t() [but for a good reason]
Component: dashboard.module » documentation
Priority: Major » Normal
Issue tags: -String freeze

benjamin_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.

sun’s picture

jhodgdon’s picture

Title: dashboard_permission() is violating our standards for t() [but for a good reason] » Standards for t() are missing a use case that dashboard_permission() uses
Assigned: Unassigned » jhodgdon

Re-title issue so it's clear we are fixing up t() doc. Which is somewhat of a mess. I'm taking a first stab.

jhodgdon’s picture

Status: Active » Needs review
FileSize
15.82 KB

That 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().

apaderno’s picture

There's a pretty good case in this context that 'Administer blocks' should be translated independently and the same regardless of context.

The 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 to t(). contact page could be a string that has been passed separately to t().

    $output .= '<p>' . t('Go to the <a href="@contact-page">contact page</a>.', array('@contact-page' => url('contact'))) . '</p>';
 

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:

  drupal_set_message(t('Database port') . t(' must be a number.'), 'warning');

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.

jhodgdon’s picture

I 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...

jhodgdon’s picture

I 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.

apaderno’s picture

I 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.

That 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.

jhodgdon’s picture

Who 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:

'Customizing the dashboard requires the <a href="!permission-url">Administer blocks</a> permission.'

instead of:

'Customizing the dashboard requires the !permission-name permission.'

(where currently !permission_name is doing l() to return the whole A element).

apaderno’s picture

Following what reported in the documentation, the string should be

'Customizing the dashboard requires the <a href="@permission-url">Administer blocks</a> permission.'

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.

tstoeckler’s picture

What 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:

  // Here we translate 'Database port' seperately so that it is consistent with [bar].
  drupal_set_message(t('@database_port must be a number.', array('@database_port => t('Database port')), 'warning');
jhodgdon’s picture

Actually, #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.

tstoeckler’s picture

Well #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.

sun’s picture

in some languages, the word order in the two sentences is different, or the word used for the (requires the) is different.

Regardless 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

Go to the contact page.

and

The %contact page is where you should go.
jhodgdon’s picture

I 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).

Cinque’s picture

I agree with kiamlaluno and jhodgdon. Translations and articles can and do change, depending on the language and context.

sun’s picture

even proper nouns can change

And 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

In order to <a href="@blocks-url">administer blocks</a>, ensure that...

then this clearly does not reference the proper noun and unique permission name "Administer blocks".

However, whenever we do

You need the <a href="@permission-url">%permission-name</a> permission to configure blocks.
Click the %label button to continue.
...

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.

jhodgdon’s picture

Should 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.

sun’s picture

Normally, we put such terms in %placeholders, without any surrounding markers/punctuation.

jhodgdon’s picture

I 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.

sun’s picture

Well, 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 not the 'Label' thing or any other possible variation).

apaderno’s picture

@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.

jhodgdon’s picture

OK, 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:

+ * One exception to the rule of using an embedded link title: if the link title
+ * text is translated elsewhere as a separate string, it may be desirable to use
+ * a placeholder for the link text. For example, if you are linking to a
+ * specific permission on the Permissions page, you would use a placeholder for
+ * the permission name, so that that permission name gets translated in exactly
+ * the same way. Example:
+ * @code
+ * $description = t('Customizing the dashboard requires the <a href="@permission-url">!permission-name</a> permission.',
+ *   array(
+ *     '!permission-name' => t('Administer blocks'),
+ *     'permission-url' => url('admin/people/permissions', array('fragment' => 'module-block')),
+     ));
+ * @endcode

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...

jhodgdon’s picture

re: #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?

jhodgdon’s picture

Issue tags: +i18n, +internationalization

Adding tags in case the i18n people haven't seen this... Should we ping someone?

apaderno’s picture

Issue tags: -i18n, -internationalization
+ * $message[] = t(
+ *   "If you don't want to receive such e-mails, you can change your settings at !url.", array('!url' => url("user/$account->uid", array('absolute' => TRUE))));

The used placeholder is different from the one used in another example, which is reported to be correct.

+ * Correct HTML in t():
+ * @code
+ * $output .= '<p>' . t('Go to the <a href="@contact-page">contact page</a>.', array('@contact-page' => url('contact'))) . '</p>';
+ * @endcode

For the rest, the patch seems fine to me.

apaderno’s picture

Issue tags: +i18n, +internationalization

I apologize. Removing the tags was not intentional.

jhodgdon’s picture

I 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?

sun’s picture

E-mail bodies are considered to be HTML in D7. Therefore, usage of the @url placeholder like in any other HTML context should be correct.

jhodgdon’s picture

OK, 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:

+ * The !variable placeholder indicates that the text should be inserted as-is.
+ * This is useful for inserting variables into things like e-mail. Example:

So when should we say !variable is useful?

sun’s picture

The !placeholder is useful, when the injected string comes from another source and is already escaped. A bad example would be l(). A better example is

    form_error($element, t('!name field is required.', array('!name' => $element['#title'])));
jhodgdon’s picture

FileSize
15.02 KB

OK. Here's a new patch. Removes bad recommendation as discussed at length above, and puts in a new example/explanation for the ! placeholder.

apaderno’s picture

It seems I am late. :-)

I was going to suggest the following example:

  global $user;

  $name = t('User !uid', array('!uid' => $user->uid));
Dave Reid’s picture

Let's use a good example. :)

t('The user last logged in !ago.', array('!ago' => format_interval($account->login));
Gábor Hojtsy’s picture

My 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.

jhodgdon’s picture

Status: Needs review » Needs work

#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.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
11.68 KB

It 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.

jhodgdon’s picture

#40: 997884-40-gut-t-doc.patch queued for re-testing.

jhodgdon’s picture

I 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.

Gábor Hojtsy’s picture

Looks 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.

jhodgdon’s picture

FileSize
11.88 KB

Good thought. Here's a new patch, with stronger-worded language about not doing t($variable).

jhodgdon’s picture

Title: Standards for t() are missing a use case that dashboard_permission() uses » t() documentation overhaul

better title...

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Nice, it all looks good to me know.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)
Issue tags: -i18n, -internationalization

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