Problem/motivation
Drupal 8 has configuration schemas to describe the structure of configuration. This was proposed in part to resolve regressions and keep content types, menu items, etc. translatable. The schemas describe the structure of configuration but not yet which items are translatable and there is no connection with the software translation system (locale module) in core to use that information to make shipped configuration translatable. This solves regressions making shipped settings like email texts, content types, etc. translatable again.
Proposed solution
Patches at #1648930: Introduce configuration schema and use for translation all included this integration but that was postponed on other attempts and #1866610: Introduce Kwalify-inspired schema format for configuration got committed after all, which was more minimalistic. So we need to resurrect all the previously written code and adapt to the current schema system. Take things removed in progress in #1866610: Introduce Kwalify-inspired schema format for configuration and the feature set delta between that and #1648930: Introduce configuration schema and use for translation and make it work with the committed schema system.
Changed user interfaces
The only user interface change is a new step is added in the installer to import configuration translations. Module enable and disable batches will last a bit longer because the configuration translation update steps are included in the existing batches.
API changes
Configuration schema elements get a 'translatable' boolean property, and the text and label types are marked true. Core already uses this guideline to use text and label types for UI facing text elements that are to be made translatable, so this is just codifying an existing pattern.
API additions
Drupal\locale\LocaleConfigManager is added to wrap shipped configuration and access with translations. locale_config() is added in locale.module to access an instance of that. Drupal\locale\LocaleTypedConfig is added to wrap each config Element.
Steps to test #
1. Apply patch.
2. When installing Drupal and enabling modules(Language, Interface Translation), strings from the configuration of said modules will show up under Admin > Configuration > User interface translation.
3. Search for strings there like the shipped user role names ("Authenticated user"), the default contact category ("Website feedback"), block placement titles ("User login", "Footer menu"), content type names and descriptions ("Basic page", "Use basic pages for your static content, such as an 'About us' page.") etc.
4. Translate them on this user interface (some/all of them may already be translated based on the .po files imported from localize.drupal.org, feel free to modify the translations for testing too).
5. Watch as the translation is being used when you switch language on the site (on the block, add content page, contact page, user editing screen in a foreign language, etc). (The contact category title will show on the dedicated URL of the contact category, eg. /contact/feedback for the default one, it does not show on the main /contact page).
Remaining tasks
- Needs more manual testing and code reviews, then done :) (The patch is functionally complete, tests are included and expanded. )
- figure out if we want to keep implementing Drupal\Core\TypedData\TranslatableInterface, which is what makes the getTranslationLanguages() weird code and the strict mode in getTranslation() required. (see #153)
Followups
#1966538: Translation is not updated for configuration, when the config changes
#2029075: Configuration translation step in the installation takes a reeeeaaallly long time when installing in a non-English language
Comment | File | Size | Author |
---|---|---|---|
#154 | interdiff.txt | 7.76 KB | Gábor Hojtsy |
#154 | 1905152-metadata-locale-integration-154.patch | 55.46 KB | Gábor Hojtsy |
#153 | interdiff.txt | 6.95 KB | Gábor Hojtsy |
#153 | 1905152-metadata-locale-integration-153.patch | 56.9 KB | Gábor Hojtsy |
#151 | 1905152-metadata-locale-integration-151.patch | 58.57 KB | vijaycs85 |
Comments
Comment #1
Gábor HojtsyBring over some tags from #1648930: Introduce configuration schema and use for translation since this affects missing features in VDC and the config system somewhat.
Comment #2
Gábor HojtsyRestoring the translatable key from #1866610: Introduce Kwalify-inspired schema format for configuration (#13). This is far from doing the scope of the issue. Also I think this did not have any test coverage, but we should get that as part of the locale integration.
Comment #4
Gábor Hojtsy#2: metadata-locale-integration-2.patch queued for re-testing.
Comment #6
plach#2: metadata-locale-integration-2.patch queued for re-testing.
Comment #8
Gábor HojtsyAlso a quick extract of the locale related stuff from #1648930: Introduce configuration schema and use for translation. Obviously almost none of that would apply now so just a txt for posterity, while we figure out where should things go.
Comment #9
Gábor Hojtsy#2: metadata-locale-integration-2.patch queued for re-testing.
Comment #10
Gábor HojtsyMerged in all of those except the getLanguage() from TypedConfig(), not sure where that would go in the new system. Most things will not pass yet due to the straight merge using the old metadata stuff :D Need to work those out.
Comment #12
Gábor HojtsyRestoring changes in InstallerStorage too for the locale code. The LocaleTypedConfig will need a lot more thinking since it is based on the TypedConfig one by one CMI key accessor idea from #1648930: Introduce configuration schema and use for translation. The current metadata system uses a factory for so it needs to be reimagined on the locale side.
Comment #14
Gábor HojtsyMade minimal changes in the LocaleTypedConfig class to reflect the now missing underlying structure; it still behaves as a Config-alike solution like before.
Comment #16
Gábor HojtsyAll right, needed to restore most of the hunks from #1884182: Import of large number of translations fails on SQLite (except the last hunk that actually caused the SQLite issue), since tracking the individual strings is needed for us to be able to look up exactly which CMI items to update. Without this tracking, we'd need to update all of the CMI translation files, which sounds like would be a bad idea.
The test fails properly pointed at this problem BTW :)
Comment #18
Gábor HojtsyPatch only applied with various offsets. Rerolling. Looking at remaining fails.
Comment #19
Gábor HojtsySo the failures are all down to Fatal error: Call to a member function read() on a non-object in core/lib/Drupal/Core/Config/Config.php on line 416 which is $data = $this->storage->read($this->name); in Config. That is due to LocaleTypedConfig extending Config but not actually implementing it much. Looks like I bugosly tracked down that it was intended to be coming from Config. I could not track down the full thinking behind the original version.
In http://drupal.org/node/1648930#comment-6826770 - TypedConfig extends Config and LocaleTypedConfig extends TypedConfig which is where I took extending LocaleTypedConfig direct from Config here (since we don't have TypedConfig as a wrapper or any similar solution in core with the kwalify based approach).
Will need to spend more time to track this down.
Comment #21
Gábor HojtsyAll right, figured out we do need the injectability of $data for any CMI key since for the translation itself, we need only the original values, so the CMI-sourced data needs to be filtered before it is passed in. So LocaleTypedConfig is not really suitable as an extension to Config or TypedConfigManager for that matter. It can use config_typed() (TypedConfigManager) to dress up data with type information though.
So added a bare-data returning getData() method and a get() which returns typed data wrapped data. This mapped exactly into how this class is already used so just filling in holes. Also found out that system.site now only has 6 elements vs. the 7 there were at the time of the original test being written. Also figured out Config Element has no relation to most typed data elements and isTranslatable with a CMI condition would not make sense on most Typed Data elements, so put back the direct definition based logic similar to what it was before.
This will still fail since LocaleTypedConfig's get() will not return a reusable Config instance like some places assume it does. I did not find a clear cut way right away to make this work nicely.
Also I'm just trying to fill holes while trying to figure out the logic behind the code, so some solutions might be stop-gaps only. However, this version is now fully capable of actually translating a site name and an image style name, etc. as is mostly proven by the test :)
Comment #23
Gábor HojtsyTagging for config schema as well as keeping on sprint :)
Comment #24
Gábor HojtsyBTW #1914366: Move all configuration schema files into a schema subdirectory uses the same patch portion for InstallStorage to be able to read .yml files from other places under components, not just the regular config directory. Hopefully we can get that committed soon which would make this patch smaller :) That should not mean no work should be done here.
Comment #25
vijaycs85Re-rolling excluding InstallStorage changes...
Comment #27
Jose Reyero CreditAttribution: Jose Reyero commentedI'll be trying to update this one, possibly dropping or simplifying the LocaleTypedConfig object which doesn't make that much sense anymore.
Comment #28
Gábor Hojtsy#1763640: Introduce config context to make original config and different overrides accessible got committed, so the system is all there to welcome these files and actually use them :)
Comment #29
vijaycs85#25: 1905152-metadata-locale-integration-25.patch queued for re-testing.
Comment #31
Jose Reyero CreditAttribution: Jose Reyero commentedNew patch, lots of changes, not there yet though.
Main changes:
- Reworked LocaleTypedConfig, now it is a wrapper for Config\Schema\Element.
- Added LocaleConfigManager as a new service that encapsulates most of the funcionality:
- It is in the DIC, can be accesses with locale_config().
- Extends TypedConfigManager so it can get TypedConfig objects with default configuration data (instead of current configuration).
- Provides most of the API that was previously in locale.bulk.inc. See methods compareConfigData(), saveConfigData(), getComponentNames(), etc. This allows us to reuse the same config storage objects for all of them.
Still to do:
- Test partially updated but not passing yet.
- Update strings after a bulk translation import.
- Update locale config overrides after any configuration is updated.
Anyway I thought I would share this one to get feedback on the new LocaleConfigManager approach.
Comment #32
Jose Reyero CreditAttribution: Jose Reyero commentedDone some more improvements, cleanup, fixed tests.
- Moved translateString() method into LocaleConfigManager
- Simplified LocaleTypedConfig wrapper, now extending Element
Still working on:
- Update strings after a bulk translation import.
- Update locale config overrides after any configuration is updated.
Comment #34
Gábor HojtsyFirst off, good stuff. I like how the Locale typed data manager just extends the config typed data manager and how the locale element extends the config element. That should ensure we have added features but also use the same features. That is config > typed config > localized typed config would be layers on top of each other cleanly. That said, no complaints on the approach at all!
So most of my comments are around docs missing or incorrectly provided.
Why no argument for locale storage? Not in the DIC? I see the constructor defaults to locale_storage(), but it looks odd only this has a fallback value.
Manages localized configuration type plugins. No?
Add to the start: "(optional)". Add to the end: "Defaults to locale_storage()".
Note is not really true. Or at least not to the immediately following line.
"Saves ...".
Also would be great to document these arguments. Eg. $data would be only the translated values for example.
Also, would it make sense to name these more concretely, like saveTranslationData? Since the manager class inherits from typed data manager, it could look odd that we have this generic method name but its not really dealing with typed data, it is specifically for translations.
Copy-paste first line :) Also same for param documentation and method naming.
Missing .
Maybe name this deleteComponentTranslations? (similar to above). It is not really deleting any components, it reacts to removal of components from the live system.
I like how the naming scheme makes this function easy :) Have the same comment on the naming as above.
It would be great to document what you do with this.
\Drupal
Remove?
The documentation and the actual arguments don't match up at all.
Are these all custom to this class, not defined on any interface? It looks strange that $element on the three methods use entirely different type hints. That is pretty odd. Also inline type hints in the code would be good.
I like the translation criteria is documented on translateElement, but $options is very cryptic at all places. We should find a place to document what can be in there. "dependent on the translator" is pretty obscure. What kind of translators there are? How can one figure out?
Eg. strict and non-strict mode are useful options to document for translation access.
Remove commented out lines? The last line seems to be the current API, right?
Same, commented out code can be removed. I don't understand the first line of this comment though...
Is that to mean we should have tests for entity_load_multiple() as to whether loading config entities through that would apply language overrides? That is pre the scope of this issue IMHO, not really related to what we are doing here? Regardless, entity loading should also equally work with the page negotiated language that is.
No 'profile' component?
... in later steps?
indication of progress.
Updates
No 'profile' component?
\Drupal
Comment #35
Gábor HojtsyElevating now that #1616594: META: Implement multilingual CMI is marked duplicate.
Comment #36
Jose Reyero CreditAttribution: Jose Reyero commentedFixed multiple issues:
- Added back refreshing after manual translation update, that was breaking tests before.
- Fixed all issues pointed out by Gábor in #34, some replies below
- Added back the full feature of updating config translations after importing a translation file. To prevent issues like the previous bug caused by this features, js and config translations are refreshed on multiple steps, 100 strings at a time, see #1884182: Import of large number of translations fails on SQLite
I think the only missing features is refreshing the locale configuration after configuration updates from system forms.
And maybe one more test to check config translation updates after a po import.
About #34 agree with all, only two notes:
> Why no argument for locale storage? Not in the DIC? I see the constructor defaults to locale_storage(), but it looks odd only this has a fallback value.
Yup, not yet in DIC, that should be a different patch and then it would be trivial to add it as a parameter here.
> No 'profile' component?
Profile components are translated only on install or when retranslating all configuration so configuration names for these will be returned by InstallStorage::listAll() but 'profiles' never need to be passed around when updating only specific components, that should be only 'modules' and 'themes' when enabled / disabled.
This fact may need some explanation somewhere, just I don't know where it would make sense.
Back to needs review.
Comment #37
Gábor HojtsyThanks for the update! I might not be able to review this for a few days due to other obligations, but will get to this sometime this week or early next week latest. Other reviewers would be great of course :)
Comment #38
aspilicious CreditAttribution: aspilicious commentedSaves
Is this still true? I can't find any checks for english in the code.
Builds
Performs
Comment #39
Gábor HojtsyAs for locale storage not in the DIC, ok.
But profiles will behave as modules on an installed site, right? How is that solved in this setup?
Comment #40
Gábor HojtsyBTW looking through the interdiff, all changes look good.
Comment #41
Gábor HojtsyAlso tried the patch manually. First off, it works wonderfully. I got locale.config.hu files created for my Hungarian translations in the installer, eg. locale.config.hu.system.maintenance.yml, locale.config.hu.user.mail.yml, locale.config.hu.views.view.archive.yml, etc. Updating translations from locale's UI worked. :) Wonderful!
This functionality has almost no UI, but the little it has can be improved a bit I think. The installer spawns too many tasks for language stuff when you go to install in a foreign language in my view. Can we piggy-back the configuration translation update on the existing second translation import wizard step / batch? There are already two of those, and this patch adds one more:
It would also be great to present a percentage complete in the config batch instead of a number / total like it is now IMHO.
As for the two missing items:
Do you want to tie this to forms? Or rather config save events? (I'd say events, that is more API friendly :). I assume the translation files would be re-generated based on locale db translations when the config files are re-saved. (The contrib config translation module will need a similar solution when config files are saved, although it does not have the luxury of a db backend where the strings can be pulled from anytime to regenerate files in different structures). Do you want to target this for the current patch before commit? (It is true that before this patch, there was nobody managing these files).
That would be great too yup :)
Stopped reviewing. Looking forward to the updates! I think this is shaping up to be very nice.
Comment #42
YesCT CreditAttribution: YesCT commentedI had some awesome explanations in dreditor, but then I lost them. Sad.
So, in general:
most of this was docs and standards review, based on:
http://drupal.org/node/1354
http://drupal.org/coding-standards
http://drupal.org/node/1353118 (namespaces)
http://drupal.org/node/325974 (simple test)
#1158720: [policy, no patch] Add parameter type hinting to function declaration coding standards
Comment #44
YesCT CreditAttribution: YesCT commentedI didn't read through all the code in detail.
The part I did spend some time on was this:
The comments I changed here, need to be checked if they are accurate.
Also, $name confused me. I think it's what is referred to in the code as the location.
Is that the hash of the active config, or something else?
Comment #45
YesCT CreditAttribution: YesCT commentedoops.
Comment #46
YesCT CreditAttribution: YesCT commentedI could not see a reason to check just $source. So I added checking the rest of the arguments too.
Comment #47
YesCT CreditAttribution: YesCT commentedfrom irc with @Gábor Hojtsy
Some of this might be good in the issue summary.
tagging for needs manual testing http://drupal.org/node/1489010
updating the issue summary (aka issue summary initiative). http://drupal.org/node/1155816
---
this exposes default (shipped) config via the locale module, so you can translate pieces of default config there
anonymous user name, user email subjects and bodies from account settings, etc.
some views stuff as well as much as we have schema for that now
it works but some more testing helps :)
well, it creates the locale.config.* files in the installer from the .po file imported earlier
and it also lets you modify the translations via locale
the interface translation UI
for SHIPPED configuration files
(not for ones you create)
also if you slightly edit the config, it will still translate the rest of the pieces
since it identifies translatables key by key :)
its a very important missing piece and it in fact works
Comment #49
YesCT CreditAttribution: YesCT commentedsome fixes for type hinting.
Comment #50
YesCT CreditAttribution: YesCT commentedI'm wondering if should change the $default one similarly, since it comes from a ->read too
http://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Config%21S...
Do we need some logic here for when one of them is false? Like another speedup?
note that gets called from
Comment #51
YesCT CreditAttribution: YesCT commentedmaybe that should be \Traversable instead of mixed? and then \Traversable in the type hint in the function definition.
Comment #53
YesCT CreditAttribution: YesCT commentedputting this back to see if it does the trick.
Comment #54
YesCT CreditAttribution: YesCT commentedrunning locally too. but could be faster on the testbot.
Comment #56
YesCT CreditAttribution: YesCT commentedpassed locally. retesting.
Comment #57
YesCT CreditAttribution: YesCT commented#54: 1905152-metadata-locale-integration-54.patch queued for re-testing.
Comment #59
YesCT CreditAttribution: YesCT commentedfrom 9 to 1 fail... but that fail passes locally.
Comment #60
YesCT CreditAttribution: YesCT commented#54: 1905152-metadata-locale-integration-54.patch queued for re-testing.
Comment #62
YesCT CreditAttribution: YesCT commenteda interdiff to @Jose Reyero 's last patch (#36)
Comment #63
Gábor HojtsyI think the fail is caused by the French translations actually being downloaded and applied to configuration (which is the goal of this patch :), while in the ConfigLocaleOverrideWebTest, we use system.site config overrides from config_test module. Those overrides are written over when the French overrides are written over when this functionality saves the overrides based on the French translation, so the tests will not pass. Writing the overrides over again after adding French solves the problem (and allows us to remove the locale.config.fr.system.site.yml from the config_test module which is not done in this addendum). I think a better fix would be to use an xx language code like most other tests to avoid the lengthy download/import cycle happening in the test (speed it up) and to make sure our overrides stick.
I don't know why this fail did not occur with Jose's patch, but it seems to be the side-effect of the goal of this issue in general.
Comment #64
Gábor HojtsyAlso it did not fail for Jose, because that new test was committed 2 days ago (http://drupalcode.org/project/drupal.git/commitdiff/dd927b00083c96dd4255...) and Jose's patch was posted 8 days earlier than that.
Comment #65
YesCT CreditAttribution: YesCT commentedlocally, my test is still not ending...
using a custom language instead of french.
Comment #66
Gábor Hojtsy@Berdir points to #1887480: Automated download of translations when adding languages/enabling modules can not be disabled which proposes a way to disable the remote download for tests. I fully agree with that however the tests introduced here do not rely on the remote download either (plenty of other ways to test the functionality), so it is not required for these to work.
Comment #67
Gábor HojtsyThe patch in 65 contains some cruft from unrelated things. I also ran the test locally on a modified version of 54 based on the interdiff from 65 and figured it still fails. The reason for that is this patch is essentially making locale the truth, the canonical source of translation string information for translatable config strings. That is intended, since otherwise how would it be confidently rewriting files when new translation updates are downloaded? Other modules will need to work with this, such as I opened this issue 2 weeks ago for config_translation UI that will make it compatible with this behaviour: #1936208: If config is translated from shipped config or partially edited, also push translations to locale storage.
So regardless of whether we add a real language like fr or a made up one like xx, since the truth comes from locale, the override file from config_test will not survive past adding the language (which is when locale refreshes config override files, and will remove the file we shipped for the test). There are two options, either we add the translation on the locale UI or we write the config override direct from the test *after* we add the language, so it will be effective. Since we are not interested in the longevity of the override here past the test, I think writing the config direct is fine.
This should fully pass now.
Comment #68
YesCT CreditAttribution: YesCT commentedalso, #1936208: If config is translated from shipped config or partially edited, also push translations to locale storage related to the change from .yml to save() and
merging translations local/remote for example when the community updates translations
and
controlling which translations of your own can be overwritten on a string by string basis
[making a note of something said in irc so I can sort it out later]
Comment #69
Gábor HojtsyUpdate issue title for better explanation of the feature. I'll look into updating issue summary tomorrow.
Comment #70
YesCT CreditAttribution: YesCT commentedalso, notes from irc:
need some more test stuff
import a .po file with translations and see how it applies
it currently only tests through the translation UI
(this might be both looking for more reviews and needing work to add the tests. :) )
Comment #71
Gábor Hojtsy#67: 1905152-metadata-locale-integration-67.patch queued for re-testing.
Comment #72
Gábor HojtsyAdded tests for config translation updates after a .po import. I extended the existing import tests, since those have utility functions to import .po files already. The missing feature of changed config needing to update override files is still missing. That was the only remaining concern that was brought up above and not yet resolved.
Comment #73
Gábor HojtsyHere is a first stab at the config update => config translation update patch. Extending the config event listener in locale, we check if the saved config is among locations identified for configuration (a file in shipped config originally), and update the translation file based on locale backend data. Test coverage also added. It does not pass yet, so the implementation is certainly not entirely correct. Will debug this more, but wanted to post as an interim step to avoid extra parallel work from others just in case...
Comment #75
Gábor HojtsySolved most test issues by making sure that the location check is not run if the locale database is not yet installed. There may be better ways to check this, but this was certainly one :) Also fixed the actual override bug with better checking for location, so now it actually updates the override in the locale.config.* space as designed. There are still in-page caches which don't make our in-page assertions in the test pass. I made cache clearing effective at two places, the locale config manager clears its internal cache for a config key, when it is saved and all override listeners re-set their overrides when config is saved (the config clears prior overrides out). This still does not make the strict translation override retrieval work in-request yet, so there is likely some other static in-page cache that is not yet cleared.
@reyero: can you help here? Looks like this is very close otherwise. Thanks!
Comment #77
Gábor HojtsyI drastically updated the issue summary including steps to test, api changes/additions and remaining tasks. Please help do manual testing. Don't be shy testing this manually, the fails are not indicative of this not working :D
Comment #78
vijaycs85#75: 1905152-metadata-locale-integration-75.patch queued for re-testing.
Comment #79.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #79.1
vijaycs85Updated issue summary - link to steps
Comment #79.2
vijaycs85Updated issue summary - link to steps
Comment #80
Jose Reyero CreditAttribution: Jose Reyero commentedIt looks to me like the main issue is DIC persistent components. When a module is disabled, the container is rebuilt, but LocaleConfigListener is attached to a service (ConfigContextFactory) that persists across container rebuilds, thus it is still loaded and called on subsequent config calls, even if the module is already disabled.
(I didn't know before about these 'Services to Persist' so... wondering whether thisy makes any sense...?)
Anyway, I'll try to see where in the code makes sense to fix this and post a patch.
Comment #81
Gábor HojtsyYay, thanks!
Comment #82
Jose Reyero CreditAttribution: Jose Reyero commentedHere's a first patch, that should fix many of the test errors. It takes care of unregistering the module's own event listeners when the module is disabled, to fix the issue explained in #80
Just for the record, this looks like a potential issue for any other modules defining event listeners too, so maybe this should be better fixed at the DIC level. But for now, we need to move on with this patch.
(Moving to 'needs review' to trigger testbot. I will be doing some more cleanup in this patch though)
Comment #84
Jose Reyero CreditAttribution: Jose Reyero commentedWell, the bad news is we didn't get many more tests passing; the good news is failing tests are a bit different, some tests not passing before are passing now, but some other tests failing.
We can get some more passing by adding back the 'installed schema version' check, though it seems it doesn't really get us much further. There are, again, interesting race conditions when installing / uninstalling modules. And one of them is the container being rebuilt, and the module bundle being added (so module's event listeners kicking in), before the module is actually installed. Similar things happen with the upgrade tests: locale module enabled, thus event listeners added, but the module schema not yet upgraded.
The problem is there are a lot of configuration updates from the point the module bundle is added till the point the module's schema is installed, and even then, there are some before the module is actually loaded, and I can see still error happening after the schema is installed because the configuration may get updated before the module is loaded.
We need to work out a better solution for this than just checking the module's schema version:
- Besides it being ugly, needing to check for the schema every time, this is not always enough, we'd need to check whether the module is actually loaded.
- The event listener relying on the module API is also a problem, it should depend on the container only (because it is actually there before the module is loaded) for what we need to add some more parameters here.
- It depending on locale storage is also a problem because it is not in the container, but created with locale_storage(), we may need to fix this one first of all.
- There are some other weird errors caused by check_requirements, since the system module updating the file system and the 'system.file' configuration triggers configuration updates too.
- Overall, the configuration system doesn't provide enough hooks and needing to implement an event listener to react on configuration updates is not the better solution either because it causes a lot of circular dependencies.
In short, we may need to do a lot of minor improvements, one at a time, to fix all these issues, but we'll be able to measure progress by the number of tests that are passing.
Comment #85
Gábor HojtsyGood investigative work, sad for the race conditions.
Comment #86
Jose Reyero CreditAttribution: Jose Reyero commentedI forgot to mention, we should fix first this one #1813762: Introduce unified interfaces, use dependency injection for interface translation
That will move 'locale storage' fully into the DIC and would fix some of the issues and allow some other improvements on this one.
Comment #87
Gábor HojtsyI'm afraid if we fix the other one first, that could linger for quite a while. We have an API freeze to hit in 2.5 months. :/ So if we need the change tracking to have that, then maybe we want to make the scope smaller here, not deal with change following yet given we have a complete well working solution for that scope (it passes tests above :). And then have the change tracking be a followup dependent on #1813762: Introduce unified interfaces, use dependency injection for interface translation. What do you think?
That would allow us to push forward with the config_translation integration (which is also proposed for core even it might not have a chance :D). See #1952394: Add configuration translation user interface module in core (core issue) and #1936208: If config is translated from shipped config or partially edited, also push translations to locale storage and #1936186: Make use of locale API when it becomes available in the config translation queue. We'll take care of these issues there, however they are postponed on the basics of this issue :)
Comment #88
vijaycs85Straight re-roll of #82 after 34e6309 commit.
Changes are:
1. CoreBundle.php
from:
to
2.localBundle.php
from
to
Comment #89
YesCT CreditAttribution: YesCT commentedComment #91
Gábor HojtsyHere is a reroll of #72 instead, which was the pre-change tracking state all passing :) The services were moved to services.yml files, so those needed the same changes explained in #88 but otherwise applied. So as I said above, what about accepting this as the scope, since its already a huge step ahead, lots of added tests, etc. and do the change tracking in a followup, so we can parallelise getting reviews on this with Jose working on #1813762: Introduce unified interfaces, use dependency injection for interface translation. :) I'll try to get reviewers on this as soon as its green :)
Comment #93
Jose Reyero CreditAttribution: Jose Reyero commented@Gábor, I think this (#92) is a great idea, since the other patch is going to take a while.
Comment #94
Gábor HojtsySuperb, and that only has one fail (for some reason) in a new test we added, so should be easier to contain :)
Comment #95
Jose Reyero CreditAttribution: Jose Reyero commentedRerolled the patch fixing the only test fail (due to config file with added properties).
Comment #95.0
Jose Reyero CreditAttribution: Jose Reyero commentedUpdated issue summary.
Comment #96
Gábor HojtsyAdded #1966538: Translation is not updated for configuration, when the config changes as a followup for the change tracking. Marked postponed on this one and the t() DIC issue. Updated issue summary. Now needs manual testing and reviews :)
Comment #96.0
Gábor HojtsyUpdate for new scope.
Comment #96.1
Gábor HojtsyExpand remaining tasks.
Comment #96.2
Gábor HojtsyAdd actual examples
Comment #96.3
Gábor HojtsyMore fun for testing :)
Comment #98
vijaycs85Applied patch in #95 and followed steps in summary and seems it is working as expected.
Screenshots of steps:
Step 2:
Step 3:
Step 4:
Comment #99
Gábor HojtsySuperb, any code complaints? :)
Comment #100
vijaycs85Minor: Can't we have test .po file?
Doesn't look like added classes used here?
Minor: Double space after '='
Minor: space issue in array.
Minor: array format has more than one space in left.
Attached patch fixes few spacing issues.
Comment #101
Gábor HojtsyRe the .po file, the other tests also use an inline .po file that is saved out and imported from there. I'd not reform that here, just use what the rest does. Re the spacing fixes and the unused classes, can you include them as fixes too? (I don't think those were in the attached patch?).
As for the diff, most space fixes are good, these two look odd (the first two):
This does not seem to be an improvement? The old one is correct, no?
Put the first line on its own line then too, indent with the same, no?
Comment #102
vijaycs85Thanks @Gábor Hojtsy. Updated wrong intention on comments and removed unused classes.
Comment #103
Gábor HojtsyThis does not seem to be an improvement with =>' (no space there), the space there should definitely not be lost.
Comment #104
penyaskitoThe langcode property somehow gets dropped, so the count should be 6. Not sure if this is the desired behavior.
Also fixed the space nitpick.
Comment #105
vijaycs85Oh yay!!! It's green... Thanks @penyaskito.
Comment #106
penyaskitoOne of the values shown was 'en', so the one missing should be another one: candidates are slogan or email, but I couldn't check which one of both.
Also the langcode is a special case and I wouldn't expect 'en' but the localized value 'xx', not sure of the implications.
Comment #107
Gábor HojtsyOk, well, so the missing key is the email address. Which is pretty intriguing. It does show up with config inspector (as in the typed data wrapper for config), but it does not appear with the locale wrapper. I think this is likely indicative of a bug...
Keys returned in $properties for non-strict locale wrapper lookup:
My active store file:
Comment #108
Gábor HojtsyAll right, I dug into this a lot more. So everything works well as supposed to :D The locale typed config wrapper is supposed to return items which are identical to the original configuration. See the get() method in LocaleConfigManager that this stuff is based on. In simpletest, the original vs. the installed config difference looks like this (from debugging output in my test debug sesssion):
Original/shipped config:
Simpletest installed config:
So simpletest does not change the site name or the front page (with the minimal profile used for testing), only the site email is updated. Given that that is different from the original config, it will not show up in the locale wrapper which is designed to recognise only the default values so it can pick the translatable pieces from there.
That said, all works well, adding more comments in the patch to make it easier to understand for future reviewers.
Any more reviews/concerns? :)
Comment #109
Gábor HojtsyI also installed fresh with the patch and the following config override files were installed:
Samples of those files:
locale.config.hu.system.maintenance.yml
locale.config.hu.views.view.archive.yml
locale.config.hu.user.mail.yml
All look good and correct. Also tested updating/changing translations. When I updated the "Page" translation to "Weboldal" from "Oldal", the override file was properly upated:
locale.config.hu.views.view.archive.yml after hand-edited translation.
Comment #111
Gábor Hojtsy#108: 1905152-metadata-locale-integration-108.patch queued for re-testing.
Comment #112
Gábor HojtsyThe prior failure was due to core checkout fails, not related to this patch. This got code style fixups from vijaycs85, two people manually testing, good test coverage and me cross-checking the code from @reyero, so I think it looks good to go! Woo, let's get this in!
Comment #113
YesCT CreditAttribution: YesCT commentedComment #114
YesCT CreditAttribution: YesCT commentedoops. tagged wrong issue. this summary is great.
Comment #115
alexpottLet's create a public static function to do this... so that we can call
LocaleConfigManager::localeConfigName($langcode, $name);
from outside this class (i.e. the event listener) and then we have one place to change the filename.Why do we need to do this?
Maybe the public static idea is bad... but actually... if $name = null and you return this then maybe... dunno
the assignment to $translation is unnecessary... could be...
As FALSE is returned by default for this function.
Should use the new {@inheritdoc}
This looks a bit mad... at least needs a comment as to why we need add or remove depending on the $include_default param... ie why does locale_translatable_language_list() sometimes contain the default language and sometimes not...
if getElementTranslation returned an empty array the ternary on the return would not be needed.
However, If are going to return NULL we need to document it.
This looks weird... because translateElement() returns FALSE only if !empty($options['strict']) ... so I think the OR is unnecessary.
Additionally,
if ($translation || empty($options['strict'])) {
I think the|| empty($options['strict'])
is unnecessary... could just byif (!empty($translation)) {
From the function name I would expect this function to return the element... and throw an exception if it can't... the logic looks like at least one level of nesting could be removed.
... also need to document why we have the strict option... i.e. why does the FALSE exist.
Got tired at this point... will review tests and batch stuff tomorrow... therefore changing back to needs review.
Comment #116
Gábor Hojtsy1. TODO: static method for CMI key: agreed
2. TODO: array_filter(): I don't know and did not find an apparent reason, @reyero should now
3. DISCUSSION: $translations FALSE assignment: I'm not sure your repeated 4 levels of nested arrays is better vs. saving it in a temp variable; the loosing of the ternary I agree with
4. TODO: inheritdoc: ok, I looked up http://drupal.org/node/1354#inheritdoc is how to do it
5. DEBATED: getTranslationLanguages() looking mad, that is how TranslatableInterface in TypedData dictates we implement this as far as I see
6. DISCUSSION: for getElementTranslation() to return an empty array: I believe it would return NULL if in strict mode and there was no translation; otherwise it would return an array; I'm not sure if we need/want to distinguish strict mode results from non-strict results
7. TODO: on why non-strict mode exists; the point of this patch is the strict mode; so the non-strict pass filters out changed config and the strict mode filters out config that is non-translatable (due to its type) or does not have translation (in locale's db); so strict mode results in only the config that **is translatable and is translated** in the nested structure of the config file; now it seems like strict mode is the only mode used outside of tests (see locale_config_update_multiple()), so if we consider only the needs of this patch, we can get rid of non-strict mode; I agree non-strict mode is a bit strange since it filters out changed things but not non-translatable things; if it would filter out non-translatable things as well, that may be more interesting however still no use in the direct use case of this patch as-is
8. DEBATED: translateElement() actually in-place translating an element vs. returning it translated; we can change the return value; I'm not sure about throwing an exception; most config entries in a config files are not translatable, so throwing an exception for each of them sounds very expensive as we go through attempting to translate each; this method is called for each element individually when assembling the translation
Note that I don't have time right now to actually turn the items I agree with into the patch, might not have until early next week :/
Comment #117
Gábor HojtsyMoved the whitespace only changes from this patch to #1968446: Whitespace fixes in language installer and config install storage. The only change in this patch from #108 is I cut out those. Keeping at needs review for at least testbot :)
Comment #119
vijaycs85Thanks for the review @alexpott and @Gábor Hojtsy. Tried few of TODO in your reviews comments here:
#116.1.
LocaleConfigManager::localeConfigName()
- Implemented.#116.2. array_filter() - by default we would have $components['module'] and $components['theme'] without checking to include or not. So this filter will strip out empty array.
#116.4. {@inheritdoc} - Fixed.
in #115:
- Fixed.
- Fixed
- Fixed.
- Fixed.
TODO:
1.
2.
Comment #120
Jose Reyero CreditAttribution: Jose Reyero commentedSome comments on #115 #116:
That should work. Though my preferred way for this to work would be having some 'locale.config.storage' in the DIC, kind of:
The aim would be to make the whole thing replaceable, if in the future we decide to store this data somewhere else.
Anyway, the first patch that implements any (static or storage) would be fine for me.
The reason is $components may look like array('module' => array(), 'theme' => array()), being actually empty but not evaluating to empty if we don't 'array_filter'
I don't really mind which way, though my guess is a plain FALSE may save some memory for massive update operations. There's no point in caching empty string objects IMO.
About all these, ok with the doc changes, but about TranslateInterface, either we go with it as it is or we drop the whole thing and replace it with some procedural code.
*We don't really need it*, I've just used that interface because it was there and looked like the thing to implement but at this point -the more we discuss it gets worse- I'd say cost outweights benefits.
The whole thing could be a few lines of code in LocaleConfigManager but seriously trimming down LocaleTypedConfig.
Comment #121
alexpottFirstly, @reyero thanks for all the work on this
That sounds great... and yep replaceability will be very useful...
If this is true... simplicity++ :)
Comment #122
Gábor Hojtsy#1968446: Whitespace fixes in language installer and config install storage included one private => public change as well (documented there) that is needed for this patch, so that is why the above failed I think.
$name is string, also document what are these :)
too many spaces before rtrim
@alexpott/@reyero: simplicity as well as familiarity should be weighted. The patch uses typed data translatable interface because then that is standarized; if we make up our own API for this, then people would need to learn it just for this. So I think keeping it for now is good. We can revise later and simplify if needed.
Marked needs work for vijaycs82's cleanup issues.
Comment #123
alexpottShould use
Drupal::moduleHandler()->loadInclude()
should use
$this->container->get('locale.config.typed')->get('system.site');
should use
$this->container->get('locale.config.typed')->get('image.style.medium');
should do
$locale_config = $this->container->get('locale.config.typed');
outside the foreach and then$wrapper = $locale_config->get($config_key);
inside itWhy is this optional and javascript not? Also if this is set then it appears that Javascript translations will not be refreshed...
TBH I'm a bit confused about 'refresh configuration'... is this functon locale_translate_batch_refresh serving two different purposes?
This message seems to need to be different if 'refresh configuration' option is set
Should use
Drupal::moduleHandler()->moduleExists()
Add
@see locale_config_batch_build()
It would be great to not have this procedural helper... we should copy ../core/modules/views/lib/Drupal/views/Views.php and have a Locale public static class
Comment #124
Gábor HojtsyWhy have a Locale class if you are advocating we use the container directly anyway at all cases where locale_config() would be used?
Comment #125
alexpottBecause we need to access the 'locale.config.typed' service from procedural code and having one line wrapper functions is :(
Comment #126
Gábor HojtsyWe can just as well use drupal_container()->get('locale.config.typed') in place, introducing a whole new class so to avoid one function seems like quite a bit of overkill. Not really pointing to simplicity in my view :D
Comment #127
alexpottdrupal_container() is deprecated :)
Comment #128
Gábor HojtsySo much for simplicity :(
Comment #129
Berdirdrupal_container() is deprecated yes, in favor of Drupal::service(). Drupal::$method() and $Module::$method() are optional IMHO :) The main downside of not having those is that you don't get autocomplete for the methods on that object. But having global functions is deprecated in favor of such methods on a Drupal/Module class.
Various people argued very strongly for static methods on classes instead of functions in the issue that deprecated drupal_container(). The advantage of having a class is that it can be autoloaded and therefore also works in real PHPUnit unit tests if we can write those (I honestly don't really understand that argument as you shouldn't use the container in unit tests in theory anyway). But you can easily look up what services a certain module provides :)
Comment #130
Gábor HojtsyWell, if we just use Drupal::service() or drupal_container() direct for that matter, we loose the documentation we have on locale_config() now that is I think useful now. If the "new simple way" is we add a class with static methods instead and trade locale_storage() to Locale::Storage() because that is so much better, then so be it. It does not really matter for this patch, and others already decided this for us, so we don't need to.
Comment #131
vijaycs85Fixed both review comments in #122 and #123 except below:
1. locale_config() - has got follow up at #1968732: replace config_locale() with Locale:storage()
2.
Why is this optional and javascript not? Also if this is set then it appears that Javascript translations will not be refreshed...
TBH I'm a bit confused about 'refresh configuration'... is this functon locale_translate_batch_refresh serving two different purposes?
3.
This message seems to need to be different if 'refresh configuration' option is set
Comment #132
Gábor HojtsyThanks! My understanding is #1968732: replace config_locale() with Locale:storage() should be included here, not in a separate followup. That separation would mean we add use of deprecated APIs with this patch, which is a no-go. I'll review the other changes later.
Comment #133
vijaycs85Adding Locale class and its changes.
Comment #135
Gábor Hojtsyinstall.core.inc does not seem to have a "use Drupal" at the top. This will not work.
"Name of the original configuration. Set to NULL to get the name prefix for all $langcode overrides."
Would be better IMHO :)
locale.module also does not have "use Drupal".
Comment #136
vijaycs85Updating review comments in #135 and renaming Locale to LocaleModule as there is a class Locale in PECL intl ext http://www.php.net/manual/en/class.locale.php
Comment #138
Gábor Hojtsy#119: 1905152-metadata-locale-integration-118.patch queued for re-testing.
Comment #139
Gábor Hojtsy#117: 1905152-metadata-locale-integration-117.patch queued for re-testing.
Comment #140
vijaycs85As #117 passing, rolled back to it and updated changes in individual comments (good to have diff files!!!). Removed changes that would make this test fails. Also implemented the Locale class the way it is used in Symfony (\Symfony\Component\Locale\Locale::getLocales()).
Comment #142
vijaycs85Sorry to burn down testbot... Locally this patch failing with "Maximum function nesting level of '100' reached, aborting!".. trying to issue incremental patch from different changes...
Comment #144
Gábor HojtsyThe locale class we are looking for has nothing to do with symfony. Also you did not post interdiffs so it is kind of hard to tell what are you doing.
Comment #145
vijaycs85@Gábor Hojtsy, sorry for no-interdiff... I was trying to update changes one by one (from #117). Found few issues:
1. 'return' missing in #119
2. wrong file name: trying to load locale/bulk.inc instead of locale/locale.bulk.inc in #131
Fixed them here. Hope it reduce the number of fails from 44 to 28 or 4.
Comment #147
vijaycs851 fail that was fixed in #104. Seems its back to 7 properties... giving a try with 7 and a debug patch to see what exactly getting.
Comment #149
vijaycs85That was wrong debug change. adding new debug file (this patch in not for review).
Comment #151
vijaycs85Currently getting only 4 properties and 1 page from test (as per #149 result). Hoping this fix makes it green...
Comment #152
vijaycs85Remaining issues to address (from #115):
1.
This looks a bit mad... at least needs a comment as to why we need add or remove depending on the $include_default param... ie why does locale_translatable_language_list() sometimes contain the default language and sometimes not...
2.
if getElementTranslation returned an empty array the ternary on the return would not be needed.
3.
This looks weird... because translateElement() returns FALSE only if !empty($options['strict']) ... so I think the OR is unnecessary.
Additionally, if ($translation || empty($options['strict'])) { I think the || empty($options['strict']) is unnecessary... could just by if (!empty($translation)) {
4.
From the function name I would expect this function to return the element... and throw an exception if it can't... the logic looks like at least one level of nesting could be removed.
... also need to document why we have the strict option... i.e. why does the FALSE exist.
5.
Why is this optional and javascript not? Also if this is set then it appears that Javascript translations will not be refreshed...
TBH I'm a bit confused about 'refresh configuration'... is this functon locale_translate_batch_refresh serving two different purposes?
6.
This message seems to need to be different if 'refresh configuration' option is set
Comment #153
Gábor Hojtsy1. FIXED: Added comment on getTranslationLanguages() why do we need the funky code there. The need for $include_default comes from the typed data interface we implement. The possible missing English is coming from the language system.
2. FIXED: Made getElementTranslation() return an empty array if no translation instead of NULL. Easier on the caller.
3. FIXED: (Was already resolved earlier). Further simplified now thanks to 2.
4. NOT FIXED: Due to strict mode which is required due to typed data translatable interface implementation (also see above with $include_default in (1)), we want different behavior based on this option. So we need to transform the element to translated AND tell the level up that we did (or not), so the level up can do different things based on that. So we can introduce a property on the element but that would either require a new property (which sounds like translations property on a level where it is not used outside of locale) on elements or extend elements in locale only, which sounds like a possible disaster (all kinds of element types are possible, extensible in contrib). @alexpott already agreed we should not use exceptions here btw, this is a utility method called for each element on each level in config essentially.
5. FIXED: Removed the 'refresh_configuration' option from batches, it should not be optional. When .po files are imported, locale database storage is updated, the config translations should be updated, the locale database and config translation should not be out of sync.
6. FIXED: Slightly updated wording on the batch message pointed out by @alexpott, IMHO "Configuration" should not be capitalized, its not a brand name like "JavaScript".
--------
In short, it sounds like we should figure out if we want to keep implementing Drupal\Core\TypedData\TranslatableInterface, which is what makes the getTranslationLanguages() weird code and the strict mode in getTranslation() required. Otherwise for me locally, the config tests pass proper. I'm not aware @alexpott had any other issues identified, and the interdiffs above look good fixing issues.
Comment #153.0
Gábor HojtsyUpdated issue summary - steps update
Comment #153.1
YesCT CreditAttribution: YesCT commentedadded remaining task for last discussion of approach
Comment #154
Gábor HojtsyLooking at this more, we are not even implementing the strict mode proper. The docs for strict mode are:
So strict mode should only return translated things (that worked correct), while non-strict mode should return the full original data with translations in-place as available. The non-strict mode implemented by the patch did not work like that at all. It actually filters out data in non-strict mode and did not overlay translations. That is understandable given the role of this is NOT to overlay translations, that is the job of the config override system, and implementing a parallel solution to accessing that data would cause more confusion IMHO then use. So the code works down the data to only the translatable pieces and then only deals with that. If we'd want to have it do the overlay-ing thing for the data like the non-strict mode documents, that would mean we need to maintain per-element data about translatability or have entirely different code paths for the two modes. And even then we'd duplicate what is already being done in the config override/event system.
So in this light, I think its better to actually remove our reliance on this interface (which lets us remove code and simplify this API, even though making it not a standard implementation of typed data, we win by not over-complicating this).
I also found a random bugos string inside a code comment.
Comment #156
Gábor Hojtsy"Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupaltestbotmysql.simpletest936821cache_tags' doesn't exist: SELECT 1 AS expression" does not seem to be related.
Comment #157
Gábor Hojtsy#154: 1905152-metadata-locale-integration-154.patch queued for re-testing.
Comment #158
Gábor Hojtsy#154: 1905152-metadata-locale-integration-154.patch queued for re-testing.
Comment #159
Jose Reyero CreditAttribution: Jose Reyero commentedLatest changes look great to me, looks much simpler, we didn't really need the Translatable interface at all.
There are some improvements I'd like to see, though I'll set this to RTBC because most of these would need this patch committed too, #1813762: Introduce unified interfaces, use dependency injection for interface translation
Notes for future improvements (or in case this needs some re-rolls):
- Repurpose LocaleTypedConfig since it's not too clear anymore which functionality should be encapsulated on each class. It should take care of config language ('langcode'), so the whole 'canTranslate' makes sense. It should also check the data against the defaults by itself (Some method like 'checkDefaults()')
* The main reason for this is this one can only translate 'default English configuration strings' but a module in the future could provide a more powerful one.
- locale_config_update_multiple() would be better a method in LocaleConfigManager. This will simplify existing code and allow replacing the full feature by setting a different LocaleConfigManager.
- Minor performance improvement in batch updates. The number of strings to query at once, currently 100, could be raised to maybe 900 (Safe for both Mysql and Sqlite), and maybe it could be configurable (passed as options for the batch or with a variable). I'm talking about these two lines:
Anyway, as said above, most of this should use the full locale system in DIC and the TranslationInterface provided by the other patch so we better get this feature committed first and revisit it once/if the other patch gets in, that may take a bit longer.
Comment #160
Gábor Hojtsy#154: 1905152-metadata-locale-integration-154.patch queued for re-testing.
Comment #161
alexpottCommitted 91fc0d3 and pushed to 8.x. Thanks!
Comment #162
alexpottComment #163
Gábor HojtsyThanks a lot everyone! This is a huge step forward for us! And one critical done nonetheless :)
I coped over #159 to #1966538-3: Translation is not updated for configuration, when the config changes so we track it with the followup.
Also reopened #1936186: Make use of locale API when it becomes available and #1936208: If config is translated from shipped config or partially edited, also push translations to locale storage for the contrib config_translation module (also proposed for core at #1952394: Add configuration translation user interface module in core), so we can fix compatibility with this new solution.
Updated docs at http://drupal.org/node/1905070/revisions/view/2658980/2659994 to explain how this reflects on the schema system and http://drupal.org/node/1928898/revisions/view/2590502/2659996 on how this reflects on the config override system.
I also added a change notice at http://drupal.org/node/1977368 that explains what this means for site builder, module devs and how it relates to localize.drupal.org, the schemas and overrides.
Comment #165
PanchoAnother followup based on #41:
see #1993452: Fix confusing UX by merging "Translate configuration" into "Finish translations" task
Comment #166
YesCT CreditAttribution: YesCT commentedrelated: #2029075: Configuration translation step in the installation takes a reeeeaaallly long time when installing in a non-English language
Comment #166.0
YesCT CreditAttribution: YesCT commentedfix comment number to be a link
Comment #166.1
YesCT CreditAttribution: YesCT commentedadded the performance feedback
Comment #167
Gábor Hojtsy