Problem/Motivation

As per the patch in #2090115: Don't install a module when its default configuration has unmet dependencies, it seems that locale was not modified, so it uses a default instantiated InstallStorage which defaults to using config/install only to discover default configuration. That means that optional configuration cannot be translated with locale. Either this needs to be resolved, or #2428045: Comparing active storage to install storage is problematic, install storage may change anytime needs to be resolved.

Proposed resolution

1. Remove the install storage's service entry (its not used other than tests).
2. Use instantiated versions of install storage for the default install dir and the optional dir in locale.
3. Add tests with an optional config.

Remaining tasks

Add tests. Review.

User interface changes

None.

API changes

config.storage.installer service is removed. The class used for the service is intact and not modified.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
7.26 KB
Gábor Hojtsy’s picture

FileSize
15.76 KB
17.89 KB

This reaches a complexity where the "storage" used by locale needs to be its own class/service and injected. The multiple prefixed method names were an indication before of this complexity but now the more properties needed were even worse. Note that I did not implement StorageInterface with the new class because it has looooots of methods we don't need in locale anyway. We can name the class differently to avoid that confusion if there is such confusion.

YesCT’s picture

The changes to core/modules/config/src/Tests/ConfigInstallTest.php do not look like what would be needed to have caught this before.

Do we need some new test coverage?

rteijeiro’s picture

FileSize
17.87 KB
427 bytes

Just fixed a small nitpick. Reviewed the code and seems fine. Also checked the changes in core/modules/config/src/Tests/ConfigInstallTest.php and they don't look like they are doing something new.

Gábor Hojtsy’s picture

@YesCT/@rteijeiro: right, the ConfigInstallTest.php and the core.services.yml changes are not required to fix this bug. However the existence of the install storage service makes it appear you have a one-stop shop to access default config files, which is not the case. The service is only used in the test, actual runtime code does not use it. @alexpott suggested it be removed because then the confusion is also gone.

Gábor Hojtsy’s picture

Issue tags: +Needs tests

I think the only thing this needs now are new tests and reviews :)

Gábor Hojtsy’s picture

FileSize
9.93 KB
27.79 KB

Here is a set of tests.

Status: Needs review » Needs work

The last submitted patch, 7: 2457551-7.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
27.79 KB
498 bytes

Search and replace went a bit too happy :D

Status: Needs review » Needs work

The last submitted patch, 9: 2457551-9.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
28.19 KB

Fixed the node asserts by confirming the dependent modules too. Added more asserts for the presence of configuration. The optional views one is still not passing though :/ Not yet sure why.

Status: Needs review » Needs work

The last submitted patch, 11: 2457551-11.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
28.17 KB

Rerolled.

Status: Needs review » Needs work

The last submitted patch, 13: 2457551-13.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
27.88 KB

Rerolled again. It did not apply on several hunks.

Status: Needs review » Needs work

The last submitted patch, 15: 2457551-15.patch, failed testing.

Gábor Hojtsy’s picture

So looks like the fail is due to how we invoke LocaleConfigManager::getComponentNames() in locale_config_batch_update_components(), we only pass in the component being installed. We avoided that in locale_system_set_config_langcodes() for the same reason, due to secondary imports when installing modules.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
30.64 KB
2.75 KB

Updating locale_system_update() then.

1. Running the config update here too like with adding a language because we should parse the config files even if the locale update did not run. We should make the source strings available.
2. Run the batch for all components, because installing a component may install config from other components.

Gábor Hojtsy’s picture

Issue tags: -Needs tests

Needs review now.

Gábor Hojtsy queued 18: 2457551-18.patch for re-testing.

alexpott’s picture

Status: Needs review » Needs work

