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.
Comment | File | Size | Author |
---|---|---|---|
#58 | 2457551.58.patch | 33.19 KB | alexpott |
#58 | 54-58-interdiff.txt | 1.24 KB | alexpott |
#54 | interdiff-2457551-43-54.txt | 5.06 KB | keopx |
#54 | 2457551-54.patch | 32.25 KB | keopx |
#50 | interdiff-2457551-43-50.txt | 13.47 KB | keopx |
Comments
Comment #1
Gábor HojtsyComment #2
Gábor HojtsyThis 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.
Comment #3
YesCT CreditAttribution: YesCT commentedThe 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?
Comment #4
rteijeiro CreditAttribution: rteijeiro commentedJust 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.Comment #5
Gábor Hojtsy@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.
Comment #6
Gábor HojtsyI think the only thing this needs now are new tests and reviews :)
Comment #7
Gábor HojtsyHere is a set of tests.
Comment #9
Gábor HojtsySearch and replace went a bit too happy :D
Comment #11
Gábor HojtsyFixed 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.
Comment #13
Gábor HojtsyRerolled.
Comment #15
Gábor HojtsyRerolled again. It did not apply on several hunks.
Comment #17
Gábor HojtsySo 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.
Comment #18
Gábor HojtsyUpdating 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.
Comment #19
Gábor HojtsyNeeds review now.
Comment #21
alexpottThe solution looks good - we're not really changing how things work currently just making sure the optional config is translatable.
This should not be public - need to add
public: false
to the definition.Need to add $default_config_storage to docblock.
Missing class docblock
Let's rename this to
assertNodeConfig
so we know which call to it failed.Comment #22
Gábor HojtsyAll of those suggestions make sense, implemented.
Comment #23
eiriksmHm.
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:
With the patch:
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 :)
Comment #24
Gábor Hojtsy@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.
Comment #25
Gábor Hojtsy@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.
Comment #26
Gábor Hojtsy@eiriksm: opened #2468767: English config source strings are saved as custom translations in locale in foreign install and looking into it there.
Comment #27
rteijeiro CreditAttribution: rteijeiro commentedSome minor nitpicks solved. Code seems fine. It's RTBC for me.
Comment #28
Gábor HojtsyThe comment reflow changes look fine. Thanks!
Comment #29
eiriksmJust 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 :)
Comment #30
eiriksmSteps to reproduce:
- Try to run LocaleConfigSubscriberTest through the GUI with this patch applied.
Comment #31
eiriksmHm, 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
Comment #32
Gábor HojtsyHow 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.
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?
Comment #33
eiriksmThe 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.
Comment #34
Gábor Hojtsy@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?
Comment #35
eiriksmThe 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?
Comment #36
Gábor HojtsyDepending 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. :/
Comment #37
eiriksmOh, I think this only applies to KernelTestBase tests, not webtests
Comment #38
Gábor HojtsyWell, 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?
Comment #39
eiriksmI 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
Comment #40
Gábor HojtsyI would start trying to change
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.
Comment #41
eiriksmHm, 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:
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
Comment #42
eiriksmForgot to attach the patch.
Comment #43
eiriksmSorry for the spam, last patch did not contain the added file.
Comment #44
eiriksmHm, 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...
Comment #45
rodrigoaguileraLet's run the testbot
Comment #47
eiriksmCan 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).
Comment #48
rteijeiro CreditAttribution: rteijeiro commentedCode 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.
Comment #49
keopxComment #50
keopxHere patch
Comment #51
rteijeiro CreditAttribution: rteijeiro commentedNot sure if it needs manual testing. Looks good to me.
Comment #52
eiriksmHm. 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.
Comment #53
rodrigoaguilera@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
Comment #54
keopxSorry, I confused.
Here new patch
Comment #55
rteijeiro CreditAttribution: rteijeiro commentedNow 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?
Comment #56
alexpott@rteijeiro are you sure - it seems to be introduced by this patch.
Comment #57
alexpottThe 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.Comment #58
alexpottThis fixes the problem.
Comment #59
eiriksmGreat! 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.
Comment #60
alexpottThis 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.
Comment #62
Gábor HojtsyYay, thanks! Superb!