Problem/Motivation

Discovered in #3364108: Configuration schema & required keys.
#2663410: TypedConfigManager::_getDefinitionWithReplacements() incorrectly replaces generic definition introduced a regression: it introduced a bug that made TypedConfigManager sensitive to cache pollution. In the
case of hitting the $type = "$type||$sub_type"; edge case, 2 calls to getDefinitionWithReplacements() happen:

#2663410

Steps to reproduce

  1. buildDataDefinition() calls it WITH the $replace parameter
  2. getDefinition() calls it WITHOUT the $replace parameter

Calling getDefinition() causes the computed type to be overwritten.

Proposed resolution

Cache pollution happens when getDefinitionWithReplacements() is called without the $replacements parameter, so wrapping the defintion update into a condition can solve the problem.

Remaining tasks

Test coverage.

User interface changes

None.

API changes

None.

Data model changes

N/A

Release notes snippet

N/A

Issue fork drupal-3400181

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

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue summary: View changes

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Priority: Normal » Major
Issue tags: +Experience Builder, +Contributed project blocker
Related issues: +#3568264: Update code component props schema to support 'array' type

Canvas adopted a config schema type that consistently ran into this problem, in #3568264: Update code component props schema to support 'array' type.

diff --git a/core/lib/Drupal/Core/Config/TypedConfigManager.php b/core/lib/Drupal/Core/Config/TypedConfigManager.php
index 99fdb0d5978..8e7b9921631 100644
--- a/core/lib/Drupal/Core/Config/TypedConfigManager.php
+++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
@@ -298,7 +303,9 @@ protected function getDefinitionWithReplacements($base_plugin_id, array $replace
       }
       // Unset type so we try the merge only once per type.
       unset($definition['type']);
-      $this->definitions[$type] = $definition;
+      if (!empty($replacements)) {
+        $this->definitions[$type] = $definition;
+      }
     }
     // Add type and default definition class.
     $definition += [

— #3568264-13: Update code component props schema to support 'array' type

Consequently, it now has a built-in override of this service to work around the bug.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Related issues: +#3548299: Missing schema on nested config

Added #3548299: Missing schema on nested config as potentially related

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Assigned: Unassigned » huzooka

huzooka opened merge request !15886

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Assigned: huzooka » Unassigned
Status: Active » Needs review

If this will be fixed by the MR I just created, I propose giving credit additionally to:
- dimilias for his detailed report at #3548299: Missing schema on nested config (he was also very close);
- Besides claudiu.cristea, also herved for connecting dots;
- alexpott for the test coverage.

I don't think we need any additional test coverage; imho the definition cache clears I removed from the ConfigSchemaTest should be enough.

alorenc’s picture

alorenc
Location Wolsztyn, 🇵🇱
commented
Assigned: Unassigned » alorenc
alorenc’s picture

alorenc
Location Wolsztyn, 🇵🇱
commented

RTBC
A minimal, it prevents TypedConfigManager from caching schema definitions resolved without replacements.

alorenc’s picture

alorenc
Location Wolsztyn, 🇵🇱
commented
Assigned: alorenc » Unassigned
Status: Needs review » Reviewed & tested by the community
quietone’s picture

quietone commented
Title: Follow-up for #2663410: calling TypedConfigManager::getDefinition() causes cache pollution » [regression] calling TypedConfigManager::getDefinition() causes cache pollution
Issue summary: View changes

This comment states this is fixing a regression, therefor updating title per the special issue titles.

godotislate’s picture

godotislate
he/him
commented
Issue tags: +Needs issue summary update

Can someone update the Proposed Resolution in the IS to match the approach taken in the MR. At a glance, they are not the same.

Assigned credit, including note form #8.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Issue summary: View changes
Issue tags: -Needs issue summary update
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Issue tags: -Needs tests

As per #8, we don't need tests. We're already covered by removing the cache clears from existing tests