Problem/Motivation
Due to #3361534: KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate in core, some very eager contrib modules may already start seeing test failures like this (reported by @jibran over at #3361534-90: KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate!):
1) Drupal\Tests\dynamic_entity_reference\Kernel\DynamicEntityReferenceTestViewsTest::testDefaultConfig
There should be no errors in configuration 'views.view.test_dynamic_entity_reference_entity_test_mul_view'. Errors:
Schema key 0 failed with: [dependencies.module.0] Module 'entity_test' is not installed.
Failed asserting that Array &0 (
0 => '[dependencies.module.0] Module 'entity_test' is not installed.'
) is true.
⚠️ That should not happen, because that would imply the introduction of config validation in Drupal 10.2 introduces significant disruption.
Steps to reproduce
-
cd DRUPAL_PROJECT_ROOT cd modules git clone https://git.drupalcode.org/project/dynamic_entity_reference.git cd dynamic_entity_reference git checkout 3.x # This is the last commit at the time this issue was opened. git reset --hard 15c0f2ebfc69de604927966a1eb291e1fe073a1d # Copy the diff at #3361534-90: KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate, including the trailing blank line. pbpaste | git apply -3v - Run
Drupal\Tests\dynamic_entity_reference\Kernel\DynamicEntityReferenceTestViewsTest - You'll see:
1) Drupal\Tests\dynamic_entity_reference\Kernel\DynamicEntityReferenceTestViewsTest::testDefaultConfig There should be no errors in configuration 'views.view.test_dynamic_entity_reference_entity_test_mul_view'. Errors: Schema key 0 failed with: [dependencies.module.0] Module 'entity_test' is not installed. Failed asserting that Array &0 ( 0 => '[dependencies.module.0] Module 'entity_test' is not installed.' ) is true.
Proposed resolution
- Convert any validation errors in the case of running a contrib test to deprecation notices instead.
- Update the existing CR at https://www.drupal.org/node/3364109 to show
Expected result after doing this for the steps to reproduce above:
Testing /Users/wim.leers/core/modules/contrib/dynamic_entity_reference/tests/src/Kernel
. 1 / 1 (100%)
Time: 00:01.713, Memory: 10.00 MB
OK (1 test, 5 assertions)
Remaining self deprecation notices (5)
1x: The 'views.view.test_dynamic_entity_reference_entity_test_mul_view' configuration contains validation errors. Invalid config is deprecated in drupal:10.2.0 and will be required to be valid in drupal:11.0.0. The following validation errors were found:
- [dependencies.module.0] Module 'entity_test' is not installed.
1x in DynamicEntityReferenceTestViewsTest::testDefaultConfig from Drupal\Tests\dynamic_entity_reference\Kernel
1x: The 'views.view.test_dynamic_entity_reference_entity_test_view' configuration contains validation errors. Invalid config is deprecated in drupal:10.2.0 and will be required to be valid in drupal:11.0.0. The following validation errors were found:
- [dependencies.module.0] Module 'entity_test' is not installed.
1x in DynamicEntityReferenceTestViewsTest::testDefaultConfig from Drupal\Tests\dynamic_entity_reference\Kernel
1x: The 'views.view.test_dynamic_entity_reference_mul_entity_test_mul_view' configuration contains validation errors. Invalid config is deprecated in drupal:10.2.0 and will be required to be valid in drupal:11.0.0. The following validation errors were found:
- [dependencies.module.0] Module 'entity_test' is not installed.
1x in DynamicEntityReferenceTestViewsTest::testDefaultConfig from Drupal\Tests\dynamic_entity_reference\Kernel
1x: The 'views.view.test_dynamic_entity_reference_mul_entity_test_view' configuration contains validation errors. Invalid config is deprecated in drupal:10.2.0 and will be required to be valid in drupal:11.0.0. The following validation errors were found:
- [dependencies.module.0] Module 'entity_test' is not installed.
1x in DynamicEntityReferenceTestViewsTest::testDefaultConfig from Drupal\Tests\dynamic_entity_reference\Kernel
1x: The 'views.view.test_dynamic_entity_reference_entity_test_rev_view' configuration contains validation errors. Invalid config is deprecated in drupal:10.2.0 and will be required to be valid in drupal:11.0.0. The following validation errors were found:
- [dependencies.module.0] Module 'entity_test' is not installed.
1x in DynamicEntityReferenceTestViewsTest::testDefaultConfig from Drupal\Tests\dynamic_entity_reference\Kernel
Process finished with exit code 1
Remaining tasks
- ✅
Working MR - ✅
Reviews→ #9 + #10 - Once approved or committed by a core committer:
- ✅
tweak the release notes snippet in #3361534 - ✅
move the10.2.0 release highlights
tag to #3361534, and remove it here - ✅
update the CR for #3361534 (i.e. https://www.drupal.org/node/3362879) to explain contrib modules' tests will start seeing deprecation notices rather than test failures, and add this issue as a related issue
- ✅
User interface changes
None.
API changes
None.
Data model changes
None.
Release notes snippet
Issue fork drupal-3379899
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:
- 3379899-config-validation-contrib
changes, plain diff MR !4563
Comments
Comment #3
wim leersI marked this because I first thought this should be committed after #3364109: Configuration schema & required values: add test coverage for `nullable: true` validation support. But I just realized that is not necessary, the two can land in whichever order. 👍
Also crediting @jibran explicitly for surfacing this so incredibly early over at #3361534-90: KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate! 🤯
Comment #5
wim leersThis cannot be tested automatically, only manually. Otherwise, we'd need a custom mechanism to detect contrib vs core extensions.
The steps to reproduce in the issue summary are how this can be manually tested.
Comment #6
wim leersComment #7
wim leersThis is now a blocker for #3341682 per @catch at #3341682-56: New config schema data type: `required_label`.
Comment #8
phenaproximaI followed the steps to test this, and confirmed that before this patch, the Dynamic Entity Reference test fails, and passes (with deprecations) after. So, works exactly as advertised!
Comment #9
phenaproximaPatch looks great, just a couple of questions.
Comment #10
borisson_Did the manual testing as well, this patch works great. Not setting to rtbc yet because of the changes requested in #9.
Comment #11
wim leersPer #9 + #10:
Comment #12
phenaproximaLooks good to me.
Comment #13
lauriiiMakes sense to convert these to deprecation failures to reduce the disruption. I guess this sets a pattern that from 11.x onwards, if we introduce additional validation constraints, we'd have to introduce them as deprecations first. If that's the case, we'll need to figure out how to do that but we don't have to cross that bridge yet.
Tested this manually using Dynamic Entity Reference module. Removing the release highlights since this is just a follow-up to the issue that should be highlighted.
Committed 4c8821b and pushed to 11.x. Thanks!
Comment #16
wim leersYAY!
Updated the release notes at #3361534: KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate and moved the tag there.
This unblocked #3341682: New config schema data type: `required_label` 😊
Comment #18
wim leersRefining this further: #3395099: [meta] Allow config types to opt in to config validation, use "FullyValidatable" constraint at root as signal to opt in to "keys/values are required by default".