Working on #1872522: Compiled data in PHP storage is cleared too late in drupal_flush_all_caches(), I encountered a LocaleConfigOverride test failure that I was surprised wasn't also happening on HEAD. I discovered the reason was it was being masked by rebuildContainer() being called repeatedly rather than using the kernel's intended API of persisting across reboots. There's a @todo in HEAD's code about that, so this patch resolves that todo, which will surface at least that one failure for us to deal with.
| Comment | File | Size | Author |
|---|---|---|---|
| #23 | 1878512.23.patch | 11.64 KB | alexpott |
| #23 | 19-23-interdiff.txt | 2.21 KB | alexpott |
| #19 | 1878512.19.will-fail.patch | 8.12 KB | alexpott |
| #19 | pass-fail-interdiff.txt | 692 bytes | alexpott |
| #19 | 1878512.19.pass_.patch | 8.8 KB | alexpott |
Comments
Comment #2
effulgentsia commentedThis doesn't fix the LocaleConfigOverride test, but it implements one additional cleanup to make sure $this->container gets updated within resetAll().
The reason for the LocaleConfigOverride test failure is this: as modules are enabled within WebTestBase::setUp(), module_enable() calls updateModules() on the kernel, causing it to reboot. The patch in #1187726-118: Add caching for configuration / rework config object loading (Was: Memory usage and i/o from config objects) introduced the concept of persisting service instances across kernel reboots. For now, the only service being persisted is 'config.factory'. That service has 'config.storage' and 'event_dispatcher' injected into it, but those services are not currently tagged to persist. So, what happens is that in a reboot, the persisted 'config.factory' service retains a local copy of the old 'config.storage' and 'event_dispatcher', while calling drupal_container()->get('event_dispatcher') on the new container instantiates a new event dispatcher.
So, locale_language_init() ends up adding LocaleConfigSubscriber to the new event dispatcher, not the one used by the persisted config factory.
Possible solutions:
- Tag event_dispatcher to persist.
- Make DrupalKernel automatically persist all service instances that are dependencies of a service tagged to persist.
- Change locale_language_init() to add LocaleConfigSubscriber to the config factory's event dispatcher rather than drupal_container()'s event dispatcher.
- Add LocaleConfigSubscriber statically within LocaleBundle rather than dynamically within locale_language_init(), and make LocaleConfigSubscriber itself dynamically check language_multilingual() for deciding whether to apply overrides or not.
- Add LocaleConfigSubscriber statically within LocaleBundle and decide that overrides should apply regardless of whether the site has only 1 language installed or not.
Thoughts?
Comment #4
effulgentsia commentedThe RDF failure in #2 was random. This implements the possible solution #2.1. I don't know if it's the most logical one, but just demonstrating that it works. One possible flaw with it is that the event dispatcher itself receives 'service_container' as a dependency, which means that will be the old container, not the rebuilt one, so any lazy loaded subscribers will be lazy loaded from the old container, not the new one.
Comment #5
effulgentsia commentedA possible solution to this, and one that maybe overlaps with #2.3, is we can introduce a 'config.event_dispatcher' service that is different from the primary 'event_dispatcher', and make 'config.factory' use that one instead, and that one could use the base EventDispatcher class rather than ContainerAwareEventDispatcher.
Comment #6
effulgentsia commentedClicking the "view details" in #4 shows bot to be stuck in a loop on it: is there a way to kill it? I'll post something more along the lines of #5 when I get a chance.
Comment #7
effulgentsia commentedI'll come back to this soon, after figuring out #1885780: Remove persistence of config.factory across kernel reboots.
Comment #8
effulgentsia commentedThis combines #2 and #1885780-2: Remove persistence of config.factory across kernel reboots.
Comment #10
effulgentsia commented#8: simpletest-kernel-8.patch queued for re-testing.
Comment #12
effulgentsia commentedComment #13
effulgentsia commentedMerged into #1872522-25: Compiled data in PHP storage is cleared too late in drupal_flush_all_caches().
Comment #14
sunmmh, not sure whether merging these two issues makes sense. The testing environment handling is fairly specific to tests.
I think the two issues will be easier to tackle and discuss in isolation. Thus, un-duping for now.
That said, #1331486: Move module_invoke_*() and friends to an Extensions class introduces a range of new usages for
rebuildContainer()across all tests — primarily as a mechanism to synchronize the ModuleHandler from the test process with that of the internal browser; e.g., after enabling modules through the UI/internal browser, the kernel needs to be rebuilt, so as to match the environment of the internal browser. Previously, all of that code usedsystem_list_reset(), but with the new ModuleHandler, thesystem_list*()functions only remain to deal with themes.Comment #15
chx commentedNote that persist is broken: when I added an EntityBundle which added a config.save subscriber this was not added to the EventDispatcher that ConfigFactory holds. I filed #1893566: The persist facility is broken for this.
Comment #16
chx commentedCaution. Check the git.drupal.org:sandbox/chx/1857558.git configquery branch at 9eb99a606e2178c75e196e0b987755955f14b1d6 which had http://drupal.org/node/1885780#comment-6947798 applied and failed ~50% of the time.
Comment #17
effulgentsia commentedStarting over due to #14. This only contains the change of rebuildContainer() to prepareKernel(). And changes every caller of rebuildContainer() other than setup() to instead call resetAll(). The goal is for within a test, for the kernel to stay the same, and for only the container to get rebuilt.
This will fail since other fixes from earlier patches haven't been incorporated yet, because I want to see which of those fixes are still relevant.
Comment #19
alexpottWhilst working on another patch I discovered the the container during WebTestBase::setUp is not always consistent! A service can be constructed with a different container from \Drupal.
Patch attached adds a test for this situation, cleans up a lot of unnecessary calls to TestBase::rebuildContainer() and proposes a fix. At this point I'm not entirely sure why it works but I believe it has to do with kernel rebuilding in rebuildContainer().
No interdiff with previous patches on this issue as started afresh.
Comment #22
alexpottComment #23
alexpottSo there is something special about the kernel and language - hopefully #1862202: Objectify the language system will help resolve this.
Comment #25
berdirInteresting, I was in fact able to confirm that LanguageConfigurationTest is green when combined with the other issue (yay!) but only when *not* calling rebuildContainer(). With it, it fails uglier than it does now. Fragile stuff :(
Comment #26
berdirSomething else that I noticed.
In rebuildContainer(), we use the testing environment and prevent the container from dumping. While that makes sense for DUBT, it is in a way weird for web tests because on the first request, they will build the container and dump it. But drupal_handle_request() uses the hardcoded 'prod' environment, so they're not consistent.
In #2165549: Reduce number of password hashing iterations in all tests to improve test performance, @pwolanin tried to change that to use the testing environment when we're actually testing. Would it help/make sense to use the same environment and dump the container in web tests in the assumption that it's very likely that we will have a page request (if not then they probably shouldn't be web tests..)
Comment #27
tstoecklerAs far as I'm aware a lot of tests still have to be web tests due to old-style coupling of systems, e.g. theme/ render tests.
Comment #28
berdirWhy shouldn't render tests work as DUBT tests? :)
Comment #29
sunI'd rather do the reverse:
WebTestBase::curlExec() → $this->container = include PhpStorage->get('container');
For web/integration tests, the goal is not to implant the container of the parent site (test-runner) into the child site, but instead, to resemble the container/state of the child site in the parent site (test-runner).
In essence, we want the parent site (test-runner) environment to match the child site as close as possible.
At the same time, any kind of automation can easily result in unwanted/undesired side-effects for the test that is being executed.
Disclaimer: I didn't look at any patches here, and perhaps I misinterpreted the last comments.
Comment #30
sunIn light of #29 and #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead, I wonder:
Shouldn't we actually override the
TestBase::rebuildContainer()method inWebTestBaseto rather let it reload the already dumped container from the child site?I.e., along the lines of this?
Comment #37
jibranCan we mark this outdated now?
Comment #46
longwaveSimpletest is long gone, and we no longer rebuild the container again in installModulesFromClassProperty().