Problem/Motivation
Locale module's string translation is a powerful old style API which got bloated through the years with several special cases and new features. There are very specific workarounds in the installer and some hacks to support fixed runtime string overrides. By unifying this system under one API and using that across all systems, we can remove the boundaries between the installer and the runtime and make the translation system properly pluggable.
The current system has various inter-dependencies and is hard to debug, see #1807272: Mixed locale issues with upgrade scripts, unit tests, 'Table simpletest..locales_source doesn't exist', etc... for example.
Proposed solution
Introduce a TranslationInterface that is implemented by both simpler string / file based lookups as well as the "regular" Locale module database lookup. Implement a TranslationFallback wrapper to combine these lookups in order of preferred lookup so fallback between different methods is possible. Finally, use the dependency injection container to define which methods of lookup are to be used. The installer would use the file based lookup, while fully bootstrapped runtime would use the LocaleTranslation lookup (with fallbacks).
This makes it possible to remove st(), get_t() and $t (in a followup at #1838310: Remove st(), get_t() and $t for good to not clutter this issue up) in addition to all the API cleanup and generalization benefits.
Class structure
Comment | File | Size | Author |
---|---|---|---|
#137 | 1813762-locale_translation_interface-137.patch | 67.32 KB | ParisLiakos |
#137 | interdiff.txt | 5.19 KB | ParisLiakos |
#136 | 1813762-locale_translation_interface-136.patch | 65.69 KB | ParisLiakos |
#129 | 1813762-locale_translation_interface-129.patch | 65.75 KB | ParisLiakos |
#129 | interdiff.txt | 757 bytes | ParisLiakos |
Comments
Comment #1
Gábor HojtsyDid you do any performance testing of this? It looks bulletproof and pretty slow (gut feel, not scientific).
I think if we can prove the file translator can nicely fit in a 500k-800k .po file in memory for the lookups (which the D7 translation instructions actually instruct people to do), we might get away removing this special case, which would be a great cleanup.
Only, if we can keep the 500k-800k .po's file data representation in memory that is.
Comment #2
andypostI think this should be registered in container much early so there no need to try..catch
Comment #3
Jose Reyero CreditAttribution: Jose Reyero commented@andypost,
Right, I realize I have overlooked the place where it should be, that is inside drupal_container() function, that should at least provide some default translator, though it can be an empty one.
The installer one (files) should be possibly in install.inc and StringOverrides should be registered in CoreBundle.php
@Gábor Hojtsy,
Nope, no performance tests yet, I don't know how fast is drupal_container()->get('string.translation') but on the other side it's saving a lot of logic inside t().
As a side note I think handling one time exceptions should be much faster than double checking for conditions every time, though I'd like to see some benchmarks about it.
Comment #4
Gábor HojtsyAdding language-ui tag.
Comment #5
Jose Reyero CreditAttribution: Jose Reyero commentedNew version of the patch (Moved around too many things so I don't think an inter-diff will be useful):
- Proper initialization of DIC locale translation objects so no need to try-catch anymore in t()
- Cleaned up file translation methods and functions, now FileTranslation is mostly self contained.
- Since contrib modules can now use DIC replacement, no need for 'reset/get all translations' methods in locale translation. Simplified LocaleLookup too. Localization client should be able to inject its own translator and get whatever data it needs from page strings.
- Using st() as a t() wrapper that only adds the install language in.
- Moved locale reset from locale module into the TranslationInterface objects.
To do / Questions:
- Do we need to mark installer strings? If we use the full translation po file for the installer I don't think we need this and we could drop both st() and get_t() entirely. We would need to make drupal_language_init() installer aware though.
- There's still the issue of JavaScript strings being regenerated (locale_js_alter) before the module is properly initialized for which I've added some try-catch in locale_storage(). I think this is an issue with core module loading though we need a workaround in the meanwhile (Module hook invoked before the DIC is properly initialized)
Comment #7
Gábor HojtsyAdd feature freeze tag.
Comment #8
japicoder CreditAttribution: japicoder commentedThe last patch does not apply. I take it and try to improve.
Comment #9
japicoder CreditAttribution: japicoder commentedPatch reroll. I've been testing this, but it's not working actually. The problem is that the Container is not able to find the new Locale interface, but it's defined in drupal_container(). I can't figure why it's not working.
Comment #10
Jose Reyero CreditAttribution: Jose Reyero commentedMoved to 'needs review' to trigger testbot.
About the DIC issues, the initialization of the container is a bit of a mess atm (in Drupal core I mean) and you need to initialize it in a few different places for it to work in all cases. (drupal_container, install.inc, CoreBundle.php...)
Also since it is used before full bootstrap, and even before language is initializaed, because of file_get_stream_wrappers(), this case is still harder. See _drupal_bootstrap_code(), _drupal_bootstrap_full().
Comment #12
webflo CreditAttribution: webflo commented#9: 1813762-locale_translation_interface-9.patch queued for re-testing.
Comment #14
Jose Reyero CreditAttribution: Jose Reyero commentedAbout the reroll in #9, it is missing some parts so I'm continuing from the patch #5
The reason why it was failing is because when locale module is disabled, t() still gets called a few times till the container is rebuilt. Since StringDatabaseStorage relies on schema definition to update strings, when the module is disabled (not in module list anymore) we don't have a schema for locale tables. So this is just another one of that edge conditions that make this locale really hard to debug and if we add DIC to the equation it gets still worse.
The solution (in the patch) is to remove the dependency on drupal_get_schema() of the StringDatabaseStorage, which also saves some code from the storage class. The only drawback is that if we change the schema we need to update the storage code but well, that's not hard to do.
Comment #15
Jose Reyero CreditAttribution: Jose Reyero commentedSome notes about testing and memory benchmarks for the file translation feature:
- Using a 800 Kb po file (drupal-7.15.es.po) placed in sites/[sitefolder]/files/translations/
- The memory used to load and parse it and get a translation seems to be around 1.8 Mb.
Some code for testing and get a translated string:
Comment #16
attiks CreditAttribution: attiks commentedNice work, this looks good to me, 2 minor remarks:
I think it's best to use t() everywhere.
is the comment needed?
Comment #17
Gábor HojtsyJose: thanks for the memory testing! How is this compared to the full page memory used? http://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/constan... says D8 requires 32MB memory.
Comment #18
attiks CreditAttribution: attiks commentedFYI: I did an drupal 8 install using 28MB, it works so we have 4MB for the po files.
Comment #19
Gábor HojtsyHm, I'm concerned for distros like Drupal Commerce, where there are (a) many modules enabled (b) MUCH MUCH bigger .po file for the distro. The answer to that might be:
- A: on-demand lookup in the .po file without parsing the whole thing (talked about this to Jose before) OR
- B: elevating the memory limit of Drupal in general OR
- C: not producing compound distro .po files on l.d.o so it would properly download individual module files
Probably C is something to consider anyway, since the localization update integration should not import the big overall compound file AND the individual module files as well. BUT this still looks like it could be on the borderline of the memory limit(?) if @attiks data is right.
Comment #20
vasi1186 CreditAttribution: vasi1186 commentedAfter a first review, I did not find anything that would look odd. I did not test the actually functionality, only code review. Just a small thing:
I know this code is correct, because of the operators precedence, but maybe just for an easier reading purpose, we should use brackets... anyway, this should not stop the issue to be rtbc in my opinion.
Comment #21
attiks CreditAttribution: attiks commentedFYI:
ini_set('memory_limit', '26M');
works as well.For distros this is probably going to break, even without this patch. So we probably have to increase the memory limit anyways (or allow the profile to change the limit)
What's left is #16 and #20
Comment #22
Jose Reyero CreditAttribution: Jose Reyero commentedSince I see no major objections here to the important pant of the patch (Update to DIC) while on the other side we are far from a definitive solution for whether to completely drop get_t()/st()/$t I think we should move on with the "important thing" let the other parts (that anyway are going to need a major cleanup patch) for later. So:
- Fixed issues found by @attiks #16 and @vasi186 #20
- Moving back get_t() function to its original form (we still need it if we keep 'st' for marking installer strings).
Note also that the 'st' function serves another purpose, that is injecting the installer language to t(), so there's another reason why it is still there.
And I really think we should:
1. Commit this first part which is an important step in bringing locale system up to date with DIC and D8 architecture.
2. Keep the issue at 'needs work' while we decide whether we can safely drop get_t()/st() and add the rest of the clean up that will be a few hundred lines of s/$t/t/ and s/st/t/.
Note: I am not too enthusiastic at all about raising the memory needed by Drupal installer, I think we should find a better solution for that (or keep the current one that is always an option).
Comment #23
attiks CreditAttribution: attiks commentedI agree with #22, let's do 'baby steps'
Assuming the bot will like this, this is RTBC
Comment #25
Jose Reyero CreditAttribution: Jose Reyero commentedUpdated 3 calls to (obsoleted by this patch) locale_reset() from test.
Comment #26
attiks CreditAttribution: attiks commentedSame as #23
I agree with #22, let's do 'baby steps'
Assuming the bot will like this, this is RTBC
Interdiff attached
Comment #27
catchOverall this looks great, it's refactoring some very old code and bringing it nicely up to date. I have a few questions on it, also I agree with Gabor that this could use performance testing - I'm mainly interested in seeing an xhprof run for a page with lots of t() calls, when the site is in English and has no translations - that's the usual case and the most likely to see a regression since we're currently hyper-optimized for this case. A translated page would be interesting as well but not required. Main thing would be number of function calls before/after, also whether there's any additional database queries (looks like there shouldn't be but would be good to confirm).
A few things came up, most of these are minor:
When can we actually hit this condition? If language service is in the container, then the answer ought to be 'never'.
Looks like we can just about get rid of st() now - would it just be necessary for the installer to set the language properly? Not here of course.
Not in this issue but we need to move the custom strings out of $GLOBALS['conf'] to some other namespace and stop using variable_get(). I opened #1833516: Add a new top-level global for settings.php - move things out of $conf last week, although I suppose in this case these could potentially just move to CMI.
Will this ever be used by itself? Or is it primarily a base class for custom strings and the file translator?
We've been calling these 'chains' elsewhere, for example see the cache chain backend. There's also ChainedRouter iirc.
Calling this a Fallback makes me think the class itself is the fallback, not that it has an array of classes to check to enable fallback.
It's great to see the same pattern being used but I think it's worth bringing the terminology into line with other subsystems.
Is this feature no longer needed? I guess you could register a translation service that listens or similar, so yeah, cool that it's no longer needed then ;)
Also just a note that this isn't the sort of patch that is blocked on feature freeze, this is straight refactoring of an existing API to bring it up to date, and doesn't really have any module developer-facing API changes in it. On the other hand it'd be good to get it committed soonish since this looks like it's a strong step towards
removing the language bootstrap phase(this has already gone, but 'low level language system dependencies/special casing'.Comment #28
Gábor HojtsyNote that this allows removal of get_t(), $t() and st() for good, which is a HUGE win IMHO.
Created this class figure, which probably demonstrates we should find a more standard name for "CustomStrings" as it looks odd among the *Translation ones.
Also updating the issue summary with a much cleaner explanation (hopefully :).
Comment #28.0
Gábor HojtsyUpdated issue summary.
Comment #28.1
Gábor HojtsySimplify, extend as needed :)
Comment #28.2
Gábor HojtsyFix followup link
Comment #28.3
Gábor HojtsyUpdated issue summary.
Comment #29
Gábor HojtsyProbably more human friendly title :)
Comment #30
Jose Reyero CreditAttribution: Jose Reyero commented@catch,
Ok about needing performance testing, though it all will come down to how fast is 'drupal_container()'.
I mean it's not just these patch that needs performance testing, all the other ton of patches committed would have needed it too. Still we don't want to make the locale system any slower so I'll add it to my to-do.
About translation service in drupal_container(),
Yes, the answer 'ought to be never'. The same it ought to be for all the stuff created in drupal_container() I guess.
However at this point we have some critical edge cases specific of the locale system, and though this patch will fix some of them, there's for instance file_get_stream_wrappers(), which uses t().
(My question would be wtf is that doing in _drupal_bootstrap_code(), but I'm afraid this may be another long story so...)
More about these edge cases here #1807272: Mixed locale issues with upgrade scripts, unit tests, 'Table simpletest..locales_source doesn't exist', etc...
Also my fear with latest code I've seen getting in, is other hooks like hook_entity_info() that may use t() may be triggered before this full initialization too.
* About this possibly the proper fix would be to kick t() out of all info hooks and provide a 'hook_menu()-like' way of translation these strings, but this may be another long story too.
And anyway it all comes down to whether we may be able to use t() anywhere, not st nor get_t(), but t() and for that to be true, we need at the very least some minimal "translation interface" anywhere drupal_container() is used so as long as we have other objects defined in drupal_container(), we should at least define some translation-pass-through thing.
About 'custom strings' as we have now:
- (with this patch) A way for modules to inject 'translation interface' objects for t() + a fallback system
- Locale system translating English strings too.
... *They should be kicked out of Drupal core*.
(Though I didn't want to add more potentially disruptive 'features' into this patch so I thought it better is 'backwards compatible' with what we've got atm, let that discussion for later).
About 'StaticTranslation', yes, that is a base class but the reason it's not abstract is because it could be used on its own for tests or other stuff to load a predefined set of translations to be used....
Ok about 'fallback' / 'chain' terminology, that will be in next reroll.
Comment #31
Gábor HojtsyRemove from sprint board to return after dec 1st then :)
Comment #32
fagoRelated suggestion: Use a translatable class to postpone translation, see #1542144-11: Mark strings as localizable/translatable (new t()-alike string function that isn't t(), only for potx). Afaik, I think it would play well with this improvements also.
Comment #33
Crell CreditAttribution: Crell commentedFlagging for myself.
Comment #34
YesCT CreditAttribution: YesCT commented#1703346: Make install system more translatable postponed on this.
Comment #35
YesCT CreditAttribution: YesCT commentedWhat shall we do here to move this forward? Are we waiting until after Feb 18th now?
Comment #36
YesCT CreditAttribution: YesCT commented#25 was passing... lets see.
Comment #37
YesCT CreditAttribution: YesCT commented#25: locale_translation_interface-25.patch queued for re-testing.
Comment #39
YesCT CreditAttribution: YesCT commenteddoes not apply, needs reroll
Comment #40
Gábor HojtsyI think given that this enables us to do major API cleanup, it would be still be possible post feb 18th. It will let us get rid of st(), get_t(), $t(), and let us use the same API for translation regardless of bootsrap/installation phase. That should be a huge win.
Comment #41
Gábor HojtsyAnyone interested in reviewing / doing performance testing? @Crell, you put this on your list earlier, but then did not come back :)
Comment #42
Jose Reyero CreditAttribution: Jose Reyero commentedI want to give another try to this one, coming from #1905152: Integrate config schema with locale, so shipped configuration is translated
First of all, I'll be updating the patch, that should be easier now DIC and module bundles have a number of improvements.
Comment #43
Gábor HojtsySounds amazing :)
Comment #44
Jose Reyero CreditAttribution: Jose Reyero commentedThis is the previous patch updated for latest core.
Set to 'needs review' for tests, but a few things still to-do:
- Address all issues in #27
- There seems to be some trouble with po importing tests
- Further t/st/get_t cleanup
Comment #45
Jose Reyero CreditAttribution: Jose Reyero commentedJust a minor update of the previous patch, with some more cleanup, still a few things to do.
Comment #47
jthorson CreditAttribution: jthorson commented#44 has been cycling on the testbots. Killing it manually. :(
Comment #48
Jose Reyero CreditAttribution: Jose Reyero commented#45: 1813762-locale_translation_interface-45.patch queued for re-testing.
Comment #49
Jose Reyero CreditAttribution: Jose Reyero commented@jthorson,
It's happening again, could it be this patch that has sth wrong?
Comment #50
jthorson CreditAttribution: jthorson commentedYes ... the reason these cycle is that something in the test results are preventing the communication between the testbot and qa.d.o. This may be due to there being so many failures/assertions that the communication times out or exceeds what the xmlrpc exchange can handle ... or the results end up with something in the log file which can't be processed by xmlrpc. In either case, it is specific to a given patch.
Comment #52
Jose Reyero CreditAttribution: Jose Reyero commentedNo idea about why tests are failing. I cannot run some tests locally, though it may be my environment more than this patch.
Anyway, progressing with the code, this version adds a few things:
1. Renamed TranslationFallback to TranslationChain as suggested by @catch #27
2. Added some methods that will be useful, following the 'Chains' model: appendTranslation(), prepenTranslation() that..
3. ...allow us to dinamically add in FileTranslation for the installer.
4. BIG: Full get_t() replacement, not we have only t().
5. Repurposing st(), now only fur strings that run on the installer before container is initialized.
6. Removed t() from language_default_locked_languages(). Not sure about the side effects but this was breaking the LanguageUpgrade tests, and it seems to me it doesn't make sense to use t() within language initialization.
Still a few things to do:
7. Update all the docs for t/st/get_t
8. Add in installer language into regular language initialization somehow (Otherwise some installer page runs may use default language instead of installer language when using t)
Giving one more try to the patch. I may need some help from someone that can run the tests locally, to have an idea of what's going on...
Comment #55
Jose Reyero CreditAttribution: Jose Reyero commentedStill no luck with tests. Some findings though:
- Tests fail with a not very helpful message of the type "The test did not complete due to a fatal error." On the apache log I get "[notice] child pid 30908 exit signal Segmentation fault (11)"
(Maybe that is why the testbot is crashing too).
- This seems to be related to unit tests using t() everywhere so if it is this function that fails, test error handling crashes too. Btw base test classes are plagued with t() too, see TestBase and WebTestBase classes.
- After the test fails, I get aditional warnings on the test results page, which seem to indicate form caching has crashed too:
All this seems to be related with t() function depending on DIC, which is the one thing introduced by the patch, and DIC not being properly handled on test crashes.
I can see some code in language() function too (not introduced by this patch) that looks like unit tests don't handle the DIC properly:
However, introducing similar code in the t() function doesn't work either, which seems to indicate the 'string translation' service is actually there but it is using the wrong drupal_container() at some points (the one from the test, that relies on locale installed, instead of the one from the unit test execution environment that doesn't have locale module enabled.
At this point I'd appreciate some help from someone who knows more about how unit tests handle the DIC.
Comment #56
Gábor HojtsyHave you tried bailing from t() altogether (by falling back on format_string()) if the container or string translation service is not there? Sounds like whatever unit tests test, they should be fine because they don't use a dual system like webtests, so if t() bails then it will be the same as if they did not call t() in the first place (or used format_string() for that matter).
Comment #57
ParisLiakos CreditAttribution: ParisLiakos commentedtagging
@Jose Reyero : are you working on a sandbox? i am gonna look into this later
Comment #58
ParisLiakos CreditAttribution: ParisLiakos commentedHere is a reroll since the Installer test moved..
i tested locally, run-scripts.sh is behaving normal...there are a bunch of fails, but nothing dies..maybe php version has something to do with it?i have 5.4.4 locally
Lets see what testbot says now
Comment #59
ParisLiakos CreditAttribution: ParisLiakos commentedSeems this got stuck again..
Comment #61
ParisLiakos CreditAttribution: ParisLiakos commentedlets try this
Comment #63
ParisLiakos CreditAttribution: ParisLiakos commentednow lets see fix some tests, if not all
Comment #65
ParisLiakos CreditAttribution: ParisLiakos commentedI tracked the 4 segfaults back to CacheArray::__destruct, which instantly reminded me the same problems Berdir had in the path whitelist patch..also i think in theme registry patch..so i applied the same solution waiting for #1786490: Add caching to the state system
lets see what testbot says now..bot??
Comment #66
ParisLiakos CreditAttribution: ParisLiakos commentedMade some changes to bring it to latest docs standards and usage of \Drupal instead of drupal_container.
What about getting rid of
locale_storage()
andst()
as well?Also i think
I will roll a patch tomorrow..and will try to inject more stuff to language classes so they get to be unit-testable as well
Comment #67
Jose Reyero CreditAttribution: Jose Reyero commentedWow! This looks green, finally! :-)
@rootatwc,
Thank you so much.
Now I'd like to give the patch another pass with some improvements I had been working on...
Comment #68
ParisLiakos CreditAttribution: ParisLiakos commentedJose Reyero sure! I made a some small changes as well (been working here http://drupal.org/sandbox/rootatwc/1969260) , but i will wait for your changes! if you want you can use the sandbox
Also can we leave get_t() removal for #1838310: Remove st(), get_t() and $t for good ? This start getting really big and we already have a dedicated issue about that :)
Also: #1975490: Convert locale_custom_strings_* to settings
Comment #69
Jose Reyero CreditAttribution: Jose Reyero commentedAdded a few changes / improvements:
- Rolled back the t/get_t/st replacement, agree it will be better a follow up, posted it here, #1838310: Remove st(), get_t() and $t for good
- Renamed 'locale.translation' to 'string_translation' for the service name in the DIC. For consistency with other service names and also the initial one is not related to locale module.
- Implemented Drupal::translation() to get the 'string translation' service.
- As suggested in #66 moved t() logic into translate(), which seems to work nicely and allows for more powerful translation services. The downside is the Translation objects now need to handle default language themselves, which may cause issues during install / initialization.
Since passing the 'language_manager' service to the constructor may cause problems like the language manager being initialized too early, add a default language to get started and a setDefaultLanguage() method, which allows on one side the whole thing running without a LanguageManager and on the other side more flexibility for initialization (use installer language, build a temporary Translation service for the installer, etc...)
- When language is initialized on drupal_init_language(), now we call this method, which also allows the Translation objects to react upon that if needed.
- Added some more consistency / reliance to the st() function which can now use custom strings and file translation when running still without a container, that happens on install for the first steps.
@ParisLiakos, I'll be busy for the rest of the week, so I'll try to reply questions, but please don't wait for me to keep this moving.
Comment #71
ParisLiakos CreditAttribution: ParisLiakos commentedThis should be renamed to TranslationManager to go inline the rest of the core
wouldnt make sense renaming them to appendTranslator and prependTranslator? since the property is name translators..
The remaining format_string logic should be moved as well in the ->translate() method using String::format(), which leaves us with t() being a one line wrapper to the service
I thought we had a container on all steps?
install_begin_request() always create one
Comment #72
ParisLiakos CreditAttribution: ParisLiakos commentedi propose we rename those to translators and move them in a Translator directory. Also rename the interface, since it is actually a translator class not a translation one
So how that works? we have same class registered in two services? i am really confused now.
i have still to figure out why there is no service..if there is no container we should fix that instead. Only place we dont have one is in
DRUPAL_BOOTSTRAP_CONFIGURATION
but t() is never called there, as far as i can seeEdit: in install_verify_database_settings()
Comment #73
ParisLiakos CreditAttribution: ParisLiakos commentedrestoring tags
d.o--
Comment #74
ParisLiakos CreditAttribution: ParisLiakos commentedMade the rename/move to translators.
Added setDefaultLangcode call to the Language request listener.
Moved rest of t() logic in translate()
Moved translate() out of interface
A lot more..
Sorry interdiff to #69 not possible
Comment #76
ParisLiakos CreditAttribution: ParisLiakos commentedi have no clue why those upgrade tests fail
Comment #77
Jose Reyero CreditAttribution: Jose Reyero commentedSome thoughts about #71 and #72:
About naming, ok with any, though I think it should be StringWhatever, to differenciate de concept from EntityTranslation, etc.. (Also the other things are named 'Translation'...)
Encapsulating also the format_string(), I think we better don't, just to keep Translation interface simpler and up to the point, which is translating, not formatting. Anyway, no big deal, we can keep it.
'TranslationChain' was a suggestion in #27, and I think it makes more sense that calling it 'Manager' because it is a plain 'Translator' that implements the TranslationInterface, just like the other 'Translators' so at this point all of them are kind of replaceable. Though ok if you want to rework it a bit to be a real 'TranslationManager' (maybe we could add the format_string and/or the default language handling only there...?)
(But we better keep TranslationChain for usages like 'st' without container and also to use it in configuration translation)
Yes, there are some t() calls (st) before the database is initialized, in the case it fails I think. You can see it if you run the installer manually with no database or cause some other eary failure condition (no settings file, folder not writable, etc..)
And also if we have this fallback we can properly use translation (or simple string overrides) for these error pages.
For locale.services.yml, I think the module one overrides the core one (not sure) but yes, we should just add the LocaleTranslation without replacing (not sure how to add some callbacks to the 'string_translation' in services.yml file)
The reason why upgrade tests fail I think is the locale services being enabled without the module schema not being up to date yet (what happens when you upgrade a localized Drupal).
Maybe the solution for the last two would be using LocaleBundle instead of locale.services.yml so we can add some init checks before adding the LocaleTranslation in.
Comment #78
ParisLiakos CreditAttribution: ParisLiakos commentedyou are right i did not think of that..i ll change it to string_translation..
Thats what i am doing..It also works for st() in installer when there is no container
Yeap, found out my self in install_verify_database_settings()
Thanks for the hints, i ll look into it!
Comment #79
ParisLiakos CreditAttribution: ParisLiakos commentedlets see if this fixes upgrade path
Comment #80
Jose Reyero CreditAttribution: Jose Reyero commentedLatest version looks great to me, you've fixed all the issues nicely.
Only minor issues pending, just some thoughts:
- Naming: Translator/StringTranslator/StringTranslation? I would settle for any maybe let's wait to see what other code reviewers think.
- Just to note: It may be confusing the get_t() replacement (now returns always 't') without the full replacement patch, though it is acceptable as a temporary solution because it proves this whole thing works, while waiting for #1838310: Remove st(), get_t() and $t for good
- We could change the interface's method getTranslation() to getStringTranslation(), in advance of translators being capable of translating other things too, think getPluralTranslation() or getDateTranslation(). Just a wish for now.
- Instead of TranslationManager::setDefaultLanguageCode() we could use the full language object, and setDefaultLanguage(), I think the code would look more consistent with all language handling.
Anyway, this is really ready to be committed as is, because all these DIC issues are really hard to debug by themselves, no need to clutter the patch with more code. So I'd ask for some final code review and then move to RTBC.
Comment #81
ParisLiakos CreditAttribution: ParisLiakos commented+1 for StringTranslator
agreed on both
-------------------------
one more thing i have in mind and would be nice for discussing..might as well be a followup:
I think translators should be plugins:) then no need to register them as a service
Comment #82
Jose Reyero CreditAttribution: Jose Reyero commentedCool!
Just one more thing: There are some weird container conditions handling in language() that may not be needed anymore with this patch, maybe it's time to drop them too (if tests pass after that because I can imagine language() can be invoked from other weird places too --oh, yes, format_plural, ...)
I mean, if it breaks the tests, we can just leave it for a follow up, because it would be another of that bootstrap/DIC issues complex enough by itself....
Comment #83
ParisLiakos CreditAttribution: ParisLiakos commentedInstead of renaming all classes prefixed with String, i just changed the namespace to Drupal\Core\StringTranslation from Drupal\Core\Translation
renamed the service to string_translation
renamed getTranslation to getStringTranslation
removed container conditions in language()
Passing the language object is not possible, since in the early steps of installer we cant use language_load() to get the object from user selected langcode
Test pass locally
Created #1978484: Make translators plugins as a followup
Comment #84
ParisLiakos CreditAttribution: ParisLiakos commentedOk just found https://github.com/symfony/Translation
They have a similar structure. A translator (in patch above translation manager) , translator interface and loaders (in patch above translators).. https://github.com/symfony/Translation/tree/master/Loader
Are we doing this wrong? maybe we should pull this in instead of reinventing the wheel here?
Comment #85
Gábor Hojtsy@ParisLiakos: see #1570346: Let Symfony Translation component handle language messages and http://build2be.com/content/pinch-symfony-d8mi for background on ClemensTolboom's bitter fight to blend us in, and the ultimate failure not due to Clemens not trying hard enough but due both to mismatch in features provided/needed, lack of upstream support, licensing incompatibility, different roadmaps, etc.
Comment #86
Crell CreditAttribution: Crell commentedI don't full grok everything going on here, so I mostly just looked at the central architecture and I think it looks solid. Nice work!
A few notes below:
We probably want to mark this function @deprecated now. (I don't think we can mark t() deprecated, though.)
This seems unimplemented...
Comment #87
YesCT CreditAttribution: YesCT commentedI asked @ParisLiakos in irc if I could take a stab at this. Patch coming.
Comment #88
YesCT CreditAttribution: YesCT commentedI added the @depreciated for function language() [edit: corrected function name]
But it is called. so next coming patch just does the Drupal::service('language_manager')->getLanguage($type) directly in those places, at least in the changed lines from this patch.
Regarding loadLanguage being unimplemented...
grep -R "loadLanguage(" *
core/lib/Drupal/Core/StringTranslation/Translator/CustomStrings.php: protected function loadLanguage($langcode) {
core/lib/Drupal/Core/StringTranslation/Translator/FileTranslation.php: protected function loadLanguage($langcode) {
core/lib/Drupal/Core/StringTranslation/Translator/StaticTranslation.php: $this->translations[$langcode] = $this->loadLanguage($langcode);
core/lib/Drupal/Core/StringTranslation/Translator/StaticTranslation.php: protected function loadLanguage($langcode) {
I think, for example, CustomStrings implements it.. so StaticTranslation is allowing others to implement it.
Does static need more? I'm wondering since there were no failed tests. Maybe some manual testing can show a failure so we can know what kind of test to add?
Comment #89
YesCT CreditAttribution: YesCT commentedhere was the only place I saw language() still being called.
Comment #90
YesCT CreditAttribution: YesCT commentedI'm just guessing here...
it was either this or just something like
Drupal::translation()->setDefaultLangcode(Drupal::service('language_manager')->getLanguage(LANGUAGE_TYPE_INTERFACE)->langcode);
Comment #92
YesCT CreditAttribution: YesCT commented#90: 1813762-locale_translation_interface-90.patch queued for re-testing.
Comment #94
YesCT CreditAttribution: YesCT commentedno interdiff, this is just a new git diff to remove the hunk offset messages to see if that helps.
Comment #95
ParisLiakos CreditAttribution: ParisLiakos commentedbetter store language manager:
And lets move on with this issue, if no other objections
Comment #96
YesCT CreditAttribution: YesCT commentedthe change to save the language_manager
Comment #97
YesCT CreditAttribution: YesCT commentedComment #98
catchDidn't review all the changes in depth but this looks very encouraging to me.
Comment #99
plachSorry if this has already been asked, but would it make sense to statically cache the service object returned by
Drupal::translation()
to save a method invocation for eacht()
call?Comment #100
ParisLiakos CreditAttribution: ParisLiakos commented@plach: i will run some profiling later and do it if it helps
Comment #101
ParisLiakos CreditAttribution: ParisLiakos commentedWe would need a way to update it when kernel is being rebuilt, or if translation service changes in runtime.
But i think this is not needed. we the changes from this patch t() now uses less memory, because it saves some calls to get the default langcode..since the translation manager already holds that in a property:) yay!
here what i got:
4 calls to t() on translateble strings, with two enabled languages:
Before:
Total Incl. MemUse (bytes): 12,632 bytes
Total Incl. PeakMemUse (bytes): 6,056 bytes
Number of Function Calls: 42
After:
Total Incl. MemUse (bytes): 11,888 bytes
Total Incl. PeakMemUse (bytes): 4,968 bytes
Number of Function Calls: 38
We save on call per t() run..which makes some difference when we call it a few hundred times:)
Comment #102
aspilicious CreditAttribution: aspilicious commentedawesome
There is a standard for @deprecated tags somewhere. We don't need to define the alternative if I'm correct. It's already in the code beneath it.
Awesome patch :)
Comment #103
YesCT CreditAttribution: YesCT commented#1876842: [policy, no patch] Use @deprecated to indicate a deprecated method or function
still working out the details it looks like.
Not in
http://drupal.org/node/1354 API documentation and comment standards
yet.
Comment #104
dawehnerReally nice work!
I'm wondering whether it's worth to think about caching the translations here?
What do we do with the default langcode?
So is there an issue to convert it to settings() or something different?
Isn't the signature totally different?
need types.
There should be certainly a comment what this regex does.
Needs doc.
Missing dot at the end.
I'm wondering whether a shorter method name like ->translate() would help once we inject the translation manager into objects.
That's odd, do we maybe have to document that TRUE means that LocaleLookup didn't found it?
It seems to be that this line could get some explanation.
Nice!
Shouldn't we introduce a tag and let a compiler class take care about adding the translators?
good luck :)
Why do we need to set this manually, couldn't that be of the standard container?
Comment #105
ParisLiakos CreditAttribution: ParisLiakos commentedThanks for the review!
we cant cache them in a manager cause translators can be appended in runtime. But translators cache their own translations.
#1975490: Convert locale_custom_strings_* to settings
Translation manager already has a translate() method. I dont think translators need one, since manager will be injected
It is already documented on TranslatorInterface::getStringTranslation()
Thats a lot better idea than turning them to plugins, thanks!
Simpletest should use a minimal container, preferably with no services in it..and each individual test enable the services it needs..with t() though we have to register it initially cause t() and format_plural() calls are all over the place
Comment #106
ParisLiakos CreditAttribution: ParisLiakos commentedtags:) lets see
Comment #107
Jose Reyero CreditAttribution: Jose Reyero commentedI think this looks awesome and I'd just RTBC it if I understood the implications -if any- of adding that new 'string_translator' container tag and that 'RegisterStringTranslatorsPass' that seem to be introduced in the latest version, though missing from the diff (?). So just being cautious with what I don't fully understand.
-- The question is just whether introducing a new compiler pass, which seems to work nicely otherwise, has any other implications... ?
Just to note, for other reviewers:
- The resulting 'unclarified' role of 'get_t() / st() / t()' will be worked out in this follow up so I think the quick docs updating in this patch is ok, as they need to be redefined and re-documented anyway #1838310: Remove st(), get_t() and $t for good
- About custom strings, improving them is not the purpose of this patch and it should be ok just updating the current feature to keep it working, which is what is done atm. (Actually I'd rather see them out of core as it could done by any contrib module after this patch has landed but that's another issue...)
Comment #108
Gábor HojtsyHow was this not on the D8MI sprint yet? :) @jose I think you meant #1838310: Remove st(), get_t() and $t for good for the get_t/st, etc. issue.
Comment #109
Gábor HojtsyComment #110
dawehnerI guess constructors are special, maybe we should just use the typical "Constructs a ..."
Comment #111
Jose Reyero CreditAttribution: Jose Reyero commented@Gábor, right, updated the link in the comment to avoid confusion.
Comment #112
ParisLiakos CreditAttribution: ParisLiakos commentedRerolled and fixed #111
Comment #113
ParisLiakos CreditAttribution: ParisLiakos commentedremoved appendTranslator and prependTranslator in favor of priorities, to be consistent with rest of core
Comment #114
Schnitzel CreditAttribution: Schnitzel commentedMade a Code Review, found only this bit
There is a function locale_translate_english() for this in locale.module :)
And overall this looks really nice!
Comment #115
ParisLiakos CreditAttribution: ParisLiakos commentedah, cool, i wasnt aware we had a function like this:)
here is a patch with it and some other improvements
Comment #117
ParisLiakos CreditAttribution: ParisLiakos commentedyeah, well
locale_translate_english
brings problem during upgrade path tests, because simpletest, as usual is infested with t() calls and locale.module is not loaded yet, so there is a nice undefined function fatal error during testsI dont think i should spend any time fixing this, since this variable will be converted to config anyway (there is a critical opened for this), which will be injected in the locale translation
Comment #118
ParisLiakos CreditAttribution: ParisLiakos commentedopened #1998088: Convert locale_translate_english variable to CMI
Comment #119
attiks CreditAttribution: attiks commentedcomment too long :/
Comment #120
ParisLiakos CreditAttribution: ParisLiakos commentedComment #121
Jose Reyero CreditAttribution: Jose Reyero commentedMy only doubt is about sortTranslators() method. Being this a performance critical funcion, shouldn't we keep the order cached somehow without reordering every time?
Comment #122
ParisLiakos CreditAttribution: ParisLiakos commentedyou are right:)
here it is with an optimization according to #121
Comment #123
Jose Reyero CreditAttribution: Jose Reyero commentedNice! I think this is ready to go.
Comment #124
ParisLiakos CreditAttribution: ParisLiakos commentedReroll for #1620010: Move LANGUAGE constants to the Language class
Comment #126
ParisLiakos CreditAttribution: ParisLiakos commentedah, a use statement..should be ok now
Comment #127
ParisLiakos CreditAttribution: ParisLiakos commentedComment #129
ParisLiakos CreditAttribution: ParisLiakos commentedcopy paste failure when resolving conflicts (sigh)
Comment #130
ParisLiakos CreditAttribution: ParisLiakos commentedback to rtbc
Comment #131
Gábor Hojtsy#129: 1813762-locale_translation_interface-129.patch queued for re-testing.
Comment #132
Schnitzel CreditAttribution: Schnitzel commented+1, really like this, it will definitely break #2004684: Improve Accessibility for String Translation UI but I can reroll it then.
Comment #133
Gábor HojtsyI think both the size and impact of this issue qualifies for a major. Surprised it was not yet marked on that level.
Comment #134
plachAnd we want this in ASAP :)
Comment #135
catchOverall this looks like a good step forward, nice to see t() move to a one-liner.
I didn't do an in-depth review of the whole patch, but these two stuck out as odd for a start:
Why does this only return an array?
Doesn't this just disable caching altogether?
Comment #136
ParisLiakos CreditAttribution: ParisLiakos commentedstraight reroll for now so that i can interdiff
Comment #137
ParisLiakos CreditAttribution: ParisLiakos commentedok, this was tricky, but i believe catch's concerns are addressed
unless bot disagrees ofc
Comment #138
BerdirOnly checked the destruction parts and that looks good. It's ugly but it's the same pattern that we used for the path whitelist cache and we really really need the cache collector...
Comment #139
Jose Reyero CreditAttribution: Jose Reyero commentedJust to note, reply to @catch #135 is in the patch itself
Comment #140
Gábor HojtsyBack to catch then! :)
Comment #141
catchLooks better. The destructable interface stuff is indeed ugly. Berdir could we maybe re-open #1858616: Extract generic CacheCollector implementation and interface from CacheArray to try to get that in asap again?
Committed/pushed to 8.x, thanks!
Comment #142
Gábor HojtsyWoot, thanks! @reyero, @ParisLiakos, great job! Now onto #1838310: Remove st(), get_t() and $t for good? (and #1966538: Translation is not updated for configuration, when the config changes) - both of which were dependent on this landing!
Comment #143
jibranThis is screaming for change notification.
Comment #144
YesCT CreditAttribution: YesCT commentedhttps://drupal.org/contributor-tasks/draft-change-notification
is instructions for drafting a change notice.
Anyone can try the first draft. :)
Just make a comment saying you are going to try, and ask questions if you have any.
Comment #145
ParisLiakos CreditAttribution: ParisLiakos commentedChange notice: https://drupal.org/node/2016569
Comment #146
Crell CreditAttribution: Crell commentedI made a few small grammar tweaks, nothing major.
"(lower priority means your translator will fire first)" - This is false. Priority is opposite from weight, so low priority fires last. If the actual code that went in is using it as weight and that parenthetical is correct, that's a bug that needs to be fixed.
Also, there's code samples in the change notice for the locale storage but not the actual translation itself. That's far more useful. "What am I supposed to do now instead of calling t()" is the main question that should be answered by a code sample. (99.3% of people won't ever change the locale storage, but everyone translates strings.)
Comment #147
Gábor HojtsyYeah, well, if we want to have people work on an injected t()-like service without calling t() that sounds like in dire need of a pattern defined before people jump on the opportunity and make out all kinds of fun things, or we'll not be able to extract the translatable strings.
Comment #148
ParisLiakos CreditAttribution: ParisLiakos commentedbug followup:
#2018409: LocaleTranslation should have its translation property initialized as array
about #146,#147. while it is possible, lets also figure a nicer way with better DX than doing
$this->translation->translate()
instead oft()
before putting it in the change notice, or people will start tweeeting all kind of things for D8 againi opened #2018411: Figure out a nice DX when working with injected translation for that
Comment #149
Gábor HojtsyI see the change log fixed the priority and #2018411: Figure out a nice DX when working with injected translation is open for refining the DX. So looks like the change notice is good for this one.
Comment #150.0
(not verified) CreditAttribution: commentedFix links.