Problem/Motivation

As per #2183983: Find hidden configuration schema issues, several contact module tests would fail if we turn on full schema checking. Here is a patch to expose those errors and fix them. A subset of issues there.

Proposed resolution

Find and fix the issues.

Remaining tasks

Fix them.

User interface changes

None.

API changes

None except fixed schemas.

CommentFileSizeAuthor
#12 2379683-contact-schema-fix-12.patch5.02 KBGábor Hojtsy
#5 2379683-contact-schema-fix-5.patch5.02 KBGábor Hojtsy
#5 interdiff.txt1.49 KBGábor Hojtsy
#3 interdiff.txt864 bytesGábor Hojtsy
#3 2379683-contact-schema-fix.patch3.53 KBGábor Hojtsy
#1 2379683-config-schemas.patch2.68 KBGábor Hojtsy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
FileSize
2379683-config-schemas.patch2.68 KB

Status: Needs review » Needs work

The last submitted patch, 1: 2379683-config-schemas.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
2379683-contact-schema-fix.patch3.53 KB
interdiff.txt864 bytes

Two plugin IDs were missing from the test view in views.view.test_contact_link.

Status: Needs review » Needs work

The last submitted patch, 3: 2379683-contact-schema-fix.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.49 KB
2379683-contact-schema-fix-5.patch5.02 KB

The remaining fails were due to account settings forms side-effects.

1. The account settings form uses and saves to site.settings a mail_notification key which was missing.
2. The user notification for account cancellation is spelled "status_canceled" in the code. The default config and the schema spelled it with two L's ("status_cancelled"), which obviously does not match.

This should make tests pass, at least the config storage test passes fine locally.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Status: Needs review » Reviewed & tested by the community

Changes looks good.

+++ b/core/modules/system/config/schema/system.schema.yml
@@ -38,6 +38,9 @@ system.site:
+    mail_notification:

+++ b/core/modules/user/config/install/user.settings.yml
@@ -6,7 +6,7 @@ notify:
+  status_canceled: false

I guess only changes related to this issue is those two elements. 

+++ b/core/modules/contact/src/Tests/ContactPersonalTest.php
@@ -19,6 +19,15 @@
+  protected $strictConfigSchema = TRUE;

is it from core test which works already?

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

@vijaycs85: yes, if you grep for status_cancalled you only find these two and status_canceled will lead to the actual code dealing with it :) as for the testing variable, yeah existing tests are using this and the more places we introduce it the less it is possible to make mistakes -- we can eventually default it to TRUE in core tests if all schemas are good :)

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented

existing tests are using this and the more places we introduce it the less it is possible to make mistakes -- we can eventually default it to TRUE in core tests if all schemas are good :)

sounds good.

Gábor Hojtsy queued 5: 2379683-contact-schema-fix-5.patch for re-testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Title: Fix configuration schema issues in contact module » Fix configuration schema issues in contact (indirectly user and system) modules

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: 2379683-contact-schema-fix-5.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Reviewed & tested by the community
FileSize
2379683-contact-schema-fix-12.patch5.02 KB

Reroll due to #2316909: Revisit all built-in test/default views configuration in core. Applied with offsets. Back to RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 26f8d71 and pushed to 8.0.x. Thanks!

  • alexpott committed 26f8d71 on 8.0.x 
    Issue #2379683 by Gábor Hojtsy: Fix configuration schema issues in...

Status: Fixed » Closed (fixed)

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