Problem/Motivation
When something is annotated with a translation (or t() is called in an info hook), the translation system kicks in immediately.
This happens for very low level subsystems like entities and stream wrappers. See #2241461: locale + basic_auth results in current_user circular reference for a concrete example.
Often the translated keys are only needed in very specific situations - i.e. an entity type or bundle level is hardly ever rendered on runtime, only in the administrative interface.
Proposed resolution
Delay translating in Drupal\Core\Annotation\Translation
until we need to use the value as a string.
Remaining tasks
Review patch
User interface changes
None
API changes
Drupal\Core\Annotation\Translation::get()
returns an object which has a __toString()
Comment | File | Size | Author |
---|---|---|---|
#61 | 2244447.61.patch | 21.26 KB | alexpott |
#61 | 59-61-interdiff.txt | 2.23 KB | alexpott |
#59 | 2244447.59.patch | 21.14 KB | alexpott |
#59 | 54-59-interdiff.txt | 816 bytes | alexpott |
#54 | 2244447.54.patch | 20.84 KB | alexpott |
Comments
Comment #1
BerdirAs just discussed in IRC, for entity types, we could do the translation when you call getLabel(), but we a) can't do that for plugin types that still use arrays (everything else) and we still need a way to flag strings as translatable.
A nice side effect of this would be that we can avoid to cache by language, which would avoid quite a bit of complexity for plugin managers.
Comment #2
Gábor HojtsyWe can make up another annotation special function, instead of @Translation it could be something else, that is a no-op but the extractor can pick up.
Comment #3
Gábor HojtsyAlso is this a beta blocker? It definitely involves API changes.
Comment #4
catchProbably should be. If we determine it isn't we can bump it back down.
Comment #5
xjmSounds like this is at least two separate issues? One for entities, another for everything else?
And yeah, reviewing this, I think it potentially needs to be beta-blocking for its impacts on the plugin and entity APIs.
Comment #6
Gábor HojtsyAn alternate solution is any annotation value for a specific key, eg. "display_label" or something would be extracted for translation. The base classes can include a method to return the translated metadata label. Although this also starts being messy since that would be the plugin type label not the plugin label itself :D Heh.
Comment #7
sunIf it's just about annotations, then we should be able to easily change the processing code to retain the
Translation
object as-is and move the actual invocation of t() into__toString()
.To do so, the
Translation
annotation class should implementSerializable
.Not exactly sure what I'm patching, but here's a prototype.
Comment #8
xjmPer @tim.plunkett stream wrappers are not and will not be implemented as plugins (and they are not annotated), so they at least should have a separate issue if there's a similar problem there.
Edit: Issue for the stream wrapper conversions: #2028109: Convert hook_stream_wrappers() to tagged services.
Comment #10
killua99 CreditAttribution: killua99 commented7: drupal8.anno-trans.7.patch queued for re-testing.
Comment #12
BerdirI'm scared about going down that path, but let's try it for real, the other patch still called t() in the constructor and the missing serialization blew up the testbot completely with the famous unserialize() offset errors.
Comment #13
Gábor HojtsyThis does not look that bad ;)
Comment #15
BerdirYeah, expected something like that.
__toString() has limits, A lot of the exceptions for example caused by passing an array of labels to an select field, which treats objects as nested optgroups.
A change like that would lead to very unexpected behavior and while we could handle it in getLabel() of entity types, other plugin definitions aren't so lucky...
Comment #16
sunComment #18
tim.plunkettWe have unit test coverage of this code (moving in #2259301: Move OptGroup tests out of FormBuilderTest), we should add to it.
Comment #19
tim.plunkettAll I see in the OP is an example of a fixed issue. I don't quite understand why this is a *beta blocker*.
Comment #20
catch@Tim #2241461: locale + basic_auth results in current_user circular reference didn't really fix the circular dependency, just worked around it.
Beta blocker because we may have to either change the translation annotation or the return value of t() (to a class with __toString() or similar).
If people think that's OK to do after beta we could untag it, but I'm much more concerned about genuine beta blockers slipping through the net than vice versa. If the beta has a couple of additional critical task/bugs fixed in it that it otherwise would, that's better than intrusive API changes after it's cut.
Comment #21
Gábor HojtsyAlthough performance should not be judged by gut, my gut feel is returning a class with __toString() from t() at all times sounds like something that would be problematic for performance at best :/
Comment #22
catch@Gabor that's already being done by #1825952: Turn on twig autoescape by default (also might make it harder to do here).
Comment #23
alexpottFixed form.inc exceptions and added to OptGroupTest
Comment #25
alexpottHopefully fixing up the tests.
Comment #27
alexpottFixing the last exception
Comment #28
sunAll of these changes could use an inline comment to clarify why the code is doing what it is doing.
Aside from that, this looks good to me.
Comment #29
sunWondering whether the following hunk could/should check explicitly for
instanceof Translation
to avoid false-positives?"What could possibly go wrong?" should drive that aspect.
Comment #30
alexpottComments added and some small performance improvements and bringing this inline with the same changes as #1825952: Turn on twig autoescape by default
Comment #31
sunThanks! That addresses my concerns.
Comment #32
BerdirThis is kind of interesting, it means that we store translatable strings in configuration? Isn't that a problem? I suspect this happens for views for example?
If those are the kind of hacks that we want to live with, so be it ;)
Some sort of profiling would be nice here, but I'm not sure what exactly we should profile.
Also, this is tagged with needs issue summary update and I'm also pretty sure this needs a change record?`So setting back to needs review for now.
For the record, basic_auth in a non-en site (or one that translates en) is currently broken again and this time, it's not the annotations but a loop due to t() calls in the the field definitions in combination with the per-role translation cache. But I think the "solution" there is simply that auth providers can't use load() to get a user object, they have to use UserSession instead.
Comment #33
alexpott1,2 - Maybe we can avoid all of this by casting the display_title in the view.
I'm not sure of what to profile either.
Comment #35
BerdirActually, thinking more about this, there's an interesting side effect.
This means that whatever plugin labels we use are translated at runtime and not on discovery, which means that they're no longer cached.
So on the requests that actually need that information we have additional t() calls, which are then however cached by the LocaleLookup cache collector I guess.
However, this essentially means that our plugin definitions are no longer language specific* and we could possibly remove the by-language caching that we're doing by default in a follow-up?
* With exceptions, like modules might be calling t() in alter/build hooks or derivatives?
Comment #36
BerdirSo something to check might be if/how many plugin labels we're accessing for normal requests, if any? I guess most will be backend only.
Comment #37
alexpott@Berdir yep I think we could remove the caching per language - but yep definitely a followup.
Comment #38
effulgentsia CreditAttribution: effulgentsia commentedIn light of #32.1 and #33, is this still needed?
Comment #39
effulgentsia CreditAttribution: effulgentsia commentedWhile changing plugin definition cache to not be per language can be follow up, can we really defer *_info(), *_alter(), and derivatives to a follow up? For example, are we sure we want Annotation::get() to return $this, which is not alterable, or do we want to introduce a separate class in the StringTranslation component instead? Not necessarily to replace all cases of t() with, but at least to replace the ones related to plugin definitions, whether they come from annotations or not.
Comment #40
tstoecklerI was going to ask a similar question. I think it's totally fine to document not to use language-specific information in *_alter() (i.e. do not use t()), but instead you should simply be able to do
$definition['title'] = new TranslatedString('My title');
. I don't think instantiating an Annotation would make much sense in that context.Comment #41
alexpottSomething like this?
Comment #43
effulgentsia CreditAttribution: effulgentsia commentedWondering if the class name is confusing, given that TranslationInterface exists in the same namespace, but this class doesn't implement it. Maybe TranslationWrapper? Or a better one if someone has a suggestion?
Comment #44
alexpottFixed tests and renamed class to TranslationWrapper
Comment #45
tstoecklerThanks @alexpott! I don't know what others think, but I personally find that much more natural than linking it to the Annotation class.
Regarding the name: I'd rather think we should rename
TranslationInterface
and go withTranslation
(or evenStringTranslation
for this one), but that might be just me. I can certainly live withTranslationWrapper
as well.Comment #46
alexpottChanged hook_stream_wrappers() to use the new TranslationWrapper class and added test based on failure from #2241461: locale + basic_auth results in current_user circular reference
Comment #48
ParisLiakos CreditAttribution: ParisLiakos commentedi dont see why we need the as statement. StringTranslation is really generic, i like TranslationWrapper more
should be contains
@param needs update
nice, but i cant see where the inheritdoc comes from :)
needs visibility
shouldnt the @see point to TranslationWrapper?
this means we need to train the string extractor to also look for
new TranslationWrapper(*)
besidest(*)
and$this->t(*)
.So a followup for that should be opened
Comment #49
alexpottComment #51
alexpott#2016629: Refactor bootstrap to better utilize the kernel broke this.
Comment #52
alexpott49: 2244447.49.patch queued for re-testing.
Comment #53
ParisLiakos CreditAttribution: ParisLiakos commentedi meant there are 3 @params here and not just one?
Opened #2280843: Add support for D8 TranslationWrapper
Comment #54
alexpottLol fixed
Comment #55
effulgentsia CreditAttribution: effulgentsia commentedPatch looks great. Just needs a change record, and then I think it can be RTBC.
Comment #56
mrf CreditAttribution: mrf commentedWorking on this change record right now over at Documentation table at sprint.
Comment #57
mrf CreditAttribution: mrf commentedDraft of change record here: https://drupal.org/node/2281377
Comment #58
ParisLiakos CreditAttribution: ParisLiakos commentedIt took me a while to figure out why the early_translation_test module is needed and what exactly it helps.
(or well maybe i still did not get it?)
The class comment is wrong. It actually tests parsing the annotation of User's entity storage, that contains a translation wrapped by TranslationWrapper?
Comment #59
alexpottImproved the test help.
Comment #60
ParisLiakos CreditAttribution: ParisLiakos commentedthanks, thats definitely helpful
Comment #61
alexpott@tim.plunkett noticed some things.
Comment #62
tim.plunkettMuch much cleaner! Thanks.
Comment #63
catchOpened #2281905: Stop caching plugin discovery/info hooks by language.
Committed/pushed to 8.x, thanks!
Comment #65
XanoThis breaks when the wrapper is used for the needle argument of
strpos()
. See #2282453: strpos() does not work with TranslationWrapper for a fix.Comment #67
effulgentsia CreditAttribution: effulgentsia at Acquia commentedFolks here might be interested in #2509180: [meta] Various plugin definition alter() hooks incorrectly document to use t() .