[PP-2] After a recipe is successfully applied, validate all of the config that was imported or modified by config actions

This is also blocked by #3407956: [PP-1] Create a config storage checkpoint before applying a recipe, and clean it up afterwards → .

#3407956: Create a config storage checkpoint before applying a recipe → is in!

#3401867: Validate config that gets imported from modules or the recipe's config directory → is in!

Last blocker: #3401723: Config modified by action plugins should be validated before it is saved →

And that's fixed too now 👍

I think it's 99% likely that this will run into the exact same problem that @phenaproxima surfaced for #3417835: Convert the Standard install profile into a set of recipes → at https://git.drupalcode.org/project/distributions_recipes/-/merge_request..., and which I proposed a solution for over at #3417835-56: Convert the Standard install profile into a set of recipes → .

IMHO it's reasonable to use this issue to implement that strategy here.

Okay, I'm struggling with how to test this.

With any fully validatable config, it's kind of impossible to put it into an invalid state. As soon as we save it, we validate it, and that throws an exception. How, therefore, can we contrive a situation where the imports and the actions (both of which validate everything they can) somehow fail to catch an invalid config object?

The problem and fix that Wim mentions in #8 are, in fact, the reason why this is critical. From a Slack thread:

Application fails if it runs into invalid config from contrib.

So we have to limit validation to only FullyValidatable config, pronto.

#12: Yup.

#11: I already provided guidance to @narendraR on how to do that. In a single Recipe, first modify a Editor config entity (e.g. change label), second modify the corresponding FilterFormat to disallow the <br> tag. Each individual config action results in a valid config entity. But re-validating the entire set of changes at the end will result in the Editor triggering a validation error because it is no longer in sync with the FilterFormat.

Re #13, I tried to add test in https://git.drupalcode.org/project/distributions_recipes/-/merge_request... but it is not failing as expected.

🤔 That makes no sense. Did you already step through CKEditor 5's "fundamental compatibility" MR with a debugger when executing validation?

Re #15, I tried adding debugger in CKEditor5::validatePair, but that is not getting triggered. May be something is wrong in this MR's core/tests/fixtures/recipes/basic_html_format_editor/recipe.yml

I'll investigate 🕵️

Huh, the test immediately fails for me?

Testing /Users/wim.leers/core/core/tests/Drupal/KernelTests/Core/Recipe
E                                                                   1 / 1 (100%)R

Time: 00:03.229, Memory: 10.00 MB

There was 1 error:

1) Drupal\KernelTests\Core\Recipe\RecipeRunnerTest::testValidationAfterRecipeApplied
Drupal\Core\Field\FieldException: Field 'uid' on entity type 'file' references a target entity type 'user' which does not exist.

/Users/wim.leers/core/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php:171
/Users/wim.leers/core/core/lib/Drupal/Core/Field/BaseFieldDefinition.php:668
/Users/wim.leers/core/core/lib/Drupal/Core/Field/BaseFieldDefinition.php:691
/Users/wim.leers/core/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php:414
/Users/wim.leers/core/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php:975
/Users/wim.leers/core/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php:418
/Users/wim.leers/core/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:1518
/Users/wim.leers/core/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:1592
/Users/wim.leers/core/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:1519
/Users/wim.leers/core/core/lib/Drupal/Core/Entity/EntityTypeListener.php:93
/Users/wim.leers/core/core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManager.php:184
/Users/wim.leers/core/core/lib/Drupal/Core/Extension/ModuleInstaller.php:298
/Users/wim.leers/core/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php:83
/Users/wim.leers/core/core/lib/Drupal/Core/Recipe/RecipeRunner.php:66
/Users/wim.leers/core/core/lib/Drupal/Core/Recipe/RecipeRunner.php:27
/Users/wim.leers/core/core/tests/Drupal/KernelTests/Core/Recipe/RecipeRunnerTest.php:198
/Users/wim.leers/core/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

Are you not seeing the same locally? https://git.drupalcode.org/issue/distributions_recipes-3401925/-/jobs/97... is definitely not showing this, so just pushed a commit to verify that this actually gets executed on GitLab CI…

Huh … https://git.drupalcode.org/issue/distributions_recipes-3401925/-/jobs/97... ← that did fail as expected. But I cannot reproduce this locally, it fails long before it gets to that point. Bizarre. GitLab CI uses PHP 8.1.27, I use … the same exact version. I did a fresh composer install twice, even after manually deleting the vendor directory just in case.

🤪 What is going on here?!

