Validate config that gets imported from modules or the recipe's config directory

This blocks #3401723: [PP-1] Config modified by action plugins should be validated before it is saved → .

This also blocks #3401925: [PP-2] After a recipe is successfully applied, validate all of the config that was imported or modified by config actions → .

I think this is ready for initial review. It has test coverage and everything!

RTBC per https://git.drupalcode.org/project/distributions_recipes/-/merge_request... — AFAICT this is a net improvement.

This blocks three issues.

See MR.

Back to review here; if it looks good then I think we might need to postpone this issue on #3390919: [PP-2] If a recipe fails to apply due to config validation errors, revert the site's config to its original state → . I tried to explain why in https://git.drupalcode.org/project/distributions_recipes/-/merge_request....

I discussed this with @alexpott. He confirmed that my understanding in https://git.drupalcode.org/project/distributions_recipes/-/merge_request... is correct.

We probably need to ultimately do something like this:

Do the current check (ensure dependencies look right beforehand), using validateDependencies().
Create all the config, with createConfiguration().
Go back and call validate() on all the config we just created. If any of it fails, throw the new ConfigValidationException, and let the recipe runner revert everything back to its pre-recipe state.

Therefore, this issue is postponed on #3390919: [PP-2] If a recipe fails to apply due to config validation errors, revert the site's config to its original state → .

I was wrong, you were right! 😊

Now eagerly following #3390919: If a recipe fails to apply due to config validation errors, revert the site's config to its original state → 😄

One question though:

1. Go back and call validate() on all the config we just created. If any of it fails, throw the new ConfigValidationException, and let the recipe runner revert everything back to its pre-recipe state.

We could already do that today, even without #3390919: If a recipe fails to apply due to config validation errors, revert the site's config to its original state → . But rather than an automatic revert, we'd have to tell the user something like "Either manually fix the configuration or revert both the code and the database to the previous state."

To @bircher's point in #3390919-23: If a recipe fails to apply due to config validation errors, revert the site's config to its original state → — we may need to support that anyway.

So AFAICT this is hard blocked, but it just make more sense to do the other issue first. Is my understanding corect?

So AFAICT this is [not] hard blocked, but it just make more sense to do the other issue first. Is my understanding corect?

That's correct!

Understanding this

So this is built on:

1. * 6cc95f1b6f5286b93198ef36f678a88005045d80 Make core dispatch config events for non-default collections. aka 🐛 Config collections do not trigger configuration events consistently Fixed

2. * 9270b7d97ab43e644942ff13d6548eec49f91934 Add checkpoints support
   

   aka #3390919: [PP-1] If a recipe fails to apply due to config validation errors, revert the site's config to its original state → , but note that this is causing one of the PHPStan level 9 violations: https://git.drupalcode.org/project/distributions_recipes/-/merge_request...

Could you in the future please state precisely which commit you've applied from those issues? 🙏

The other PHPStan level 9 error is https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iter... — not quite sure how to fix it.

Review

To review this, look at git diff 9270b7d97ab43e644942ff13d6548eec49f91934.

Overall: very nice start! 😄

1. I think the #1 thing to add is the most basic test coverage possible: a trivial config-only recipe that fails causes a validation error, and is then automatically reverted.
2. Then subsequently, have a recipe FOO that depends upon another recipe BAR, and have BAR trigger a validation error, which should cause the end result to revert to the pre-FOO state, not the pre-BAR state.
3. Next, add modules/themes to the mix that do not alter DB schema, but do install config of their own — to test that that too can be cleanly reverted.

This is also blocked by #3407956: [PP-1] Create a config storage checkpoint before applying a recipe, and clean it up afterwards → , which will alter the code in this MR.

I was able to manually test this and I have some confirmation that it does what we intend, based on the situation detailed in #3390919: Create a config storage backend that can set "checkpoints", recording the changes to config that happen in between them → ! Here's what I did:

git clone git@github.com:phenaproxima/recipe-test.git
cd recipe-test
composer i
./vendor/bin/drush si minimal
./vendor/bin/drush cex -y

Then, edit web/recipes/gin-admin-experience/recipe.yml thus:

• Remove the entire actions section
• Remove help from the install list

Now, do this:

cd web
php core/scripts/drupal recipe ./recipes/gin-admin-experience

You'll see something like this:

In InvalidConfigException.php line 21:
                                                                    
  Object(Drupal\Core\Config\Schema\Mapping).dependencies.module.0:  
      Module 'help' is not installed.                               
                                                                    

In RecipeConfigInstaller.php line 60:
                                                                    
  Object(Drupal\Core\Config\Schema\Mapping).dependencies.module.0:  
      Module 'help' is not installed.                               

That's what we'd expect to see. But here's the proof that all changes were rolled back: if you run ../vendor/bin/drush cex, it should tell you that there are no config changes!!

Wim Leers → changed the visibility of the branch 3401867-validate-config-that to hidden.

Wim Leers → changed the visibility of the branch 3401867-validate-config-post-checkpointstorage to hidden.

Wim Leers → changed the visibility of the branch 3401867-validate-config-post-checkpointstorage to active.

Closed/hid https://git.drupalcode.org/project/distributions_recipes/-/merge_request..., created https://git.drupalcode.org/project/distributions_recipes/-/merge_request... instead, because 🐛 Config collections do not trigger configuration events consistently Fixed and #3390919: Create a config storage backend that can set "checkpoints", recording the changes to config that happen in between them → have both landed.

(Also made sure the MR was created correctly this time, so now only 27 commits show up instead of Drupal core commits that we didn't do — MUCH easier to review👍)

So we went from postponed on 2 to postponed on 0 … but AFAICT #3407956: Create a config storage checkpoint before applying a recipe, and clean it up afterwards → is the new blocker here, because that's where we'll add the checkpointing first, without actually doing anything with it just yet.

I think we can say this is, in fact, postponed. It's hard-blocked by #3407956: Create a config storage checkpoint before applying a recipe → .

Annnd, blocker is in!

Updated the issue summary to clarify what will actually happen if a recipe imports invalid config.

A small amount of MR simplification is possible and will make it easier to land this MR, so let's do that? 😊

I think all feedback thus far is addressed. Back to you!

Agreed, this looks great now! 🤩

Overview of the changes:

1. One exception is gone and is replaced by a more generic one …
2. … because the logic that triggered the original exception is no longer owned by Recipes, but by core's existing validation constraints 👍
3. The RecipeCommand class' ::boot() became slightly more resilient …
4. … because now there is automatic rollback of a recipe whenever a config validation error is encountered when applying a recipe 👍
5. All other changes are test coverage, which became quite a bit more complete now 👍

Removed the boot() changes. We need to handle container the container better in \Drupal\Core\Recipe\RecipeRunner::processRecipe too... - it's a hard problem. The solution is probably to pass the kernel object around but I think we should address all of this in one issue and not do anything piecemeal.

Committed and pushed 21216ed8d00 to 11.x and 76055eeffe1 to 10.2.x. Thanks!

This was reverted from the 10.2.x patch

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