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

  1. 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
    
  2. Run Drupal\Tests\dynamic_entity_reference\Kernel\DynamicEntityReferenceTestViewsTest
  3. 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

  1. Convert any validation errors in the case of running a contrib test to deprecation notices instead.
  2. 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

  1. ✅ Working MR
  2. ✅ Reviews → #9 + #10
  3. Once approved or committed by a core committer:
    1. ✅ tweak the release notes snippet in #3361534
    2. ✅ move the 10.2.0 release highlights tag to #3361534, and remove it here
    3. ✅ 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

See #3361534: KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate.

Issue fork drupal-3379899

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:

Comments

Wim Leers created an issue. See original summary.

Wim Leers credited jibran.

wim leers’s picture

Title: [PP-1] Follow-up for #3361534: config validation errors in contrib modules should cause deprecation notices, not test failures » Follow-up for #3361534: config validation errors in contrib modules should cause deprecation notices, not test failures
Issue summary: View changes

I marked this PP-1 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! 🤯

wim leers’s picture

Assigned: wim leers » Unassigned
Status: Active » Needs review
Issue tags: +Needs manual testing

This 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.

wim leers’s picture

wim leers’s picture

This is now a blocker for #3341682 per @catch at #3341682-56: New config schema data type: `required_label`.

phenaproxima’s picture

I 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!

phenaproxima’s picture

Status: Needs review » Needs work
Issue tags: -Needs manual testing

Patch looks great, just a couple of questions.

borisson_’s picture

Did the manual testing as well, this patch works great. Not setting to rtbc yet because of the changes requested in #9.

wim leers’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs change record updates

Per #9 + #10:

  1. implemented @phenaproxima's excellent suggestions
  2. went ahead and already updated the CR: https://www.drupal.org/node/3362879#contrib, but holding off for core committer approval to also tweak the release notes snippet and move the release highlights tag
phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

lauriii’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -10.2.0 release highlights

Makes 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!

  • lauriii committed 4c8821bc on 11.x
    Issue #3379899 by Wim Leers, phenaproxima, jibran, borisson_: Follow-up...

wim leers’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.