Problem/Motivation

In #2201437: [META-1] Config overrides and language the proposal is of several steps. This is yet another break-out issue to solve the issue. By moving language responsibility from being baked into the configuration factory to its own system will allow implementers to swap this out when implementing other types of overrides, eg. domain. Also, this will result in a solid base implementation of overrides and *much less* code to run on sites where foreign languages are not needed. Also much simpler and more consistent base API and easier to understand OO interfaces.

Proposed resolution

- Remove special language handling from Config and ConfigFactory
- Move config language setting to LanguageManagers
- Implement language as it own overrides

Note that #2201437: [META-1] Config overrides and language has several other benefits that are not yet resolved here. For example, we are not introducing its own storage for language which is one of the key points there. This is so we can minimize the impact (which is already pretty big here).

Remaining tasks

- Review.
- Commit.

User interface changes

None whatsoever.

API changes

- Config and ConfigFactory do not have direct language support anymore
- Cache handling in ConfigFactory is a lot more generalized
- Active language for configuration is now set on the LanguageManager

Files: 
CommentFileSizeAuthor
#30 interdiff.txt1.83 KBswentel
#30 2219499.30.patch71.57 KBswentel
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,570 pass(es).
[ View ]
#25 2219499.25.patch71.54 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,494 pass(es), 0 fail(s), and 44 exception(s).
[ View ]
#21 2219499.20.patch72.34 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2219499.20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#20 2219499.20-interdiff.txt6.05 KBBerdir
#20 2219499.20.txt72.34 KBBerdir
#19 2219499.18-interdiff.txt890 bytesBerdir
#18 loaded_config_names.txt2.47 KBBerdir
#18 2219499.18.patch71.16 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,390 pass(es).
[ View ]
#18 difference_after_bugfix_overall.png236.68 KBBerdir
#18 difference_after_bugfix.png259.6 KBBerdir
#18 difference_before_bugfix.png259.6 KBBerdir
#18 difference_no_languages.png172.91 KBBerdir
#17 interdiff.txt1.19 KBGábor Hojtsy
#17 2219499.17.patch71.15 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,413 pass(es).
[ View ]
#15 2219499.15.patch71.18 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,422 pass(es).
[ View ]
#15 interdiff.txt1.29 KBGábor Hojtsy
#14 Config language overrides responsibility (1).png61.88 KBGábor Hojtsy
#12 2219499.13.patch71.06 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,345 pass(es).
[ View ]
#12 11-13-interdiff.txt958 bytesalexpott
#11 2219499.11.patch71.21 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,408 pass(es).
[ View ]
#10 2219499.10.patch80.42 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#10 9-10-interdiff.txt3 KBalexpott
#9 2219499-diff-7-9.txt1.7 KBvijaycs85
#9 2219499-generalize-config-override-9.patch71.16 KBvijaycs85
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,345 pass(es).
[ View ]
#7 2219499-diff-4-7.txt992 bytesvijaycs85
#7 2219499-generalize-config-override-7.patch71.54 KBvijaycs85
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,438 pass(es).
[ View ]
#4 2219499.4.patch71.12 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,312 pass(es), 15 fail(s), and 5 exception(s).
[ View ]
#4 2-4-interdiff.txt1.07 KBalexpott
#2 Config language overrides responsibility.png53.33 KBGábor Hojtsy
#2 config-override-language-responsibility.patch70.76 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,350 pass(es), 56 fail(s), and 5 exception(s).
[ View ]

Comments

Gábor Hojtsy’s picture

Title:Generalize language override responsibility» Generalize language config override responsibility
Gábor Hojtsy’s picture

Issue summary:View changes
Status:Active» Needs review
StatusFileSize
new70.76 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,350 pass(es), 56 fail(s), and 5 exception(s).
[ View ]
new53.33 KB

All right, here is a refactoring of parts of #2201437: [META-1] Config overrides and language which are relevant for this issue. The hardest part was to retain the base config storage model, and make language overrides work with that. I did not in fact succeed in making language overrides work fully. I think this is due to caching problems, but looked on this way too much to be able to tell today. So posting for testbot at least and also manual review.

Most of the code is from @alexpott, so he should be credited heavily. The integration between LanguageConfigFactoryOverride, LanguageManager and the existing storage system for config is my work.

The architecture looks like the attached patch (updating the issue summary with that).

Gábor Hojtsy’s picture

Issue summary:View changes
alexpott’s picture