For the test coverage I proposed in #8, we first would need to fix 📌 Deprecate \Drupal\ckeditor5\Plugin\Editor\CKEditor5::validatePair() and `type: ckeditor5_valid_pair__format_and_editor` Active .

I think I've figured out a simpler test scenario that wouldn't require that to land first.

I think I've figured out a simpler test scenario that wouldn't require that to land first.

That simpler scenario is: delete the text format config entity after having updated the text editor config entity.

But, since there's no config action for deleting a config entity, I don't actually see how that's possible after all … 😬

Ways out of this to not block this on 📌 Deprecate \Drupal\ckeditor5\Plugin\Editor\CKEditor5::validatePair() and `type: ckeditor5_valid_pair__format_and_editor` Active :

• add such a "delete" action in a test-only way
• add this without test coverage

😬

best choice today: add the editor.schema.yml changes using a test-only module

Well...this is a core fork, so why not just land this change in editor.schema.yml with a todo to remove it when the appropriate fix(es) are made upstream in core?

#22: because the change in editor.schema.yml would cause other tests to fail — it's too simplistic (see the upstream core issue). Those unrelated test failures (if you run core tests locally) could cause wasted time when debugging.

delete the text format config entity after having updated the text editor config entity.

Tried this, but it doesn't work, because Editors have hard config dependencies on their text formats, which means the editors are deleted when the text format is! (This happens in \Drupal\Core\Config\Entity\ConfigEntityBase::preDelete().)

Since this is tricky to test, and encompasses a bigger fix than the current critical problem turned up in #12, I've decided to split out the critical fix into its own issue, which already has dedicated test coverage: #3425540: Only validate config which is is fully validatable → . I'm de-escalating this one.

I'm very glad to see that #3425540: Only validate config which is fully validatable → was split out.

1. This now needs a reroll. Seeing what remains of the MR would be helpful.
2. 📌 Deprecate \Drupal\ckeditor5\Plugin\Editor\CKEditor5::validatePair() and `type: ckeditor5_valid_pair__format_and_editor` Active might make this obsolete! Might.

Re-rolled the code and changes done as suggested. I am not sure what needs to be done next here as ::testValidationAfterRecipeApplied is still not failing as expected.

Yep, that's what I alluded to in #26.2. Working on that! 👍

tl;dr of what has happened since #26:

1. I've proven at 📌 Deprecate \Drupal\ckeditor5\Plugin\Editor\CKEditor5::validatePair() and `type: ckeditor5_valid_pair__format_and_editor` Active that it's possible to ensure any change to an Editor config entity can indeed trigger the validity of the Editor-FilterFormat pair 👍
2. BUT it is in turn blocked on missing infrastructure in Drupal core: 🐛 Config validation: config entities should get the same validation errors when validated as plain config vs ConfigEntityAdapter Needs review is the blocker.

Next steps

1. We could test the scope of this issue today if there were a "delete config entity" action. But it doesn't, and intentionally so.
2. That means that the only other way to test the scope of this issue is to wait for some other config validation logic to validate across config entity types. The only one that I know of are the validation constraints for ckeditor5_valid_pair__format_and_editor, which are currently only executed by calling CKEditor5::validatePair(). We shouldn't tie Recipes to that (although we temporarily could…), so we should wait for 📌 Deprecate \Drupal\ckeditor5\Plugin\Editor\CKEditor5::validatePair() and `type: ckeditor5_valid_pair__format_and_editor` Active , because then a simple ConfigEntityAdapter::createFromEntity(Editor::load('basic_html'))->validate() (which we already call) will trigger that same validation logic without the need for calling custom logic 👍
3. Consequently, this issue is blocked on not one but two core issues 😅

Finally: I did write in #26.2 that this issue might become obsolete. That is still possible, but the 2 blocking issues alone are insufficient.
For that to happen, we'd need to add a new validation constraint to Drupal core: one that is applied to all config entities and verifies that all dependents (other config entities depending on the config entity being modified) also remain valid. Both direct ones (e.g. editor.editor.basic_html depends on filter.format.basic_html) and indirect ones (e.g. an entity view display depending on a text field, and that text field having filter.format.basic_html configured as the only allowed format). If that were to happen, then only validating the actually modified config entities would be sufficient (which Recipes already does), because all interdependencies would be validated too 👍
The risk/danger/complexity lies in the fact that this risks becoming a major performance bottleneck, because for some things there could be MANY indirect dependencies that would need to be revalidated…

This is marked as Postponed. Updating the status.

moved to Drupal Core