Follow-up for #3361534: Config validation errors can still occur for contrib modules, disrupting contrib

Wim Leers → credited alexpott → .

Wim Leers → credited Berdir → .

Wim Leers → credited bircher → .

Wim Leers → credited borisson_ → .

Wim Leers → credited effulgentsia → .

Determined the root cause:

  private function isContribViolation(): bool {
    $test_file_name = (new \ReflectionClass($this))->getFileName();
…

$this was assumed to be the test class. Unfortunately, this is inaccurate, because it depends on WHAT is calling
\Drupal\Core\Config\Schema\SchemaCheckTrait::checkConfigSchema():

OOPS 😱

IMHO this is a 10.2.x beta blocker because virtually every contrib maintainer will run into this 😅🙈

1. ⚠️😱 @Berdir discovered that the Webform tests are failing nonetheless in 10.2.x, see 🐛 [PP-2] to_email and similar config keys have schema type email but allow other things Needs work . So I automated testing that in 📌 Test Webform against upcoming 10.2.x on GitLab CI Needs review . The test results confirm what @Berdir said. Which makes this critical because it disrupts contrib significantly 😐

👆 Fixed in https://git.drupalcode.org/project/drupal/-/merge_requests/5430/diffs?co...

1. ⚠️ Even deprecation notices are too disruptive: (extracted out of 📌 Follow-up for #3361534: config validation errors in contrib modules should not cause deprecation notices, unless they opt in Active )

👆 Fixed in https://git.drupalcode.org/project/drupal/-/merge_requests/5430/diffs?co...

I don't think this fix will work because the schema checker also runs in the site under test. See \Drupal\Core\Test\FunctionalTestSetupTrait::prepareSettings().

We need to pass another argument into the testing.config_schema_checker service to determine whether to do config validation - this argument can be based on the test class.

I will run the Webform test suite against 10.2.x + this MR to verify.

First made the Webform test suite run much faster on GitLab CI, then used @Berdir's tip in Slack to automatically test with this MR on 10.2.

Result:

(Difference in output is due to switching from sequential to parallel test running per @Berdir in #6.

Conclusion

@alexpott was right in #3402168-12: Follow-up for #3361534: Config validation errors can still occur for contrib modules, disrupting contrib → : the core MR does not yet fix it for functional tests.

— #3402134-12: Test Webform against upcoming 10.2.x on GitLab CI — and use concurrency to make tests 6x faster →

On it …

2926c398 is a hardening that fixes the incomplete detection logic, resulting in more functional tests passing, but not all:

Drupal\Tests\webform\Functional\States\WebformStatesManagerT   1 passes                                      
Drupal\Tests\webform\Functional\Variant\WebformVariantElemen   1 passes                                      
Drupal\Tests\webform\Functional\States\WebformStatesHiddenTe   0 passes             1 exceptions             
FATAL Drupal\Tests\webform\Functional\States\WebformStatesHiddenTest: test runner returned a non-zero error code (2).

Now working on the actual bug that @alexpott pointed out, which only occurs if the the functional test is using the UI to make changes to config. This is true for \Drupal\Tests\webform\Functional\Settings\WebformSettingsSerialTest for example, which receives this HTML response:

The website encountered an unexpected error. Try again later.<br><br><em class="placeholder">Drupal\Core\Config\Schema\SchemaIncompleteException</em>: Schema errors for webform.webform.contact with the following errors: 0 [handlers.email_confirmation.settings.to_mail] &amp;lt;em class=&amp;quot;placeholder&amp;quot;&amp;gt;&amp;amp;quot;[current-user:mail]&amp;amp;quot;&amp;lt;/em&amp;gt; is not a valid email address., 1 [handlers.email_confirmation.settings.from_mail] &amp;lt;em class=&amp;quot;placeholder&amp;quot;&amp;gt;&amp;amp;quot;_default&amp;amp;quot;&amp;lt;/em&amp;gt; is not a valid email address., 2 [handlers.email_notification.settings.to_mail] &amp;lt;em class=&amp;quot;placeholder&amp;quot;&amp;gt;&amp;amp;quot;_default&amp;amp;quot;&amp;lt;/em&amp;gt; is not a valid email address., 3 [handlers.email_notification.settings.from_mail] &amp;lt;em class=&amp;quot;placeholder&amp;quot;&amp;gt;&amp;amp;quot;[webform_submission:values:email:raw]&amp;amp;quot;&amp;lt;/em&amp;gt; is not a valid email address. in <em class="placeholder">Drupal\Core\Config\Development\ConfigSchemaChecker-&gt;onConfigSave()</em> (line <em class="placeholder">94</em> of <em class="placeholder">core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php</em>). <pre class="backtrace">call_user_func(Array, Object, &#039;config.save&#039;, Object) (Line: 111)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher-&gt;dispatch(Object, &#039;config.save&#039;) (Line: 229)
Drupal\Core\Config\Config-&gt;save() (Line: 278)
Drupal\Core\Config\Entity\ConfigEntityStorage-&gt;doSave(&#039;contact&#039;, Object) (Line: 486)
Drupal\Core\Entity\EntityStorageBase-&gt;save(Object) (Line: 257)
Drupal\Core\Config\Entity\ConfigEntityStorage-&gt;save(Object) (Line: 108)
Drupal\webform\WebformEntityStorage-&gt;save(Object) (Line: 352)
Drupal\Core\Entity\EntityBase-&gt;save() (Line: 609)
Drupal\Core\Config\Entity\ConfigEntityBase-&gt;save() (Line: 293)
Drupal\Core\Entity\EntityForm-&gt;save(Array, Object) (Line: 50)
Drupal\webform\EntitySettings\WebformEntitySettingsBaseForm-&gt;save(Array, Object) (Line: 847)
Drupal\webform\EntitySettings\WebformEntitySettingsSubmissionsForm-&gt;save(Array, Object)
…

c0ae1f84 made the Webform test suite pass — with the exception of unrelated failures (for example, WebformStatesHiddenTest and WebformSettingsPreviewTest fail because it's asserting markup literally instead of using CSS selectors, XPath or DOMDocument — probably these output changes are due to 🐛 Upgrade filter system to HTML5 Fixed ). See the results.

@alexpott's alternative MR looks much simpler and achieves the same. 👍 I was just trying to minimize change, but I'm of course fine with his proposal.

However, his proposal only fixes problem #2 in the issue summary, not problem #1 (i.e no deprecation error for contrib modules … at least not yet, because even that is considered too disruptive). The new name of the new argument he proposed is so strongly connected to deprecation errors that we're forced to. So my only concern with the MR is the $triggerDeprecationOnValidationError argument.

Unless … @alexpott now thinks that the deprecation error is actually fine to keep? 🤔

Testing Alex' MR (commit: https://git.drupalcode.org/project/webform/-/merge_requests/380/diffs?co...), hopefully the results of his MR will match the results of my MR. (Which is possible despite his MR still triggering a deprecation error because Webform is suppressing deprecations.)

Wim Leers → closed merge request !5430

Cannot find a single remaining thing to complain about in @alexpott's alternate MR.

The test results for webform on top of his MR still (see #17) match ✅

It's a very different approach from mine — much simpler, more explicit, less magic. 👍 My only contribution to this MR is adding a data provider to the test.

… except that our YAML parsing of services.yml files does not quite like enums yet:

1) Drupal\Tests\image\Functional\ImageDimensionsTest::testImageDimensions
Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The file "/builds/project/drupal/sites/simpletest/75153141/services.yml" does not contain valid YAML: The string "!php/const Drupal\Core\Config\Schema\SchemaCheckConstraintValidation::Error" could not be parsed as a constant. Did you forget to pass the "Yaml::PARSE_CONSTANT" flag to the parser? at line 12 (near "testing.config_schema_checker: { class: Drupal\Core\Config\Development\ConfigSchemaChecker, arguments: ['@config.typed', [config_schema_test.no_schema, config_schema_test.some_schema, config_schema_test.schema_data_types, config_schema_test.no_schema_data_types, config_test.dynamic.system], !php/const Drupal\Core\Config\Schema\SchemaCheckConstraintValidation::Error], tags: [{ name: event_subscriber }] }")

😬

Ready now :)

This looks good, easy to follow the new logic and mostly moving things around otherwise. Committed/pushed to 11.x and cherry-picked to 10.2.x, thanks!

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