The solution looks good - we're not really changing how things work currently just making sure the optional config is translatable.

  1. +++ b/core/modules/locale/locale.services.yml
    @@ -1,7 +1,10 @@
    +  locale.default.config.storage:
    +    class: Drupal\locale\LocaleDefaultConfigStorage
    +    arguments: ['@config.storage', '@language_manager']
    

    This should not be public - need to add public: false to the definition.

  2. +++ b/core/modules/locale/src/LocaleConfigManager.php
    @@ -112,13 +110,13 @@ class LocaleConfigManager {
        * @param \Drupal\language\ConfigurableLanguageManagerInterface $language_manager
        *   The language manager.
        */
    -  public function __construct(StorageInterface $config_storage, StorageInterface $install_storage, StringStorageInterface $locale_storage, ConfigFactoryInterface $config_factory, TypedConfigManagerInterface $typed_config, ConfigurableLanguageManagerInterface $language_manager) {
    +  public function __construct(StorageInterface $config_storage, StringStorageInterface $locale_storage, ConfigFactoryInterface $config_factory, TypedConfigManagerInterface $typed_config, ConfigurableLanguageManagerInterface $language_manager, LocaleDefaultConfigStorage $default_config_storage) {
    

    Need to add $default_config_storage to docblock.

  3. +++ b/core/modules/locale/src/LocaleDefaultConfigStorage.php
    @@ -0,0 +1,152 @@
    +
    +class LocaleDefaultConfigStorage {
    

    Missing class docblock

  4. +++ b/core/modules/locale/src/Tests/LocaleConfigTranslationTest.php
    @@ -191,4 +198,54 @@ public function testConfigTranslation() {
    +  protected function checkNodeConfig($required, $optional) {
    

    Let's rename this to assertNodeConfig so we know which call to it failed.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
31.46 KB
4.28 KB

All of those suggestions make sense, implemented.

eiriksm’s picture

Hm.

For me this patch makes it possible to have the column "Title" on the /admin/content page be translated as usual, which is good, this was not possible without the patch.

However, I noticed that in spite of this being possible, it was in fact not translated out of the box, even though the translation was downloaded. This seemed strange to me, since the string "Title" used to be translated earlier. Looking in the translation table this now forces the source "Title" to be translated into "Title", regardless of what the downloaded sources say.

Or to put it in code speak:

Without the patch:

$ echo "SELECT * FROM locales_source as ls INNER JOIN locales_target as lt ON lt.lid = ls.lid WHERE ls.source='Title';" | drush sql-cli
lid	source	context	version	lid	translation	language	customized
298	Title		8.0.0-dev	298	Tittel	nb	0

With the patch:

$ echo "SELECT * FROM locales_source as ls INNER JOIN locales_target as lt ON lt.lid = ls.lid WHERE ls.source='Title';" | drush sql-cli
lid	source	context	version	lid	translation	language	customized
298	Title		8.0.0-dev	298	Title	nb	1

So it solves one problem, but creates another, no? Those queries are both run right after installation (in Norwegian), by the way.

That's what it seems to me at least. If I misunderstood the issue (i hope not), then you are welcome to ignore this though :)

Gábor Hojtsy’s picture

@eiriksm: this does not change how that translation is saved, so it does not sound like a bug in the scope of this patch? It would be nice if you could track down where does that happen. This patch merely makes the optional configuration sources available to locale again, it does not mandate or change anything about the translation values for them.

Gábor Hojtsy’s picture

@eiriksm: ok, I reproduced your bug very easily. I believe that is due to how locale saves "translations" of values to the database after locale_system_set_config_langcodes() runs. Before this patch, the optional config was not participating in what that function does, but now it is, so its langcode is updated. Then because the locale updating flag is not on, LocaleConfigSubscriber will consider the values in the config as "translations" and save them to locale. However, this should only happen if the translation was "new" and is different from the source or not new and different from the current translation. That in theory should never save the same string as the source string, unless the object was not new somehow and use an empty translation (ie. if the config file was saved after locale was installed / the module was installed), which I am not sure how can we detect at all. Either way, it would be great to open an issue for, because it is a pre-existing bug to this issue. This issue just extends the scope of that bug to optional default configuration. It would already happen to required default configuration.

Gábor Hojtsy’s picture

rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
31.46 KB
1.6 KB

Some minor nitpicks solved. Code seems fine. It's RTBC for me.

Gábor Hojtsy’s picture

The comment reflow changes look fine. Thanks!

eiriksm’s picture

Just noticed while writing tests for #2468767: English config source strings are saved as custom translations in locale in foreign install that this patch breaks running tests (that relies on locale) through the GUI because it ends up setting the config update batch in the testrunner batch.

Not sure what the ideal solution is. Can we make the batch setting more "batch-aware"? Can we check if we are running as simpletests? Both those things sound not ideal, but even less ideal is not being able to run the tests i guess :)

eiriksm’s picture

Steps to reproduce:

- Try to run LocaleConfigSubscriberTest through the GUI with this patch applied.

eiriksm’s picture

Status: Reviewed & tested by the community » Needs work

Hm, thinking about it more, this would maybe apply also if a module were to have an update hook that enabled modules, and you were to run that in a browser context. Am i right?

Setting back to needs work

Gábor Hojtsy’s picture

this patch breaks running tests (that relies on locale) through the GUI because it ends up setting the config update batch in the testrunner batch

How does that happen? The config update batch runs in the TESTED system while the test batch itself should run in the TESTING system. The testing system has no problems running batches, eg. the multilingual install webtest runs through module install batches, etc. pretty well. Not sure this became a problem. I can reproduce this but not sure how to fix it and other batches work fine.

if a module were to have an update hook that enabled modules, and you were to run that in a browser context

I am not aware that we run the config update batch in the update process. How does this patch make that different? That sounds like a theoretic problem that you did not actually produce?

eiriksm’s picture

The last one is a theoretical problem i did not actually produce. I could just imagine that some contrib module might do that (or something else). But the first one is a real problem that is easy to reproduce.

Gábor Hojtsy’s picture

@eiriksm: but we don't do config update in the update system that I am aware of at least, so that should not be a problem. Any idea for the batch?

eiriksm’s picture

The only idea i have is to not do it in a batch. I had to do this to overcome the testing problem when I found it out. I just processed everything in the request instead.

The other way to do it would be to add it to a cron queue maybe?

Both ways are not as ideal, I would say. And the best UX would come from having it in a batch. I do not have intimate knowledge about the batch system. Is it like a global queue that you just end up adding items to if you do batch_set?

Gábor Hojtsy’s picture

Depending on the amount of configuration, this may take pretty long. I am not sure we NEED to do it in a batch but with 10 languages and a thousand config files, it is a lot of lookups and writes to happen and a thousand config files is not that much given how many files the creation of a field, content type (form mode, views modes, etc) result in.

A batch should be able to set batches, and as I said we do testing of batches in several existing tests. :/

eiriksm’s picture

Oh, I think this only applies to KernelTestBase tests, not webtests

Gábor Hojtsy’s picture

Well, I am not surprised that KernelTestBase tests would not have a batching system. We can update the test itself then to provide the requisite background (install db schemas) needed for batches I guess. Can you help with that?

eiriksm’s picture

I guess I could try. But I am not sure I understand what you mean. Update the KernelTestBase "to provide the requisite background (install db schemas) needed for batches" or update the failing test? And don't this kind of sound like a separate issue?

Also - any pointers? From the sound of it, I have no idea where to start. Which is partly why it sounds interesting to do :p

Gábor Hojtsy’s picture

I would start trying to change

    $this->installSchema('locale', ['locales_source', 'locales_target', 'locales_location']);

in LocaleConfigSubscriberTest to also install db schema for the batch tables from system module, which is what the simpletest UI is saying is not present.

eiriksm’s picture

FileSize
855 bytes

Hm, I am a bit at loss here now. I resolved that concrete error (not finding the table), but now I am getting this PDO exception, that I have never seen:

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[HY000]: General error: failed to instantiate user-supplied statement class: INSERT INTO {queue} (name, data, created) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2); Array ( [:db_insert_placeholder_0] => drupal_batch:9:1 [:db_insert_placeholder_1] => a:2:{i:0;s:32:"locale_config_batch_refresh_name";i:1;a:2:{i:0;a:20:{i:0;s:14:"core.extension";i:1;s:36:"core.menu.static_menu_link_overrides";i:2;s:15:"action.settings";i:3;s:25:"views.view.test_bulk_form";i:4;s:19:"aggregator.settings";i:5;s:64:"core.entity_view_display.aggregator_feed.aggregator_feed.default";i:6;s:64:"core.entity_view_display.aggregator_feed.aggregator_feed.summary";i:7;s:64:"core.entity_view_display.aggregator_item.aggregator_item.summary";i:8;s:45:"core.entity_view_mode.aggregator_feed.summary";i:9;s:45:"core.entity_view_mode.aggregator_item.summary";i:10;s:24:"aggregator_test.settings";i:11;s:22:"block.block.test_block";i:12;s:40:"core.entity_view_mode.block_content.full";i:13;s:32:"field.storage.block_content.body";i:14;s:25:"block.block.foobargorilla";i:15;s:13:"book.settings";i:16;s:42:"core.base_field_override.node.book.promote";i:17;s:42:"core.entity_form_display.node.book.default";i:18;s:42:"core.entity_view_display.node.book.default";i:19;s:41:"core.entity_view_display.node.book.teaser";}i:1;a:2:{i:0;s:2:"en";i:1;s:2:"de";}}} [:db_insert_placeholder_2] => 1428937930 ) in _batch_populate_queue() (line 889 of /Library/WebServer/Documents/d8-dev/core/includes/form.inc).

Adding some breakpoints, I can see that the first test is always passing, but the second one is always failing (no matter what order they are in). But if I change it to only run one test (no matter which one) it always runs fine. So I thought maybe something about id's being reused or something. Like maybe I need the sequences table or something. But no. Based on the error message, it seems the pdo extending class can not be instantiated or something?

Also, there are no other kerneltestbase tests that are running batches, as far as I could see, so nothing to look after :/

Hints? Solutions? Or worse - is it just on my machine? :p

eiriksm’s picture

FileSize
26.73 KB

Forgot to attach the patch.

eiriksm’s picture

FileSize
32.3 KB

Sorry for the spam, last patch did not contain the added file.

eiriksm’s picture

Hm, I found these issues: #2239181: SessionManager::isStarted() returns TRUE even after session_write_close() has been called and #2239969: Session of (UI) test runner leaks into web tests where one of them deals with the same pdo exception, and the other one is talking about KernelTestBase tests not being run because the test runner session is leaking to the test. Which kind of feels like something similar...

rodrigoaguilera’s picture

Status: Needs work » Needs review

Let's run the testbot

The last submitted patch, 42: 2457551-42.patch, failed testing.

eiriksm’s picture

Can I just add that the testbot would not pick up the problem I reported in #29 / #30. You would have to manually run the test through the simpletest UI. And the patch still does not fix that. Not sure how to fix the latest exception message, but please feel free to pick it up where I left off (I do not have time to work on this for a couple of days, probably).

rteijeiro’s picture

+++ b/core/modules/locale/src/Tests/LocaleConfigTranslationTest.php
@@ -191,4 +198,54 @@ public function testConfigTranslation() {
+    $this->drupalPostForm(NULL, array(), t('Continue'));

Code looks nice. Just being a little bit picky maybe we should use the short syntax for array() and use []

I'll do some manual tests and RTBC the issue if everything goes well.

keopx’s picture

Assigned: Unassigned » keopx
keopx’s picture

Assigned: keopx » Unassigned
FileSize
45.84 KB
13.47 KB

Here patch

rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community

Not sure if it needs manual testing. Looks good to me.

eiriksm’s picture

Status: Reviewed & tested by the community » Needs work

Hm. I really don't want to be a party pooper here, but I have 1 concern and one question:

- First of all, is it really RTBC when this will make us unable to run some tests through the UI? I mean, this patch _breaks_ the running of LocaleConfigSubscriberTest through the simpletest UI. Which kind of makes manual testing extremely important, I would say. Shouldn't this be adressed first? I'm setting it back to needs work, but as I said, if I am the only one who things this is a problem, then I guess the majority means this is RTBC.

And then, one question. Why is the interdiff mainly about the array syntax of the book module? Surely we should clean that up, but is that really something that relates to this issue?

I am really sorry for being negative here, just want to make sure that RTBCing this is a good idea. Also, awesome job with progression on this, and reviewing it really fast.

rodrigoaguilera’s picture

@eiriksm
Yes that about running tests through the id should be definitely adressed.

And also the last patch and interdiff is touching book module when it shouldn't

keopx’s picture

Status: Needs work » Needs review
FileSize
32.25 KB
5.06 KB

Sorry, I confused.

Here new patch

rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community

Now it looks good to me. It seems that the bug reported in #52 is not introduced by this patch. I can reproduce it even when the patch is not applied. Should I open an issue or is it there any to cover that problem?

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

@rteijeiro are you sure - it seems to be introduced by this patch.

alexpott’s picture

The problem is being caused by the batch set in locale_system_update. This can't be done in the test runner - it has to be done in a request to the child site - or we need to do the necessary batch steps in the setUp so this is a no-op.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.24 KB
33.19 KB

This fixes the problem.

eiriksm’s picture

Status: Needs review » Reviewed & tested by the community

Great! Didnt think about that, that was of course the way to go!

Both the bug the initial patch was for and the test runner bug the first patch "introduced" is now looking good to me.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 2ca1f2d and pushed to 8.0.x. Thanks!

My involvement in this issue was only fixing the test to work when testing through the UI - in doing that I've reviewed the patch several times so I think it is fine for me to commit.

  • alexpott committed 2ca1f2d on 8.0.x
    Issue #2457551 by Gábor Hojtsy, rteijeiro, keopx, eiriksm, alexpott:...
Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, thanks! Superb!

Status: Fixed » Closed (fixed)

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