Over in #1808248: Add a separate module install/uninstall step to the config import process, we're messing with the ConfigImporter.

That apparently can break our install of default config during module install, which is just nuts.

This patch breaks the connection between 'install default config' and 'config import', so we can deal with the complexities of each without breaking the other.

CommentFileSizeAuthor
#14 interdiff.txt789 bytestayzlor
#12 2095489-12.patch6.92 KBtayzlor
#12 interdiff.txt788 bytestayzlor
#9 2095489-9.patch6.84 KBtayzlor
#9 interdiff.txt3.11 KBtayzlor
#6 2095489-6.patch4.62 KBAnonymous (not verified)
#6 interdiff.txt534 bytesAnonymous (not verified)
#2 2095489-2.patch4.26 KBAnonymous (not verified)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Issue tags: +Configuration system

tagging.

Anonymous’s picture

FileSize
4.26 KB

here's the patch.

i didn't git rm ConfigInstaller yet, that can be done after #2095115: delete ViewTestConfigInstaller and have ViewTestData create views, not 'import' them lands.

Status: Needs review » Needs work
Issue tags: -Configuration system

The last submitted patch, 2095489-2.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review

#2: 2095489-2.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Configuration system

The last submitted patch, 2095489-2.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
534 bytes
4.62 KB

now with less durp durp durpal.

Status: Needs review » Needs work

The last submitted patch, 2095489-6.patch, failed testing.

tim.plunkett’s picture

Then you can straight up remove the ConfigInstaller class

tayzlor’s picture

Status: Needs work » Needs review
FileSize
3.11 KB
6.84 KB

New patch which removes the ConfigInstaller class and should hopefully fix the broken test in ConfigInstallTest.

I also added another line to reset the static cache (via the ConfigFactory) for the config name being imported in config_install_default_config() as other tests were failing and it seemed like this was because it was retrieving stale config items from there.

Anonymous’s picture

tayzlor++ i think this is RTBC, but it would be good to get alexpott to have a look at it.

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/includes/config.inc
@@ -66,20 +63,34 @@ function ($value) use ($name) {
+    $entity_manager = Drupal::service('entity.manager');
+    $config_factory = Drupal::service('config.factory');
+    $context = new FreeConfigContext(Drupal::service('event_dispatcher'), Drupal::service('uuid'));
+    $target_storage = Drupal::service('config.storage');

We need to call the ConfigFactory->enterContext before we do anything with config and we need to call ConfigFactory->leaveContext

tayzlor’s picture

FileSize
788 bytes
6.92 KB

New patch attached which switches in and out of context around the config saving.

tayzlor’s picture

Status: Needs work » Needs review
tayzlor’s picture

FileSize
789 bytes

Wrong interdiff uploaded but the patch is still good - so ignore the interdiff in #12

Status: Needs review » Needs work
Issue tags: -Configuration system

The last submitted patch, 2095489-12.patch, failed testing.

tayzlor’s picture

Status: Needs work » Needs review

#12: 2095489-12.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Configuration system

The last submitted patch, 2095489-12.patch, failed testing.

Anonymous’s picture

bump.

Anonymous’s picture

another reason to do this: #2069373: Race conditions on import if CUD on ConfigEntity A triggers changes in ConfigEntity B can't do the obvious thing, because we run import code during install, so the 'don't create fields' flag gets set, and no default config entities get extra fields, breaking all the things.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community

Finally got a pass let's get this done

catch’s picture

Committed/pushed to 8.x, thanks!

swentel’s picture

Status: Reviewed & tested by the community » Fixed

So this is fixed then I guess :)

Status: Fixed » Closed (fixed)

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

xjm’s picture