Problem/Motivation

t() and TranslationManager still point to SafeMarkup::format()

and

best practices have changed

Proposed resolution

they should point to FormattableMarkup

and the docs should be updated

Remaining tasks

User interface changes

No.

API changes

No.

Data model changes

No.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

xjm’s picture

Status: Postponed » Active

Zoom!

YesCT’s picture

I'm going to work on this now.

jhodgdon’s picture

Component: base system » documentation

docs component perhaps? :)

YesCT’s picture

just a start. wip.

YesCT’s picture

Title: Point t() and TranslationManager::translate() docs to FormattableString » Point t() and TranslationManager::translate() docs to FormattableMarkup

retitling since #2576533: Rename SafeStringInterface to MarkupInterface and move related classes renamed FormattableString to FormattableMarkup

YesCT’s picture

Status: Active » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

This may not be finalized, but I had one thought:

+++ b/core/includes/bootstrap.inc
@@ -279,25 +279,23 @@ function drupal_get_path($type, $name) {
- * You should never use t() to translate variables, such as calling t($text)
- * unless the text that the variable holds has been passed through t()
- * elsewhere (e.g., $text is one of several translated literal strings in an
...
+ * You should never use t() to translate variables, such as calling t($text).
+ * Doing that can lead to cross-site scripting vulnerabilities and other
+ * security problems.
+ *

So... We lost some information here that is important.

There are two reasons not to call t() on a $text variable:

1) User-entered text - security problem. That you have in the new version.

2) A string that you composed entirely of other stuff and that doesn't have any security problems. This is still a problem because it doesn't get into the translation database.

(2) got lost. Can we retain that information somehow? Because someone might read the new text here and think "yeah but this isn't a security problem because every piece of this concatenated string is safe" and do it anyway.

The rest of the patch looks good to me. I don't know if it satisfies the scope of the issue yet. Summary mentions a few places but I think not all are fixed? Not sure.

YesCT’s picture

Issue tags: +D8MI
Gábor Hojtsy’s picture

Issue tags: +sprint, +language-ui

The patch looks good to me. I agree with @jhodgdon on the subtlety lost, that would be good to get back though.

YesCT’s picture

I guess I dont see why it is ok to:

call t($text) when "the text that the variable holds has been passed through t() elsewhere (e.g., $text is one of several translated literal strings in an"

if t has already been called on the parts?

Maybe an example would be helpful?

Gábor Hojtsy’s picture

That was meant to convey:

$values = [t('Home'), t('Operations')];
$operations = 'Operations';
print t($operations);

NOT doing double t(). But that *elsewhere* it was already registered as a source string. Otherwise if people would use only the last two lines, potx AKA localize.drupal.org would never find those strings. That is what that text was supposed to cover. There is no way to statically figure out 'Operations' as a translatable string, if only the last two lines of that snippet happen.

YesCT’s picture

if only

$operations = 'Operations';
print t($operations);

then, Operations might not be in the .po file to be translated?

------------

Cause the docs sounded like me like they were saying it would be ok to do

print t($home $operations);

... and I guess it is ok? cause translations might need to reorder them.

YesCT’s picture

Status: Needs work » Needs review
FileSize
1.42 KB
3.23 KB

ok. how is this?

Gábor Hojtsy’s picture

Cause the docs sounded like me like they were saying it would be ok to do

print t($home $operations);

... and I guess it is ok? cause translations might need to reorder them.

That is *definitely* what the docs intended to suggest. It intends to suggest that t($home . ' ' . $operations) is only ok if there was also a t('Home Operations') assuming $home = 'Home' and $operations = 'Operations'; Otherwise potx will never be able to identify the source string, so your stuff will not be translatable on l.d.o.

YesCT’s picture

or... maybe that hunk is not needed, if the exception is still true.

so here is one without that.

I suppose we can have another issue to clarify that exception paragraph and add examples. this was just suppose to just make sure t() pointed to the correct place.

YesCT’s picture

wait... I thought print t($home $operations); was not ok, and should be done with placeholders....

Gábor Hojtsy’s picture

