Problem/Motivation

This is spun off from #3390919-12: Create a config storage backend that can set "checkpoints", recording the changes to config that happen in between them.

Right now, when a recipe imports config (either from modules or from its own config directory), that config is not validated in any way.

Proposed resolution

RecipeConfigInstaller should validate each piece of config it imports, through the typed config manager, after actually creating it. If there are any violations, an exception should be thrown and the recipe should be rolled back to the backup checkpoint.

Remaining tasks

Implement that, with tests.

User interface changes

None, although a recipe might fail mid-way with a different message/exception.

API changes

Potentially. A new exception will likely be introduced.

Issue fork distributions_recipes-3401867

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

phenaproxima created an issue. See original summary.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue tags: +blocker

This blocks #3401723: Config modified by action plugins should be validated after it is saved.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

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.

phenaproxima opened merge request !58

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Active » Needs review

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

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

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

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Category: Feature request » Task
Priority: Normal » Major

This blocks three issues.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

See MR.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Needs review

Back to review here; if it looks good then I think we might need to postpone this issue on #3390919: Create a config storage backend that can set "checkpoints", recording the changes to config that happen in between them. I tried to explain why in https://git.drupalcode.org/project/distributions_recipes/-/merge_request....

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Title: Validate config that gets imported from modules or the recipe's config directory » [PP-1] Validate config that gets imported from modules or the recipe's config directory
Status: Needs review » Postponed
Issue tags: -blocker, -Needs tests

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:

  1. Do the current check (ensure dependencies look right beforehand), using validateDependencies().
  2. Create all the config, with createConfiguration().
  3. 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: Create a config storage backend that can set "checkpoints", recording the changes to config that happen in between them.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

I was wrong, you were right! 😊

Now eagerly following #3390919: Create a config storage backend that can set "checkpoints", recording the changes to config that happen in between them 😄

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

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: Create a config storage backend that can set "checkpoints", recording the changes to config that happen in between them. 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: Create a config storage backend that can set "checkpoints", recording the changes to config that happen in between them — 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?

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

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!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Postponed » Needs work
Issue tags: +Needs tests
Related issues: +#3405800: Config collections do not trigger configuration events consistently

Understanding this

So this is built on:

  1. * 6cc95f1b6f5286b93198ef36f678a88005045d80 Make core dispatch config events for non-default collections. aka #3405800: Config collections do not trigger configuration events consistently
  2. * 9270b7d97ab43e644942ff13d6548eec49f91934 Add checkpoints support

    aka #3390919: Create a config storage backend that can set "checkpoints", recording the changes to config that happen in between them, 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.
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Title: [PP-1] Validate config that gets imported from modules or the recipe's config directory » [PP-2] Validate config that gets imported from modules or the recipe's config directory
Related issues: +#3407956: Create a config storage checkpoint before applying a recipe

This is also blocked by #3407956: Create a config storage checkpoint before applying a recipe, which will alter the code in this MR.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

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
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’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » wim leers
Issue tags: +Configuration schema, +validation

Wim Leers opened merge request !64

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.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Closed/hid https://git.drupalcode.org/project/distributions_recipes/-/merge_request..., created https://git.drupalcode.org/project/distributions_recipes/-/merge_request... instead, because #3405800: Config collections do not trigger configuration events consistently 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👍)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: [PP-2] Validate config that gets imported from modules or the recipe's config directory » [PP-1] Validate config that gets imported from modules or the recipe's config directory

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

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Postponed

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.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Title: [PP-1] Validate config that gets imported from modules or the recipe's config directory » Validate config that gets imported from modules or the recipe's config directory
Status: Postponed » Needs work

Annnd, blocker is in!

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue summary: View changes

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

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Needs review
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » Unassigned
Status: Needs review » Needs work
Issue tags: -Needs tests

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

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Needs review

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

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

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 👍

alexpott made their first commit to this issue’s fork.

alexpott’s picture

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

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.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

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

  • alexpott committed 76055eef on 10.2.x 
    Issue #3401867 by phenaproxima, Wim Leers, alexpott: Validate config...

  • alexpott committed 21216ed8 on 11.x 
    Issue #3401867 by phenaproxima, Wim Leers, alexpott: Validate config...

  • alexpott committed 0292f8b8 on 10.2.x 
    Revert "Issue #3401867 by phenaproxima, Wim Leers, alexpott: Validate...

  • 4683c884 committed on patch 
    Update recipe 10.2.x patch 0292f8b8 Revert "Issue #3401867 by...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 10.2.x-dev » 10.3.x-dev

This was reverted from the 10.2.x patch

Status: Fixed » Closed (fixed)

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