Robustly validate the structure of recipe.yml

First: WOW! 🤩

Second: it doesn't quite work perfectly yet:

Drupal\KernelTests\Core\Recipe\RecipeRunnerTest::testConfigFromModule
Drupal\Core\Recipe\RecipeFileException: Array[config][import][0]:
    This field is missing. (code 2fa2158c-2a7f-484b-98aa-975522539ff8)
Array[config][import][config_test]:
    This field was not expected. (code 7703c766-b5d5-4cef-ace7-ae0dd82304e9)

… so we'll need to adjust what you have in there so far. But it's a superb start!

The beauty of this approach is that we can reuse the very same validation constraints that we're already using for config 👍

Validation expanded to confirm this works

1. For example, the values under install should not just be non-empty strings. They should conform to what Drupal considers valid extension names. For that, #3324150: Add validation constraints to config_entity.dependencies added the ExtensionName constraint 😄

   This was wrong, because due to #3311111: Support modern syntax for defining module dependencies, syntax like drupal:forum are supported, to allow specifying either drupal:forum or forum:forum.

2. recipes ✅ The ExtensionName constraint can be used for the values under this key — done 👍
3. name ✅ Matched the constraints used for type: label in Drupal core

Its now back to the same set of failures as before I started contributing. I'll let @phenaproxima figure out how to deal with the remaining things 🤓

Importance for recipe authoring to get precise feedback

Since the DX of authoring a new recipe is essentially 100% manual YAML construction today, and it will be for a good while to come, I think it's crucial that we have test coverage proving that every small mistake a recipe author might make is in fact caught by validation.

I just pushed a first test case for this.

Prior art/pattern to copy to thoroughly test this?

I have some prior art/experience with this very problem, because I faced the same challenge for CKEditor 5 plugin definitions. However, that one is far more complicated because it needs to mock modules. Since validating a recipe happens entirely outside the context of a running Drupal site, it'll be far simpler here. What matters IMHO is to not have dozens of recipe.yml files in the codebase, because that'll be chaotic very quickly. I think it'd be easier to have them defined in the test provider itself.

See \Drupal\Tests\ckeditor5\Kernel\CKEditor5PluginManagerTest::providerTestInvalidPluginDefinitions() for an example. (Although the value is arguably less here, since there's only a YAML file for each test case, whereas for CKEditor 5 you'd need the entire module directory structure + *.info.yml file.)

@alexpott: would you prefer actual YAML files on disk, like in the first test case I added, or would you prefer the approach taken in CKEditor 5?

What matters IMHO is to not have dozens of recipe.yml files in the codebase, because that'll be chaotic very quickly. I think it'd be easier to have them defined in the test provider itself.

Are you talking about managing recipe.yml files in different places or in the core/tests/fixtures/recipes folder?

I like the idea of having actual recipes in there to provide examples in the code base, not just in documentation places.

Yes, correct recipes make sense to have actual Recipe YAML files.

In this case, we're working to detect invalid Recipe YAML files. Those I'd argue should have actual YAML files on disk. 😊

Oh and this actually resolved

// @todo validate structure of $config['import'] and $config['actions'].

in \Drupal\Core\Recipe\ConfigConfigurator::__construct() too! 🤩

Before going much further here, I'd like to get feedback from @alexpott.

I really like that we are reusing the symfony validator to ensure that everything is correct, I think the approach here is good. We will of course need more/better tests.

@phenaproxima++ for #12.

This is crucial for recipe creators's usability/DX (whichever you want to call it).

One issue which I found while working on #3417835: Convert the Standard install profile into a set of recipes is adding wrong key to config in recipe.yml. I think currently config expects config:import or/and config:actions and any other key suppose config:install does nothing. So should we validate that config should not have any other key except 2 defined?

Yes, we should. Any and every guardrail we can add that would make a problem explicit (rather than the infrastructure silently accepting it and needing a debugger to figure out) is worth adding 👍

In continuation to #14, recipes keys are name, description, type, recipes, install, config and content and defining any other key should give validation error.

#16: the MR already has validation for all those 😄 (and #14's suggestion too btw) — we just need to finish what @phenaproxima started! 🤞

@Wim Leers, I tried to add the validation constraint, but it is not getting triggered. I think something is not correct in

      'config' => new Optional([
        new Collection([
          'import' => new Optional([
            new Required([
              new Collection([
                new All([
                  new Type('string'),
                  new NotBlank(),
                  new Regex('/^.+\./'),
                  new ConfigImportKeyExistsConstraint(),
                ]),
              ]),
            ]),
          ]),
          'actions' => new Optional([
            new Type('array'),
          ]),
        ]),
      ]),

That's totally possible! What did stepping through the execution with a debugger reveal?

I added ConfigImportKeyExistsConstraint to 'install' and it is working now 😬. Not sure if this is the correct place though.
If this seems correct then we might expand ConfigImportKeyExistsConstraint to validate other things like whether to import some, all, or none of the module's config.

TBH, I don't know if recipes support config:import none.

I think there's plenty of things that can continue here? #3421197: It is impossible for recipes to depend on recipes outside of their own directory may block commit of this issue, but there's lots of work we can already do here! 😊

This is definitely moving in the right direction 👍 Did another review round to keep this going 😊

This is definitely going in the right direction — the only thing still missing here is more test cases! 👍

This is now looking very close! 😄

Several follow-ups have been identified — let's get those created and let's make this MR point to them. 50% of the remaining work is creating those follow-ups, 50% are for small remarks.

#3421197: It is impossible for recipes to depend on recipes outside of their own directory landed!

There's a few threads from my feedback 2 days ago that are not yet addressed, but then again @phenaproxima is working on it as we speak! 😄

It's VERY exciting to see that this is unblocked and nearly ready 🤩

Looks wonderful! We can nitpick exact validation messages, but I'd rather not do that here, and do it later, when Recipes is further along. Right now, this is a HUGE improvement over the status quo, and will help authors of future recipes enormously! 👏🤩

A few follow-ups have already been created (thanks @phenaproxima):

• #3423526: Validate that every config entity listed in the config.actions section of recipe.yml belongs to an extension that is installed, or will be by the recipe or one of its dependencies
• #3423523: Test that recipe runner fails and rolls back if a recipe tries to use a non-existent config action

Left a big comment about :: validateExtensionHasImportEntry() - I think the best way forward is to move that part of the validation to a follow-up.

That now extracted into #3424603: Validate that extension listed in the config.import section of recipe.yml has option to bypass all config of that extension 👍

Committed and pushed fa6b83c3727 to 11.x and e902f1c4937 to 10.2.x. Thanks!

Eh … what's going on here? 😨

This broke 10.2.x so I reverted and created 10.3.x :) so this is on 10.3.x and 11.x and 10.2.x is now stuck in time. I should have opened 10.3.x when core did.