StatusFileSize
new1.07 KB
new71.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,312 pass(es), 15 fail(s), and 5 exception(s).
[ View ]

Attached patch fixes the language configuration overrides

Status:Needs review» Needs work

The last submitted patch, 4: 2219499.4.patch, failed testing.

The last submitted patch, 2: config-override-language-responsibility.patch, failed testing.

vijaycs85’s picture

StatusFileSize
new71.54 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,438 pass(es).
[ View ]
new992 bytes

Here is a fix for test fails...

vijaycs85’s picture

Status:Needs work» Needs review
vijaycs85’s picture

StatusFileSize
new71.16 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,345 pass(es).
[ View ]
new1.7 KB

as discussed with @Gábor Hojtsy on IRC, we can use ConfigFactory instead of trying to get the same from languageManager. Updating patch as per this condition.

alexpott’s picture

StatusFileSize
new3 KB
new80.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

We can improve on the deleteLanguageTranslations to only delete things that actually exist.

alexpott’s picture

StatusFileSize
new71.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,408 pass(es).
[ View ]

Forgot to rebase :( - interdiff still valid.

alexpott’s picture

StatusFileSize
new958 bytes
new71.06 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,345 pass(es).
[ View ]

In fact we can get away with even less change from 8.x here

Gábor Hojtsy’s picture

Thos changes look good. The issue summary is accurate and up to date. Now need reviews :) Its my and @alexpott's code so we need someone else to review :)

Gábor Hojtsy’s picture

Issue summary:View changes
StatusFileSize
new61.88 KB

Updated issue summary image with what happened to ConfigFactory and Config, so they don't only appear in the before section. They are obviously not gone but changed.

Gábor Hojtsy’s picture

Assigned:Gábor Hojtsy» Unassigned
StatusFileSize
new1.29 KB
new71.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,422 pass(es).
[ View ]

Fixed the two docs @todos.