@YesCT: if there is a t('Home Operations') string somewhere too, then why not? I mean not this specific example, its a silly example, its just for the sake of discussing it...

YesCT’s picture

@Gábor Hojtsy :)

---------

oh, missed one in the t @param.

YesCT’s picture

Status: Needs review » Needs work

and... is gonna "needs work" for the and part of the title of the issue: "Point t() and TranslationManager::translate() docs to FormattableMarkup"

and since Translation Manager is:
class TranslationManager implements TranslationInterface, TranslatorInterface {

means changes to:

\Drupal\Core\StringTranslation\TranslationInterface::translate()

and... I think we should check where:
\Drupal\Core\StringTranslation\StringTranslationTrait::t()
points also.

YesCT’s picture

Issue summary: View changes
YesCT’s picture

Status: Needs work » Needs review
FileSize
8.49 KB
8.67 KB

just moved around the changes from before.

the similar functions do not have their own @param but, they didn't before either.

This can use a review.

YesCT’s picture

+++ b/core/lib/Drupal/Core/StringTranslation/TranslationInterface.php
@@ -17,26 +17,58 @@
+   * The t() function serves two purposes. First, at run-time it translates

but now this is translate() so will need to update some language.

I'll leave this for feedback on the moving around of the docs though.

jhodgdon’s picture

Status: Needs review » Needs work

I think moving this makes sense, but a few comments:

