Problem/Motivation

\Drupal\FunctionalTests\Update\UpdatePathTestBase does not support the $configSchemaCheckerExclusions property.

During regular testing it's possible to skip specific configuration by listing them in the $configSchemaCheckerExclusions property.

Proposed resolution

Add this functionality so that contrib update tests can leverage it. Specifically the update test for the Media Entity module to using core's Media module. At the moment it is not possible to support the schema for media.type.image because of how the media entity image exif module replaces the core media type class. Rather than try and fix this in media entity image exif module first we should allow modules to have tests that skip schema checking because whilst it is valuable - so it is a working update path test.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

N/a

CommentFileSizeAuthor
#13 3062556-8.7.x.patch2.18 KBalexpott
#13 3062556-follow-up.patch852 bytesalexpott
#5 3062556-5.patch2.1 KBalexpott
#5 3062556-5.test-only.patch928 bytesalexpott
#4 3062556-4.patch1.19 KBalexpott
#4 2-4-interdiff.txt2.29 KBalexpott
#2 3062556-2.patch1.97 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Active » Needs review
FileSize
3062556-2.patch1.97 KB

Here's patch I've tested it whilst re-enable the media entity update test.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Needs work

Only two nits, probably needs tests too...?

  1. +++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
@@ -363,15 +363,23 @@ protected function runUpdates() {
+        // The config schema can be incorrect while the update functions are being
+        // executed. But once the update has been completed, it needs to be valid
+        // again. Assert the schema of all configuration objects now.

    Nit: over 80 characters. 

  2. +++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
@@ -363,15 +363,23 @@ protected function runUpdates() {
+          if (in_array($name, $exclude)) {

    We should always pass TRUE as the third argument to in_array().

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Title: UpdatePathTestBase does not support the $strictConfigSchema and $configSchemaCheckerExclusions properties » UpdatePathTestBase does not support the $configSchemaCheckerExclusions property
Issue summary: View changes
Status: Needs work » Needs review
FileSize
2-4-interdiff.txt2.29 KB
3062556-4.patch1.19 KB

Ah realised we shouldn't do the $strcitConfigSchema thing - this is false for all update tests. That's why there's this extra config schema checking in \Drupal\FunctionalTests\Update\UpdatePathTestBase::runUpdates(). So what we need to do is to support $configSchemaCheckerExclusions property.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
3062556-5.test-only.patch928 bytes
3062556-5.patch2.1 KB

Re #3 and the question about tests. Worked out a simple way to prove that we now obey the property.

The test only patch is the interdiff.

The last submitted patch, 4: 3062556-4.patch, failed testing. View results

The last submitted patch, 5: 3062556-5.test-only.patch, failed testing. View results

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
CreditAttribution: chr.fritsch at Thunder commented
Status: Needs review » Reviewed & tested by the community

This looks good to me. We have a test and https://www.drupal.org/project/media_entity/issues/3062560#comment-13152099 already leverages that functionality and shows that it works.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed 54f8eb6 and pushed to 8.8.x. Thanks!

  • catch committed 54f8eb6 on 8.8.x 
    Issue #3062556 by alexpott, phenaproxima: UpdatePathTestBase does not...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Fixed » Reviewed & tested by the community

RTBC for 8.7.x for after the freeze.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Needs work 
  1. +++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBaseTest.php
@@ -182,4 +182,17 @@ public function testModuleListChange() {
+   * Tests that test running environment is updated when module list changes.
+   *
+   * @see update_test_schema_update_8003()

    These comments are copy/pasted from the method above.

  2. +++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBaseTest.php
@@ -182,4 +182,17 @@ public function testModuleListChange() {
+  public function testSchemaChecking() {

    Are we sure we want a whole new update test just for this - especially one that updates from 8.0 standard?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Needs work » Reviewed & tested by the community
FileSize
3062556-follow-up.patch852 bytes
3062556-8.7.x.patch2.18 KB

Here's a quick follow-up and a new patch for 8.7.x.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

Crediting @larowlan

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

This test doesn't really fit in the scope of any of the other tests in UpdatePathTestBaseTest unfortunately. What's also interesting is this test is also run as part of UpdatePathTestBaseFilledTest due to class inheritance.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed both the follow-up and the 8.7.x patch, thanks!

  • catch committed eada7ff on 8.8.x 
    Issue #3062556 by alexpott, catch, phenaproxima, larowlan:...

  • catch committed 192647d on 8.7.x 
    Issue #3062556 by alexpott, catch, phenaproxima, larowlan:...

Status: Fixed » Closed (fixed)

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