Berdir’s picture

  1. +++ b/core/lib/Drupal/Core/Language/LanguageManagerInterface.php
    @@ -168,4 +168,23 @@ public function getFallbackCandidates($langcode = NULL, array $context = array()
    +   *
    +   * @return $this
    +   *   The language manager.

    No description needed for $this.

  2. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigSchemaTest.php
    @@ -21,7 +21,7 @@ class ConfigSchemaTest extends DrupalUnitTestBase {
        */
    -  public static $modules = array('system', 'locale', 'field', 'image', 'config_test');
    +  public static $modules = array('system', 'language', ',locale', 'field', 'image', 'config_test');

    Can you spot the error?

    (Hint: Count the ",").

    And yes, DUBT does not validate this right now.

Looks great, a lot of *awesome* cleanup in tests and config_translation.

Want to do some profiling here, just to understand how this changes single and multi-language sites, will do that asap.

Gábor Hojtsy’s picture

StatusFileSize
new71.15 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,413 pass(es).
[ View ]
new1.19 KB

Easy ones, thanks! :)

Berdir’s picture

StatusFileSize
new172.91 KB
new259.6 KB
new259.6 KB
new236.68 KB
new71.16 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,390 pass(es).
[ View ]
new2.47 KB

Ok, profiling. A regression has to be expected when languages are enabled as we're doing separate calls now, so let's see what happens:

Notes:
- All xhprof runs were taken by looking at 4 results and going with the fastest one. There were considerable call time differences, so don't rely too much on that, but the function call changes are obviously stable.
- I did test using an empty front page. Real sites will obviously load many config files more, which will increase the difference.

First, comparing a site without language.module. Not surprisingly, no big difference there, this patch is a tiny bit faster but it's almost not measurable. The difference between multiple xhprof runs was considerable larger.

Then, I added a second language and access the site with that. And was seeing a 40% regression of the whole request and 500% in ConfigFactory::loadMultiple():

That's way more than expected, so I started looking into why it's happening. Quickly identifed that we again had a problem with caching the missing language override config files, so we were doing a cache get + file get + cache set for them. Every time. Tracking down why that happens took me a bit more time:

LanguageConfigFactoryOverride passes an array like array('system.something' => 'langcode.config.en.system.something'), to storage, that passes it to the cache backend, that passes it as query arguments to the database. Which uses the keys to generate the query placeholder, resulting in :cids_system.something, which apparently silently doesn't work (I guess the ".") and doesn't return anything, so it never found the cache entries that it wrote before.

Easily fixed by doing an array_values() on the config names passed to the storage in LanguageConfigFactoryOverride. I'm not sure what the correct layer to fix this is, though. Do we even need them to be keyed by the original name? We don't seem to be using it right now? Should the config storage enforced numeric keys, or the cache implementation? (possibly cache?)

Anyway, another profile run, now it looks considerably better and pretty much what was to be expected. 37 additional config loads, resulting in 37 additional cache requests. That's a 7% regression on the whole site (still within the variations of the different runs, I had before xhprof results that were slower than after xhprof results, but also equally slower after runs) and 40% on ConfigFactory::loadMultiple().

I'm not sure if what we can do there. We *need* to make the implementation better anyway, and having it in a separate class makes it much easier to improve that in a follow and doing so here would make this more complicated and it would be kind of cheating (implementing generic improvements that would also have applied to the previous implementation to make it look like this is not a regression. It is.):

Some ideas to improve performance of loading language overrides:
- A simple blacklist perhabs, of config files that are always loaded and can definitely not be translated. Especially of the low-level config files that are always loaded like system.performance, system.module (per-language enabled module list => mind. blown.), timezone settings and stuff like that. Attaching a list of config files that are loaded on an empty site, we can look at what we can exclude from that.
- A more intelligent system that could track a black and/or whitelist of files to load automatically and save it in a cache or state would als be a possibility, but that's much more complicated as then the override would need to ract on config writes
- Preloading will help, if we can load some of these config files early on and together, it will also be a single load for the language overrides for them.

=> Open a major (at least) follow-up, non-beta blocking to discuss this and try out things?

=> Left to do here: Decide if we want to do a different fix than I did for the cache problem. Can we (unit) test this somehow (Verify that loading a config file with enabled but missing language override (and also one that exists) does not result in cache writes/file storage reads?

Berdir’s picture

StatusFileSize
new890 bytes

Ah, forgot the interdiff.

Berdir’s picture

StatusFileSize
new72.34 KB
new6.05 KB

Changing the logic with the default language a bit:

Right now, we always set the default language and then it is eventually set again to the actual language once negotiation happened.

Instead, this patch does only either of those. For monolingual sites (but with language.module enabled), we set the default language through setLanguageFromDefault() directly set on the service definition and for multilingual sites, we set it after language negotiation did run. That means multilingual sites have no config override language set before language negotiation happens as those overrides would be wrong anyway.

This saves us 10 language override reads/cache gets. Monolingual sites with language.module still have them right now but we can still look into the ideas described above and exclude those with a blacklist instead.

Also removed the now unnecessary persist tag.

Berdir’s picture

StatusFileSize
new72.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2219499.20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Ouch.

Status:Needs review» Needs work

The last submitted patch, 21: 2219499.20.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review

21: 2219499.20.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 21: 2219499.20.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new71.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,494 pass(es), 0 fail(s), and 44 exception(s).
[ View ]

Grr, accidently added my xhprof hacks to the patch :(

Status:Needs review» Needs work

The last submitted patch, 25: 2219499.25.patch, failed testing.

Berdir’s picture

Ok, so those tests fail because getConfigOverrideLanguage() no longer always returns a language. Specifically, it doesn't in a multilingual environment within the test method as there's no bootstrap, obviously I *think* that's ok, if we really want to test langage config override loading in there, we could set it manually.

I'm not sure what's the correct way to fix this, though. Should the language be optional there? LanguageConfigFactoryOverrideInterface::setLanguage() already is, should setConfigOverrideLanguage() be consistent with that? Or should getConfigOverrideLanguage() ensure to always return a language? Looking at the code, I think the first would be the expected behavior, as we want to restore the original behavior and that was no language.

Gábor Hojtsy’s picture

Yeah the first sounds like more in line with what is already done at other places then.

Berdir’s picture

Ok, all we need to do then is make that argument optional. Should be an easy task for someone at the sprint tomorrow?

swentel’s picture

Status:Needs work» Needs review
StatusFileSize
new71.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,570 pass(es).
[ View ]
new1.83 KB
Gábor Hojtsy’s picture

Status:Needs review» Reviewed & tested by the community

Looks good, fixes the only remaining concern. RTBC-ing based on the feedback from @Berdir too. I did do some of the code, but its overwhelmingly @alexpott and @Berdir. Unblocks other work as well. :) Yay.

xjm’s picture

Assigned:Unassigned» catch

  • Commit ad29309 on 8.x by catch:
    Issue #2219499 by Berdir, alexpott, Gábor Hojtsy, vijaycs85, swentel:...
catch’s picture

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

Status:Fixed» Closed (fixed)

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