Problem/Motivation

The ConfigImporter records errors as it validates and when it is processing the configuration import. We should empty the errors out prior to re-validating. This makes the ConfigImporter easier to test and more consistent.

Steps to reproduce

See changed tests and #2392815: Module uninstall validators are not used to validate a config import

Proposed resolution

Reset \Drupal\Core\Config\ConfigImporter::$errors in ::validate() method

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

N/a

CommentFileSizeAuthor
#8 3284270-7.patch22.6 KBalexpott
#8 6-7-interdiff.txt804 bytesalexpott
#6 3284270-6.patch22.57 KBalexpott
#3 3284270-3.patch4.8 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Tagged with Distribution Modernization Initiative because this blocks #2392815: Module uninstall validators are not used to validate a config import which in turn blocks #1170362: Install profile is disabled for lots of different reasons and core doesn't allow for that.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 3284270-3.patch4.8 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

If you look at the two tests changed here you can see the errors have items from the previous call to ::validate() which is incorrect. Each call to validate() should create a completely new list. We can't see errors to an empty array in \Drupal\Core\Config\ConfigImporter::reset() because otherwise we can't get the errors at the end of a configuration import because we call ::reset() in \Drupal\Core\Config\ConfigImporter::finish().

bircher’s picture

bircher
Location 🇨🇿
commented

The test bot is not done running.. but..
don't we have to set back $sync->write('core.extension', $extensions); otherwise the second validation still contains the error about the missing module.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 3284270-6.patch22.57 KB

@bircher that's part of the bug - the test in HEAD has things duplicated in it :)

Also I think we should take this opportunity to refactor \Drupal\KernelTests\Core\Config\ConfigImporterTest to use \Drupal\Tests\ConfigTestTrait properly - there's no need to this test to do it's own stuff.

bircher’s picture

bircher
Location 🇨🇿
commented

Ok, checking out the code and applying the patch reveals that we currently assert that the error messages are duplicated!
Lol... we even assert that the bug exists! I was going to write we should set it back so that we can see the error message go away the second time.. but of course just fixing the duplication is good enough.

cross-post
Yes I saw that better with the patch applied and the IDE than just by looking at the patch alone

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 6-7-interdiff.txt804 bytes
new 3284270-7.patch22.6 KB

Let's be consistent and always define the variable outside of the try...

bircher’s picture

bircher
Location 🇨🇿
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.
I just tested this to see if it could also fix a bug in my contrib project (#3255091: Deactivating via the UI can fail in some circumstances) but alas, my bug is in another castle.

One could nitpick to add a comment before resetting the errors in validate.. but actually this makes so much sense that it is rather fascinating how the original code was ok with it being duplicated in the first place. So I am good with RTBC.

  • catch committed 51ab247 on 10.0.x 
    Issue #3284270 by alexpott, bircher: Reset \Drupal\Core\Config\...
  • catch committed dc80da2 on 9.4.x
    Issue #3284270 by alexpott, bircher: Reset \Drupal\Core\Config\...
  • catch committed 452ae8b on 9.5.x
    Issue #3284270 by alexpott, bircher: Reset \Drupal\Core\Config\...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Resetting errors makes total sense. A bit odd that the tests were verifying the bug exists but can also see how it happened given the bug is more errors than there is supposed to be rather than some vs. zero.

Committed/pushed to 10.0.x, cherry-picked to 9.5.x and 9.4.x, thanks!

immaculatexavier’s picture

immaculatexavier commented
immaculatexavier’s picture

immaculatexavier commented

Status: Fixed » Closed (fixed)

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