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.
Comment | File | Size | Author |
---|---|---|---|
#55 | interdiff.2578377.51.54.txt | 3.8 KB | YesCT |
#55 | 2578377.54.patch | 18.16 KB | YesCT |
#51 | interdiff.2578377.48.51.txt | 4.25 KB | YesCT |
#51 | 2578377.51.patch | 18.18 KB | YesCT |
#48 | interdiff.2578377.46.48.txt | 13.8 KB | YesCT |
Comments
Comment #2
xjmZoom!
Comment #3
YesCT CreditAttribution: YesCT commentedI'm going to work on this now.
Comment #4
jhodgdondocs component perhaps? :)
Comment #5
YesCT CreditAttribution: YesCT commentedjust a start. wip.
Comment #6
YesCT CreditAttribution: YesCT commentedretitling since #2576533: Rename SafeStringInterface to MarkupInterface and move related classes renamed FormattableString to FormattableMarkup
Comment #7
YesCT CreditAttribution: YesCT commentedComment #8
jhodgdonThis may not be finalized, but I had one thought:
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.
Comment #9
YesCT CreditAttribution: YesCT commentedComment #10
Gábor HojtsyThe patch looks good to me. I agree with @jhodgdon on the subtlety lost, that would be good to get back though.
Comment #11
YesCT CreditAttribution: YesCT commentedI 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?
Comment #12
Gábor HojtsyThat was meant to convey:
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.
Comment #13
YesCT CreditAttribution: YesCT commentedif only
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.
Comment #14
YesCT CreditAttribution: YesCT commentedok. how is this?
Comment #15
Gábor HojtsyThat 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.
Comment #16
YesCT CreditAttribution: YesCT commentedor... 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.
Comment #17
YesCT CreditAttribution: YesCT commentedwait... I thought print t($home $operations); was not ok, and should be done with placeholders....
Comment #18
Gábor Hojtsy@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...
Comment #19
YesCT CreditAttribution: YesCT commented@Gábor Hojtsy :)
---------
oh, missed one in the t @param.
Comment #20
YesCT CreditAttribution: YesCT commentedand... 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.
Comment #21
YesCT CreditAttribution: YesCT commentedComment #22
YesCT CreditAttribution: YesCT commentedjust 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.
Comment #23
YesCT CreditAttribution: YesCT commentedbut 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.
Comment #24
jhodgdonI think moving this makes sense, but a few comments:
Also you would have to go there for details about the function parameters and return value.
Also you would have to go there for details about the function parameters and return value.
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?
Comment #25
YesCT CreditAttribution: YesCT commentedyeah. 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.
Comment #26
herom CreditAttribution: herom commentedRe #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.
Comment #27
YesCT CreditAttribution: YesCT commentedok. I'm going to leave the bulk of the docs on translate() and work on the wording there.
Comment #28
jhodgdonUm. 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.
Comment #29
YesCT CreditAttribution: YesCT commentedI think this addresses #24.
Comment #30
jhodgdonYeah 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?
Comment #31
herom CreditAttribution: herom commentedIn 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.
Comment #32
alexpottThis is now very incorrect. Infact an exception will be throw if you pass TranslatableMarkup into TranslatableMarkup.
Comment #33
YesCT CreditAttribution: YesCT commented#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.
Comment #34
jhodgdonPeople ***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.
Comment #35
YesCT CreditAttribution: YesCT commented@jhodgdon
Talked with @alexpott and I think #2598874: Add support for D8 TranslatableMarkup means l.d.o will parse now that
Comment #36
YesCT CreditAttribution: YesCT commentedrevisiting #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. :)
Comment #37
YesCT CreditAttribution: YesCT commentedThis 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.
Comment #38
jhodgdonWe 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.
Comment #39
YesCT CreditAttribution: YesCT commentedthat went in. this is just a rebase.
gonna think about *where* next.
Comment #40
YesCT CreditAttribution: YesCT commentedjust 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.
Comment #41
YesCT CreditAttribution: YesCT commentedNot sure about "First" and "Second". might change this to One and Two or something... run-time translation doesn't really happen "first".
Comment #42
YesCT CreditAttribution: YesCT commentedlooking 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:
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.
Comment #43
YesCT CreditAttribution: YesCT commentedok. this needs an actual review.
Comment #45
jhodgdonI 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:
I'm not sure why this line was rewrapped -- the pre-patch line was already below 80 characters?
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!)
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.
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.
I am not sure what purpose this paragraph serves, given the previous information? Maybe take it out?
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.
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.
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.
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.
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?
Normally, right before a list we would want to have a line ending in :
So maybe:
There are several reasons for this:
needs rewrapping here
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...
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
--
In this sentence, "it" doesn't really have an antecedent.
I think I would say
... and sanitized values will be substituted at translation time.
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.
Comment #46
YesCT CreditAttribution: YesCT commentedjust 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.
Comment #47
YesCT CreditAttribution: YesCT commentedComment #48
YesCT CreditAttribution: YesCT commentedsome 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?
Comment #49
jhodgdonWe already have topics and docs for this stuff I think:
https://api.drupal.org/api/drupal/core!lib!Drupal!Component!Render!Forma...
https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Language!language...
?
Comment #50
YesCT CreditAttribution: YesCT commented@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:
?
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
Comment #51
YesCT CreditAttribution: YesCT commentedread through the whole patch, and fixed a few nits.
Comment #52
jhodgdonOK 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:
Wrapping could be better here? (first line could be longer I think)
needs "the" before "formatted"
Maybe
... instead of creating this object directly. ...
Because I think $this->t() does create one of these objects, right?
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.
Normally, start bullet list items with capital letters.
Capital letter. There are other spots in the patch that need attention for that too.
We might not need links to this same page in both of these lines. ;) But OK to leave them too.
"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"?
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])
Normally end a list item with a . not a ,
punctuation:
... safe text (for example, user interface text provided literally in code), will not...
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.)
Probably best to leave a blank line between the @see lines and @ingroup
Comment #53
YesCT CreditAttribution: YesCT commentedthanks. I'm doing these now.
Comment #54
YesCT CreditAttribution: YesCT commented4. 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:
(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)
Comment #55
YesCT CreditAttribution: YesCT commentedThanks so much for the many thoughtful through quick reviews.
See if these changes look good enough (they are not *exactly* the suggested changes). :)
Comment #56
jhodgdonLooks great! Let's do it.
This is RC eligible, as it just changes API docs.
Comment #65
Gábor HojtsyThis is important security documentation. Elevating accordingly.
Comment #66
alexpottThis 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 thetranslate()
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++
Comment #68
Gábor HojtsyYay, thanks!