Issue #3478332 by phenaproxima, nicxvan, thejimbirch, alexpott: Add a way to...

Issue #3478332 by phenaproxima, nicxvan, thejimbirch, alexpott: Add a way to prevent recipes' imported config from being compared too strictly to active config

core/lib/Drupal/Core/Recipe/RecipeExtensionConfigStorage.php → core/lib/Drupal/Core/Recipe/AllowListConfigStorage.php

@@ -4,118 +4,104 @@
 
 namespace Drupal\Core\Recipe;
 
-use Drupal\Core\Config\FileStorage;
 use Drupal\Core\Config\StorageInterface;
 
 /**
- * Allows the recipe to select configuration from the module.
+ * A read-only storage wrapper that only allows access to certain config names.
  *
  * @internal
  *   This API is experimental.
  */
-final class RecipeExtensionConfigStorage implements StorageInterface {
-
-  protected readonly StorageInterface $storage;
+final class AllowListConfigStorage implements StorageInterface {
 
   /**
-   * @param string $extensionPath
-   *   The path extension to read configuration from
-   * @param array $configNames
-   *   The list of config to read from the extension. An empty array means all
-   *   configuration.
-   * @param string $collection
-   *   (optional) The collection to store configuration in. Defaults to the
-   *   default collection.
+   * @param \Drupal\Core\Config\StorageInterface $decorated
+   *   A config storage backend to wrap around.
+   * @param string[] $allowList
+   *   A list of config names. Only these names will be visible, or readable,
+   *   by this storage. Cannot be empty.
    */
-  public function __construct(protected readonly string $extensionPath, protected readonly array $configNames, protected readonly string $collection = StorageInterface::DEFAULT_COLLECTION) {
-    $this->storage = new RecipeConfigStorageWrapper(
-      new FileStorage($this->extensionPath . '/config/install', $this->collection),
-      new FileStorage($this->extensionPath . '/config/optional', $this->collection),
-      $collection
-    );
+  public function __construct(
+    private readonly StorageInterface $decorated,
+    private readonly array $allowList,
+  ) {
+    if (empty($allowList)) {
+      throw new \LogicException('AllowListConfigStorage cannot be constructed with an empty allow list.');
+    }
   }
 
   /**
    * {@inheritdoc}
    */
   public function exists($name): bool {
-    if (!empty($this->configNames) && !in_array($name, $this->configNames, TRUE)) {
-      return FALSE;
+    if (in_array($name, $this->allowList, TRUE)) {
+      return $this->decorated->exists($name);
     }
-    return $this->storage->exists($name);
+    return FALSE;
   }
 
   /**
    * {@inheritdoc}
    */
-  public function read($name): array|bool {
-    if (!empty($this->configNames) && !in_array($name, $this->configNames, TRUE)) {
-      return FALSE;
-    }
-    return $this->storage->read($name);
+  public function read($name): array|false {
+    return $this->exists($name) ? $this->decorated->read($name) : FALSE;
   }
 
   /**
    * {@inheritdoc}
    */
   public function readMultiple(array $names): array {
-    if (!empty($this->configNames)) {
-      $names = array_intersect($this->configNames, $names);
-    }
-    return $this->storage->readMultiple($names);
+    $names = array_intersect($names, $this->allowList);
+    return $this->decorated->readMultiple($names);
   }
 
   /**
    * {@inheritdoc}
    */
-  public function write($name, array $data): bool {
-    throw new \BadMethodCallException();
+  public function write($name, array $data): never {
+    throw new \BadMethodCallException('This storage is read-only.');
   }
 
   /**
    * {@inheritdoc}
    */
-  public function delete($name): bool {
-    throw new \BadMethodCallException();
+  public function delete($name): never {
+    throw new \BadMethodCallException('This storage is read-only.');
   }
 
   /**
    * {@inheritdoc}
    */
-  public function rename($name, $new_name): bool {
-    throw new \BadMethodCallException();
+  public function rename($name, $new_name): never {
+    throw new \BadMethodCallException('This storage is read-only.');
   }
 
   /**
    * {@inheritdoc}
    */
   public function encode($data): string {
-    throw new \BadMethodCallException();
+    return $this->decorated->encode($data);
   }
 
   /**
    * {@inheritdoc}
    */
   public function decode($raw): array {
-    throw new \BadMethodCallException();
+    return $this->decorated->decode($raw);
   }
 
   /**
    * {@inheritdoc}
    */
   public function listAll($prefix = ''): array {
-    $names = $this->storage->listAll($prefix);
-    if (!empty($this->configNames)) {
-      $names = array_intersect($this->configNames, $names);
-    }
-    return $names;
+    return array_intersect($this->decorated->listAll($prefix), $this->allowList);
   }
 
   /**
    * {@inheritdoc}
    */
-  public function deleteAll($prefix = ''): bool {
-    throw new \BadMethodCallException();
+  public function deleteAll($prefix = ''): never {
+    throw new \BadMethodCallException('This storage is read-only.');
   }
 
   /**
@@ -123,9 +109,8 @@ public function deleteAll($prefix = ''): bool {
    */
   public function createCollection($collection): static {
     return new static(
-      $this->extensionPath,
-      $this->configNames,
-      $collection
+      $this->decorated->createCollection($collection),
+      $this->allowList,
     );
   }
 
@@ -133,14 +118,14 @@ public function createCollection($collection): static {
    * {@inheritdoc}
    */
   public function getAllCollectionNames(): array {
-    return $this->storage->getAllCollectionNames();
+    return $this->decorated->getAllCollectionNames();
   }
 
   /**
    * {@inheritdoc}
    */
   public function getCollectionName(): string {
-    return $this->collection;
+    return $this->decorated->getCollectionName();
   }
 
 }

core/lib/Drupal/Core/Recipe/ConfigConfigurator.php

@@ -5,6 +5,7 @@
 namespace Drupal\Core\Recipe;
 
 use Drupal\Core\Config\FileStorage;
+use Drupal\Core\Config\NullStorage;
 use Drupal\Core\Config\StorageInterface;
 
 /**
@@ -15,6 +16,8 @@ final class ConfigConfigurator {
 
   public readonly ?string $recipeConfigDirectory;
 
+  private readonly bool|array $strict;
+
   /**
    * @param array $config
    *   Config options for a recipe.
@@ -25,8 +28,20 @@ final class ConfigConfigurator {
    */
   public function __construct(public readonly array $config, string $recipe_directory, StorageInterface $active_configuration) {
     $this->recipeConfigDirectory = is_dir($recipe_directory . '/config') ? $recipe_directory . '/config' : NULL;
+    // @todo Consider defaulting this to FALSE in https://drupal.org/i/3478669.
+    $this->strict = $config['strict'] ?? TRUE;
+
     $recipe_storage = $this->getConfigStorage();
-    foreach ($recipe_storage->listAll() as $config_name) {
+    if ($this->strict === TRUE) {
+      $strict_list = $recipe_storage->listAll();
+    }
+    else {
+      $strict_list = $this->strict ?: [];
+    }
+
+    // Everything in the strict list needs to be identical in the recipe and
+    // active storage.
+    foreach ($strict_list as $config_name) {
       if ($active_data = $active_configuration->read($config_name)) {
         // @todo https://www.drupal.org/i/3439714 Investigate if there is any
         //   generic code in core for this.
@@ -90,10 +105,10 @@ public function getConfigStorage(): StorageInterface {
       $module_list = \Drupal::service('extension.list.module');
       /** @var \Drupal\Core\Extension\ThemeExtensionList $theme_list */
       $theme_list = \Drupal::service('extension.list.theme');
-      foreach ($this->config['import'] as $extension => $config) {
+      foreach ($this->config['import'] as $extension => $names) {
         // If the recipe explicitly does not want to import any config from this
         // extension, skip it.
-        if ($config === NULL) {
+        if ($names === NULL) {
           continue;
         }
         $path = match (TRUE) {
@@ -101,12 +116,36 @@ public function getConfigStorage(): StorageInterface {
           $theme_list->exists($extension) => $theme_list->getPath($extension),
           default => throw new \RuntimeException("$extension is not a theme or module")
         };
-        $config = $config === '*' ? [] : $config;
-        $storages[] = new RecipeExtensionConfigStorage($path, $config);
+
+        $storage = new RecipeConfigStorageWrapper(
+          new FileStorage($path . '/config/install'),
+          new FileStorage($path . '/config/optional'),
+        );
+        // If we get here, $names is either '*', or a list of config names
+        // provided by the current extension. In the latter case, we only want
+        // to import the config that is in the list, so use an
+        // AllowListConfigStorage to filter out the extension's other config.
+        if ($names && is_array($names)) {
+          $storage = new AllowListConfigStorage($storage, $names);
+        }
+        $storages[] = $storage;
       }
     }
+    $storage = RecipeConfigStorageWrapper::createStorageFromArray($storages);
+
+    if ($this->strict) {
+      return $storage;
+    }
+    // If we're not in strict mode, we only want to import config that doesn't
+    // exist yet in active storage.
 
-    return RecipeConfigStorageWrapper::createStorageFromArray($storages);
+    $names = array_diff(
+      $storage->listAll(),
+      \Drupal::service('config.storage')->listAll(),
+    );
+    return $names
+      ? new AllowListConfigStorage($storage, $names)
+      : new NullStorage();
   }
 
   /**

core/lib/Drupal/Core/Recipe/Recipe.php

@@ -257,6 +257,16 @@ private static function parse(string $file): array {
               ]),
             ]),
           ]),
+          'strict' => new Optional([
+            new AtLeastOneOf([
+              new Type('boolean'),
+              new All([
+                new Type('string'),
+                new NotBlank(),
+                new Regex('/^.+\./'),
+              ]),
+            ], message: 'This value must be a boolean, or a list of config names.', includeInternalMessages: FALSE),
+          ]),
           'actions' => new Optional([
             new All([
               new Type('array'),

core/recipes/administrator_role/config/user.role.administrator.yml

+langcode: en
+status: true
+dependencies: {  }
+id: administrator
+label: Administrator
+weight: 3
+is_admin: true
+permissions: {  }

core/recipes/administrator_role/recipe.yml

@@ -2,11 +2,5 @@ name: 'Administrator role'
 description: 'Provides the Administrator role.'
 type: 'User role'
 config:
-  actions:
-    user.role.administrator:
-      # If this role already exists, then this action has no effect. If it doesn't exist, we'll create it with the following values.
-      createIfNotExists:
-        id: administrator
-        label: Administrator
-        weight: 3
-        is_admin: true
+  # If the administrator role already exists, we don't really care what it looks like.
+  strict: false

core/recipes/content_editor_role/config/user.role.content_editor.yml

+langcode: en
+status: true
+dependencies: {  }
+id: content_editor
+label: 'Content editor'
+weight: 2
+is_admin: false
+permissions: {  }

core/recipes/content_editor_role/recipe.yml

@@ -2,14 +2,10 @@ name: 'Content editor role'
 description: 'Provides the Content editor role.'
 type: 'User role'
 config:
+  # If the content_editor role already exists, we don't really care what it looks like.
+  strict: false
   actions:
     user.role.content_editor:
-      # If this role already exists, then this action has no effect. If it doesn't exist, we'll create it with the following values.
-      createIfNotExists:
-        id: content_editor
-        label: 'Content editor'
-        weight: 2
-        is_admin: false
-        permissions:
-          - 'access administration pages'
-          - 'view own unpublished content'
+      grantPermissions:
+        - 'access administration pages'
+        - 'view own unpublished content'

core/tests/Drupal/FunctionalTests/Core/Recipe/RecipeTestTrait.php

@@ -97,4 +97,22 @@ protected function applyRecipe(string $path, int $expected_exit_code = 0, array
     return $process;
   }
 
+  /**
+   * Alters an existing recipe.
+   *
+   * @param string $path
+   *   The recipe directory path.
+   * @param callable $alter
+   *   A function that will receive the decoded contents of recipe.yml as an
+   *   array. This should returned a modified array to be written to recipe.yml.
+   */
+  protected function alterRecipe(string $path, callable $alter): void {
+    $file = $path . '/recipe.yml';
+    $this->assertFileExists($file);
+    $contents = file_get_contents($file);
+    $contents = Yaml::decode($contents);
+    $contents = $alter($contents);
+    file_put_contents($file, Yaml::encode($contents));
+  }
+
 }

core/tests/Drupal/KernelTests/Core/Recipe/ConfigConfiguratorTest.php

@@ -5,8 +5,15 @@
 namespace Drupal\KernelTests\Core\Recipe;
 
 use Drupal\Component\Serialization\Yaml;
+use Drupal\Core\Entity\EntityDisplayRepositoryInterface;
+use Drupal\Core\Field\Entity\BaseFieldOverride;
 use Drupal\Core\Recipe\Recipe;
+use Drupal\Core\Recipe\RecipePreExistingConfigException;
+use Drupal\Core\Recipe\RecipeRunner;
+use Drupal\FunctionalTests\Core\Recipe\RecipeTestTrait;
 use Drupal\KernelTests\KernelTestBase;
+use Drupal\node\Entity\NodeType;
+use org\bovigo\vfs\vfsStream;
 
 /**
  * @covers \Drupal\Core\Recipe\ConfigConfigurator
@@ -14,6 +21,8 @@
  */
 class ConfigConfiguratorTest extends KernelTestBase {
 
+  use RecipeTestTrait;
+
   public function testExistingConfigWithKeysInDifferentOrder(): void {
     $recipe_dir = uniqid('public://recipe_test_');
     mkdir($recipe_dir . '/config', recursive: TRUE);
@@ -43,4 +52,112 @@ public function testExistingConfigWithKeysInDifferentOrder(): void {
     $this->assertInstanceOf(Recipe::class, Recipe::createFromDirectory($recipe_dir));
   }
 
+  /**
+   * @testWith [false]
+   *   [[]]
+   */
+  public function testExistingConfigIsIgnoredInLenientMode(array|false $strict_value): void {
+    $recipe = Recipe::createFromDirectory('core/recipes/page_content_type');
+    $this->assertNotEmpty($recipe->config->getConfigStorage()->listAll());
+    RecipeRunner::processRecipe($recipe);
+
+    // Clone the recipe into the virtual file system, and opt the clone into
+    // lenient mode.
+    $recipe_dir = $this->cloneRecipe($recipe->path);
+    $this->alterRecipe($recipe_dir, function (array $data) use ($strict_value): array {
+      $data['config']['strict'] = $strict_value;
+      return $data;
+    });
+    // The recipe should not have any config to install; all of it already
+    // exists.
+    $recipe = Recipe::createFromDirectory($recipe_dir);
+    $this->assertEmpty($recipe->config->getConfigStorage()->listAll());
+  }
+
+  public function testSelectiveStrictness(): void {
+    $recipe = Recipe::createFromDirectory('core/recipes/page_content_type');
+    RecipeRunner::processRecipe($recipe);
+
+    $node_type = NodeType::load('page');
+    $original_description = $node_type->getDescription();
+    $node_type->set('description', 'And now for something completely different.')
+      ->save();
+
+    $form_display = $this->container->get(EntityDisplayRepositoryInterface::class)
+      ->getFormDisplay('node', 'page');
+    $this->assertFalse($form_display->isNew());
+    $this->assertIsArray($form_display->getComponent('uid'));
+    $form_display->removeComponent('uid')->save();
+
+    // Delete something that the recipe provides, so we can be sure it is
+    // recreated if it's not in the strict list.
+    BaseFieldOverride::loadByName('node', 'page', 'promote')->delete();
+
+    // Clone the recipe into the virtual file system, and opt only the node
+    // type into strict mode.
+    $clone_dir = $this->cloneRecipe($recipe->path);
+    $this->alterRecipe($clone_dir, function (array $data): array {
+      $data['config']['strict'] = ['node.type.page'];
+      return $data;
+    });
+    // If we try to instantiate this recipe, we should an exception.
+    try {
+      Recipe::createFromDirectory($clone_dir);
+      $this->fail('Expected an exception but none was thrown.');
+    }
+    catch (RecipePreExistingConfigException $e) {
+      $this->assertSame("The configuration 'node.type.page' exists already and does not match the recipe's configuration", $e->getMessage());
+    }
+
+    // If we restore the node type's original description, we should no longer
+    // get an error if we try to instantiate the altered recipe, even though the
+    // form display is still different from what's in the recipe.
+    NodeType::load('page')
+      ->set('description', $original_description)
+      ->save();
+
+    $recipe = Recipe::createFromDirectory($clone_dir);
+    RecipeRunner::processRecipe($recipe);
+
+    // Make certain that our change to the form display is still there.
+    $component = $this->container->get(EntityDisplayRepositoryInterface::class)
+      ->getFormDisplay('node', 'page')
+      ->getComponent('uid');
+    $this->assertNull($component);
+
+    // The thing we deleted should have been recreated.
+    $this->assertInstanceOf(BaseFieldOverride::class, BaseFieldOverride::loadByName('node', 'page', 'promote'));
+  }
+
+  public function testFullStrictness(): void {
+    $recipe = Recipe::createFromDirectory('core/recipes/page_content_type');
+    RecipeRunner::processRecipe($recipe);
+
+    NodeType::load('page')
+      ->set('description', 'And now for something completely different.')
+      ->save();
+
+    // Clone the recipe into the virtual file system, and opt all of its config
+    // into strict mode.
+    $clone_dir = $this->cloneRecipe($recipe->path);
+    $this->alterRecipe($clone_dir, function (array $data): array {
+      $data['config']['strict'] = TRUE;
+      return $data;
+    });
+    // If we try to instantiate this recipe, we should an exception.
+    $this->expectException(RecipePreExistingConfigException::class);
+    $this->expectExceptionMessage("The configuration 'node.type.page' exists already and does not match the recipe's configuration");
+    Recipe::createFromDirectory($clone_dir);
+  }
+
+  private function cloneRecipe(string $original_dir): string {
+    // Clone the recipe into the virtual file system.
+    $name = uniqid();
+    $clone_dir = $this->vfsRoot->url() . '/' . $name;
+    mkdir($clone_dir);
+    $clone_dir = $this->vfsRoot->getChild($name);
+    vfsStream::copyFromFileSystem($original_dir, $clone_dir);
+    return $clone_dir->url();
+  }
+
 }

core/tests/Drupal/KernelTests/Core/Recipe/RecipeValidationTest.php

@@ -258,6 +258,58 @@ public static function providerRecipeValidation(): iterable {
         '[config][import][0]' => ['This value should satisfy at least one of the following constraints: [1] This value should be identical to string "*". [2] Each element of this collection should satisfy its own set of constraints.'],
       ],
     ];
+    yield 'config strict is not a boolean or array' => [
+      <<<YAML
+name: Invalid strict flag
+config:
+  strict: 40
+YAML,
+      [
+        '[config][strict]' => ['This value must be a boolean, or a list of config names.'],
+      ],
+    ];
+    yield 'config strict is an array of not-strings' => [
+      <<<YAML
+name: Invalid item in strict list
+config:
+  strict:
+    - 40
+YAML,
+      [
+        '[config][strict]' => ['This value must be a boolean, or a list of config names.'],
+      ],
+    ];
+    yield 'config strict list contains blank strings' => [
+      <<<YAML
+name: Invalid item in strict list
+config:
+  strict:
+    - ''
+YAML,
+      [
+        '[config][strict]' => ['This value must be a boolean, or a list of config names.'],
+      ],
+    ];
+    yield 'config strict list item does not have a period' => [
+      <<<YAML
+name: Invalid item in strict list
+config:
+  strict:
+    - 'something'
+YAML,
+      [
+        '[config][strict]' => ['This value must be a boolean, or a list of config names.'],
+      ],
+    ];
+    yield 'valid strict list' => [
+      <<<YAML
+name: Valid strict list
+config:
+  strict:
+    - system.menu.foo
+YAML,
+      NULL,
+    ];
     yield 'config actions list is valid' => [
       <<<YAML
 name: 'Correct config actions list'