  1. +++ b/core/includes/bootstrap.inc
    @@ -258,56 +258,10 @@ function drupal_get_path($type, $name) {
    + * See \Drupal\Core\StringTranslation\TranslationInterface::translate() for
    + * details about important security information and usage guidelines.
    

    Also you would have to go there for details about the function parameters and return value.

  2. +++ b/core/lib/Drupal/Core/StringTranslation/StringTranslationTrait.php
    @@ -36,11 +36,10 @@
    +   * See \Drupal\Core\StringTranslation\TranslationInterface::translate() for
    +   * details about important security information and usage guidelines.
    

    Also you would have to go there for details about the function parameters and return value.

  3. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationInterface.php
    @@ -17,26 +17,58 @@
    +   * The t() function serves two purposes. First, at run-time it translates
    

    Well. This is documentation for the translate() method on this interface. And it starts out by saying:

    The t() function serves two purposes.

    So... that's a bit odd. It think it needs some information that tells you that you should not ever call this translate() method directly -- if I'm not mistaken, you must use t() function or t() method in order for the PO extraction to pick up the strings, right?

    So that should be added, and then I think this will make more sense in its current location.

    Thoughts?

YesCT’s picture

yeah. but, it would be nice to have the docs on the thing that we want people to use to call it. it would be more practical for people using, but not as nice strictly "correct", to have the docs on \Drupal\Core\StringTranslation\StringTranslationTrait::t()

yeah, I agree with #24 3.

herom’s picture

Re #24.3: Potx doesn't recognize the "translate()" method right now. And, if we wanted to support it, we would have to keep the "translate" method name reserved for the translation manager, otherwise potx would find a lot of false-positives.

YesCT’s picture

ok. I'm going to leave the bulk of the docs on translate() and work on the wording there.

jhodgdon’s picture

Um. So we don't want people to use translate(), and you said you wanted the docs on what we want people to use? So it seems they should be on the trait's t() method? Up to you really, but we do I think in any case need to add a note to translate() saying that you shouldn't normally call it directly.

Also I think the param/return docs for the trait's t() method should be on the trait's t() method. It seems really silly to say "You should use this function but you have to look over here to get its documentation". ?!? By that argument all the important docs should be on the trait.

YesCT’s picture

Status: Needs work » Needs review
FileSize
8.88 KB
3.02 KB

I think this addresses #24.

jhodgdon’s picture

Yeah but it doesn't address #28. Thoughts? I think the best place for these docs, from a *developer* perspective, would be on the trait's t() method, not on translate() (which shouldn't be used directly and really is an implementation detail that they shouldn't even know about unless they are working on the core translation API directly).

Right?

herom’s picture

In my opinion, the advantage of having this on the TranslationInterface is that someone might replace the translation manager service with their own implementation of TranslationInterface, and the docs would still be valid. But they would lose those docs if we moved them elsewhere.

alexpott’s picture

+++ b/core/lib/Drupal/Core/StringTranslation/TranslationInterface.php
@@ -17,26 +17,64 @@
+   * You should never use t() to translate variables, such as calling t($text)
+   * unless the text that the variable holds has been passed through t()
+   * elsewhere (e.g., $text is one of several translated literal strings in an
+   * array). It is especially important never to call t($user_text) where

This is now very incorrect. Infact an exception will be throw if you pass TranslatableMarkup into TranslatableMarkup.

YesCT’s picture

Issue summary: View changes

#2598874: Add support for D8 TranslatableMarkup was noticed as we were talking about... that \Drupal\Tests\user\Unit\TestTranslationManager::translate() is another wrapper for (creating a new) \Drupal\Core\StringTranslation\TranslatableMarkup object.

And that .... may be the preferred thing we want people to do instead of calling the procedural t().

So... well, I guess I will pull TranslatableMarkup into this issue.

jhodgdon’s picture

People ***must call t() function/method or formatPlural()*** with their UI strings. Otherwise they will not be detected by POTX, and hence will not be translated. They should not ever be calling translate() or making TranslatableMarkup objects directly.

YesCT’s picture

@jhodgdon
Talked with @alexpott and I think #2598874: Add support for D8 TranslatableMarkup means l.d.o will parse now that

YesCT’s picture

Assigned: Unassigned » YesCT
Status: Needs review » Needs work

revisiting #8 to try and understand better the subtlety that I had temporarily taken out.

went through some example, just noting them here so they dont get lost.

I'm also focusing on this issue today, assigning it to me. ping me to collaborate. :)

   * Because l.d.o is a static text analyzer. t('$text') vs t('text') vs t($text).
   * t('first' . 'second') // wont work, cause static
   * $value[1] = t('one');
   * t('your choice is' . $value[1]); // cause static
   * t('your choice is @value', array('@value' => $value[1])); // fine cause t('one') sent one to the translations system
   *
   * For user interface text your module is supplying, when there is a temptation to make an array of several literal strings like
   * @code
   * $values = ['home' => t('Home'), 'work' => t('Work'), 'operations' => t('Operations')];
   * print $values['work'];
   *
   * $values = ['home' => 'Home', 'work' => 'Work', 'operations' => 'Operations'];
   * print t($values['work']); // wont work
   *
   * $values = ['home' => 'Home', 'work' => 'Work', 'operations' => 'Operations'];
   * t('Home');
   * t('Work');
   * t('Operations');
   * print t($values['work']); // will work but is not a good idea
   * @endcode
   * the way to do that is to
   * @code
   * $values = ['home' => new TranslatableMarkup('Work');
   * @endcode
YesCT’s picture

This is just working on that clarification. for "unless the text that the variable holds has been passed through t() elsewhere "

If this direction for the clarification is something we want to do, I'm sure we can improve the wording in my interdiff.

but... fixing this is outside the scope of point to FormattableMarkup. :(
We could retitle this to: improve t() docs...

I'm still continuing to thank and work on this issue.

jhodgdon’s picture

We actually have a separate, active, actually RTBC issue whose title is almost exactly "Improve t() docs":
#2585781: Wrong documentation of t()
This issue's current patches may conflict. I was wondering why I was seeing double. ;)

Regarding this being out of scope, probably. Anyway I think the direction that interdiff is going is good and whether here or on another issue, we should eventually do something like that.

YesCT’s picture

Status: Needs work » Needs review
FileSize
9.26 KB

that went in. this is just a rebase.

gonna think about *where* next.

YesCT’s picture

Title: Point t() and TranslationManager::translate() docs to FormattableMarkup » Make translatable docs consolidated and better for developers
Issue summary: View changes
FileSize
5.77 KB

just posting a part, intermediate work.

how it might look to place (most) of the docs on TranslatableMarkup class.
changed some wording to make it more generic for when t() and $this->t() from the trait to point at this.

...

next, update the other locations. they will have @params and point to TranslatableMarkup. will post that next.

YesCT’s picture

+++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
@@ -58,24 +62,75 @@ class TranslatableMarkup extends FormattableMarkup {
+   * purposes. First, at run-time it translates user-visible text into the
+   * appropriate language. Second, various mechanisms that figure out what text

Not sure about "First" and "Second". might change this to One and Two or something... run-time translation doesn't really happen "first".

YesCT’s picture

looking at trying to actually explain when to use $this-t() from the trait and when to make a new TranslatableMarkup, I wrote something... a work in progress:

   * In order for strings to be localized, you must use either the $this->t()
   * method (if your class uses
   * \Drupal\Core\StringTranslation\StringTranslationTrait, or you should create
   * a new \Drupal\Core\StringTranslation\TranslatableMarkup object. It is
   * preferable to do $this->t() because if you make a new translation manager
   * it needs to be injected in all the proper places. And, it is easier to make
   * a new t() via the trait then to inject it in all the places it would be
   * for new TranslatableMarkup() to work.

but.... while discussing this, we noted that docs like this are difficult when they are for different audiences. so we reaffirmed that the goal here is to make better docs (not perfect ones) that are not incorrect. and then we can iterate more later.

So, I'm going to write something like:

When possible, use the trait and call $this->t(), otherwise create a new TranslatableMarkup object.

YesCT’s picture

ok. this needs an actual review.

Status: Needs review » Needs work

The last submitted patch, 43: 2578377.42.patch, failed testing.

jhodgdon’s picture

I guess it needs a reroll too. ;)

So, I gave this a careful readthrough -- wow, really nice work!

I have a few small suggestions here and there that might improve it a bit:

  1. +++ b/core/includes/bootstrap.inc
    @@ -258,56 +258,39 @@ function drupal_get_path($type, $name) {
    - *   - 'context' (defaults to the empty context): The context the source string
    - *     belongs to.
    + *   - 'context' (defaults to the empty context): The context the source
    + *     string belongs to.
    

    I'm not sure why this line was rewrapped -- the pre-patch line was already below 80 characters?

  2. +++ b/core/lib/Drupal/Core/StringTranslation/StringTranslationTrait.php
    @@ -36,11 +36,38 @@
    +   *   A string containing the English string to translate.
    

    Can we maybe change this (on the other functions/methods too) to:

    A string containing the English text to translate.

    (To me, "string containing the string" is ... weird at best!)

  3. +++ b/core/lib/Drupal/Core/StringTranslation/StringTranslationTrait.php
    @@ -36,11 +36,38 @@
    +   *   - 'langcode' (defaults to the current language): The language code to
    +   *     translate to a language other than what is used to display the page.
    

    This has been bothering me for quite a while, every time I looked at any of these docs. ;)

    Can we add a comma after "code", and change "the" to "a", so it says:

    A language code, to translate to a language ...

    And this would need to be done on the other methods/functions too.

  4. +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
    @@ -13,12 +13,16 @@
    + * Formats a translatable string for HTML display by replacing variable
    + * placeholders. Handles an optional context for the string. This object, when
    + * cast to a string, will yield the translated string.
    

    Given some discussions the last few days at BADCamp... wondering if we should just mention here that the translation etc. are delayed as long as possible and the real reason we have this is because it can be cached and then translated/etc. at the last possible moment, thus making it possible to cache just once for multiple languages?

    So maybe this for this intro paragraph, something like this:

    This object, when cast to a string, will yield the formatted, translated string. Avoid casting it to a string yourself, though -- it is preferable to let the rendering system do the cast as late as possible in the rendering process, so that this object itself can be put, untranslated, into render caches and thus the cache can be shared between different language contexts.

  5. +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
    @@ -13,12 +13,16 @@
      * This is useful for using translation in very low level subsystems like entity
      * definition and stream wrappers.
    

    I am not sure what purpose this paragraph serves, given the previous information? Maybe take it out?

  6. +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
    @@ -13,12 +13,16 @@
    + * This class delays translation until rendering.
    

    I think we can also lose this sentence. Anyway it's only accurate if the caller avoids making the cast, as described hopefully in the previous paragraph.

  7. +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
    @@ -58,17 +62,68 @@ class TranslatableMarkup extends FormattableMarkup {
    +   * use $this->t() instead of creating this object. But, in a class that has
    +   * t(), in a static method, create this object to do translation.
    

    huh, "a class that has t() in a static method" ???? what does this mean?

    I think I would change that second sentence to say:

    But if you are writing procedural code or your class doesn't use StringTranslationTrait, instantiate this object to do translation.

  8. +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
    @@ -58,17 +62,68 @@ class TranslatableMarkup extends FormattableMarkup {
    +   * Doing new TranslatableMarkup(), or calling the t() function, serves two
    

    I think I would change the start of this to say:

    Calling the trait's t() method or instantiating a new TranslatableMarkup object serves two purposes.

  9. +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
    @@ -58,17 +62,68 @@ class TranslatableMarkup extends FormattableMarkup {
    +   * appropriate language. Second, various mechanisms that figure out what text
    +   * needs to be translated work off the string -- the text inside the call is
    +   * added to the database of strings to be translated. These strings are
    

    Hm... maybe this wording for that sentence:

    Second, static analyzers detect calls to t() and new TranslatableMarkup, and add the first argument (the string to be translated) to the database of strings that need translation.

  10. +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
    @@ -58,17 +62,68 @@ class TranslatableMarkup extends FormattableMarkup {
    +   * passed through new TranslatableMarkup(), t(), or a related function. See
    +   * the @link https://www.drupal.org/node/322729 Localization API @endlink
    +   * pages for more information, including recommendations on how to break up or
    +   * not break up strings for translation.
    

    Could/should we instead link to a topic here (something in a defgroup) and make sure we have the essential information covered -- and then in that topic link to d.o for more info?

  11. +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
    @@ -58,17 +62,68 @@ class TranslatableMarkup extends FormattableMarkup {
    +   * - Using a variable for $string that is user input is a security risk.
    

    Normally, right before a list we would want to have a line ending in :

    So maybe:

    There are several reasons for this:

  12. +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
    @@ -58,17 +62,68 @@ class TranslatableMarkup extends FormattableMarkup {
    +   *   picked up by
    +   *   the localization static text processor. (Unless the text that the
    

    needs rewrapping here

  13. +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
    @@ -58,17 +62,68 @@ class TranslatableMarkup extends FormattableMarkup {
    +   *   the localization static text processor. (Unless the text that the
    +   *   variable holds has been passed through t() elsewhere. But that strategy
    

    Here I think I would say:

    (Unless the entire string in $text has been put into a t() call as one entire literal string elsewhere, but that strategy...

  14. +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
    @@ -58,17 +62,68 @@ class TranslatableMarkup extends FormattableMarkup {
    +   * or t($user_text) where $user_text is some text that a user entered - doing
    

    This should be a dash and not a hyphen near the end of this line.

    But rather than including a non-Ascii em-dash character here just use

    --

  15. +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
    @@ -58,17 +62,68 @@ class TranslatableMarkup extends FormattableMarkup {
    +   * Basically, you can put variables like @name into your string, and it will
    +   * substitute their sanitized values at translation time. (See the
    

    In this sentence, "it" doesn't really have an antecedent.

    I think I would say

    ... and sanitized values will be substituted at translation time.

  16. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationInterface.php
    @@ -17,26 +17,35 @@
    +   * @link https://www.drupal.org/node/322729 Localization API @endlink. When
    

    Again, it's much preferable to link from functions/classes to topics (that are defined in defgroups) so that we make sure we have the essential information covered in the topics, and then have those topics link to d.o for more info.

YesCT’s picture

Status: Needs work » Needs review
FileSize
15.41 KB

just a reroll for #2576431: Improve t() docs "fully-translatable site" is overstating
needs review just for the bot.

this needs work.
fixes on their way.

YesCT’s picture

Status: Needs review » Needs work
YesCT’s picture

Status: Needs work » Needs review
FileSize
18.19 KB
13.8 KB

some fixes that @dawehner found and relayed to me.

also, addressing #45

1. "I'm not sure why this line was rewrapped -- the pre-patch line was already below 80 characters?"
fixed.
this happened because I copied it out of classes, and classes are indented more. and this bootstrap.inc isn't cause the methods are not in a class.

10. 16. I'm not sure what to do about. Can we make defgroups in another issue?

jhodgdon’s picture

YesCT’s picture

@jhodgdon are you suggesting I change the i18n defgroup to have the information (adding information about the ways supported by the Localization API) and could do:

In order for strings to be
   * localized, make them available in one of the ways supported by the
   * @link i18n Localization API @endlink

?

Since we already link to the API handbook page, I suggest we leave it with the link to the page, and do the defgroup thing in a separate issue. #2600928: link to i18n defgroup (and update the information) instead of linking to the Localization API handbook page

YesCT’s picture

read through the whole patch, and fixed a few nits.

jhodgdon’s picture

Status: Needs review » Needs work

OK on the separate issue for the links.

Reviewed the #51 patch... Really looking good! I think we're down to nitpicks and proofreading.

Some comments:

  1. +++ b/core/includes/bootstrap.inc
    @@ -258,56 +258,39 @@ function drupal_get_path($type, $name) {
    + *   (optional) An associative array of replacements to make after
    + *   translation. Based on the first character of the key, the value is escaped
    

    Wrapping could be better here? (first line could be longer I think)

  2. +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
    @@ -13,13 +13,13 @@
    + * This object, when cast to a string, will return formatted, translated string.
    

    needs "the" before "formatted"

  3. +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
    @@ -58,17 +58,74 @@ class TranslatableMarkup extends FormattableMarkup {
    +   * use $this->t() instead of creating this object. But, even a class that has
    

    Maybe
    ... instead of creating this object directly. ...

    Because I think $this->t() does create one of these objects, right?

  4. +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
    @@ -58,17 +58,74 @@ class TranslatableMarkup extends FormattableMarkup {
    +   * use $this->t() instead of creating this object. But, even a class that has
    +   * t() might have a static method, in that static method it will need to
    +   * create this object to do translation.
    

    I still have zero idea what this "static method" stuff is talking about?

    Oh. Hm. I see.

    OK.

    Here's my suggestion for this entire paragraph:

    The (namespace)TranslatableMarkup class should always be used to make translations. That said, if your class uses (namespace)StringTranslationTrait, always call $this->t() instead of creating a (namespace)TranslatableMarkup object directly. Only create an object directly if your class doesn't use the trait, in static methods on any class, or in procedural code.

  5. +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
    @@ -58,17 +58,74 @@ class TranslatableMarkup extends FormattableMarkup {
    +   * - at run-time it translates user-visible text into the appropriate
    

    Normally, start bullet list items with capital letters.

  6. +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
    @@ -58,17 +58,74 @@ class TranslatableMarkup extends FormattableMarkup {
    +   * - static analyzers detect calls to t() and new TranslatableMarkup, and add
    

    Capital letter. There are other spots in the patch that need attention for that too.

  7. +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
    @@ -58,17 +58,74 @@ class TranslatableMarkup extends FormattableMarkup {
    +   * @link https://www.drupal.org/node/322729 Localization API @endlink.
    +   * See the @link https://www.drupal.org/node/322729 Localization API @endlink
    

    We might not need links to this same page in both of these lines. ;) But OK to leave them too.

  8. +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
    @@ -58,17 +58,74 @@ class TranslatableMarkup extends FormattableMarkup {
    +   * $string should always contain, at least in part, an English literal.
    

    "at least in part"??!? That is going to cause confusion. It's explained way better below so... possible to leave this out and maybe just say it should be in English, rather than "contain, at least in part"?

  9. +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
    @@ -58,17 +58,74 @@ class TranslatableMarkup extends FormattableMarkup {
    +   * $string should never contain a variable, such as:
    +   * @code
    +   * new TranslatableMarkup($text)
    

    Maybe it would be good to say:

    $string should never contain a variable or the result of a calculation, such as:

    And then add some more examples, like:

    new TranslatableMarkup('Part 1' . ' ' . 'Part 2')

    new TranslatableMarkup($options[$foo])

  10. +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
    @@ -58,17 +58,74 @@ class TranslatableMarkup extends FormattableMarkup {
    +   * - using a variable for $string that is user input is a security risk,
    

    Normally end a list item with a . not a ,

  11. +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
    @@ -58,17 +58,74 @@ class TranslatableMarkup extends FormattableMarkup {
    +   * - using a variable for $string that has even guaranteed safe text, for
    +   *   example, user interface text provided literally in code, will not be
    

    punctuation:

    ... safe text (for example, user interface text provided literally in code), will not...

  12. +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php
    @@ -58,17 +58,74 @@ class TranslatableMarkup extends FormattableMarkup {
    +   *   picked up by the localization static text processor. (Unless the entire
    +   *   string in $text has been processed for translation as one entire literal
    +   *   string elsewhere. But that strategy is not recommended.)
    

    These two things in the parens are not actually sentences.

    Maybe:

    (You could possibly get away with it if the entire string in $text has been passed into t() or new TranslatableMarkup() elsewhere as the first argument, but that strategy is not recommended.)

  13. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationInterface.php
    @@ -17,26 +17,35 @@
    +   * @see \Drupal\Core\StringTranslation\TranslatableMarkup::__construct()
    +   * @ingroup sanitization
    

    Probably best to leave a blank line between the @see lines and @ingroup

YesCT’s picture

thanks. I'm doing these now.

YesCT’s picture

4. nice. I like "Only create an object directly if your class doesn't use the trait, in static methods on any class, or in procedural code." a lot.
when I went to change it, I remembered that we had some other words ... and then I tried to combine them, and thought it was repetitive... so just went with the other words:

   * When possible, use the
   * \Drupal\Core\StringTranslation\StringTranslationTrait $this->t(). Otherwise
   * create a new \Drupal\Core\StringTranslation\TranslatableMarkup object
   * directly.

(with the nice directly clarification.)

8. "contain, at least in part"
yeah, that is awkward, but that line was added because @dawehner suggested we start with saying what the string *should* be before (in the next part) saying what it should not be. and so... that phrase is the point.

How about
* $string should always be an English literal string.

it is less complex (but also less completely correct)

YesCT’s picture

Status: Needs work » Needs review
FileSize
18.16 KB
3.8 KB

Thanks so much for the many thoughtful through quick reviews.

See if these changes look good enough (they are not *exactly* the suggested changes). :)

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +rc eligible

Looks great! Let's do it.

This is RC eligible, as it just changes API docs.

The last submitted patch, 14: 2578377.14.patch, failed testing.

The last submitted patch, 16: 2578377-15.patch, failed testing.

The last submitted patch, 19: 2578377.18.patch, failed testing.

The last submitted patch, 22: 2578377.22.patch, failed testing.

The last submitted patch, 29: 2578377.28.patch, failed testing.

The last submitted patch, 37: 2578377-36.patch, failed testing.

The last submitted patch, 39: 2578377-39.patch, failed testing.

The last submitted patch, 43: 2578377.42.patch, failed testing.

Gábor Hojtsy’s picture

Priority: Normal » Major
Issue tags: +Security

This is important security documentation. Elevating accordingly.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This is great because it fixes lots of incorrect docs and now we have one place to collect this documentation - TranslatableMarkup. Addressing @herom's point in #31 about TranslationInterface... in fact the translate() method is largely pointless and in fact we could remove it. If someone swaps out the TranslationManager they we need to be able to translate the TranslatableMarkup value objects - another reason why this design is so nice.

I've credited everyone who commented on this issue include myself and @xjm because there were several in-person discussions about this patch @badcamp.

I think there might be several things that might be polished in further patches but I think perfect here would be the enemy of progress so... Committed 04966aa and pushed to 8.0.x. Thanks!

YesCT++

  • alexpott committed 04966aa on 8.0.x
    Issue #2578377 by YesCT, jhodgdon, Gábor Hojtsy, alexpott, herom, xjm:...
Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, thanks!

  • alexpott committed 04966aa on 8.1.x
    Issue #2578377 by YesCT, jhodgdon, Gábor Hojtsy, alexpott, herom, xjm:...

Status: Fixed » Closed (fixed)

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