Problem/Motivation

As per #2183983: Find hidden configuration schema issues, several block and block content 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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Status: Needs review » Needs work

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

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
5.24 KB
7.83 KB

Fixes for the fails:

1. The test view had a wrong field key, should be fields.

2. The block content translation test encounters content_translation.settings, which is impossible to provide schema for. See #2363155: content_translation.settings config is not scalable. To make these pass, we need to provide an ignore schema. I also removed the schema assert from ContentTranslationSettingsTest.php in favor of the strict checking test option.

3. The block tests use test_field types and link fields respectively and schemas were missing for those.

Gábor Hojtsy’s picture

In #2183983-74: Find hidden configuration schema issues @Berdir removed the cached data setting, which is indeed not referenced anywhere. Also fixing the default value for the test field.

The last submitted patch, 3: 2379697-block-schema-fix.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 4: 2379697-block-schema-fix-4.patch, failed testing.

Gábor Hojtsy’s picture

Patch did not apply anymore. Rerolled. Also moved schema ignore fix to the checker. I should have remembered we don't have iteration support on ignore.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 7: 2379697-block-schema-fix-7.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
9.11 KB
753 bytes

Resolving that PHP fatal by not invoking the method I am removing :)

Status: Needs review » Needs work

The last submitted patch, 10: 2379697-block-schema-fix-10.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
2.38 KB
9.02 KB

Field settings schema keys were restructured in #2370305: Refactor field type configuration schemas for DX, easier to find errors, changed for those. Also found link_type field setting on links that I previously missed for some reason.

Gábor Hojtsy’s picture

Title: Fix configuration schema issues in block and block content modules » Fix configuration schema issues in block content (indirectly link and field test) modules

Status: Needs review » Needs work

The last submitted patch, 12: 2379697-block-schema-fix-12.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
9.06 KB

Status: Needs review » Needs work

The last submitted patch, 15: 2379697-block-schema-fix-15.patch, failed testing.

Gábor Hojtsy’s picture

Priority: Normal » Major
Status: Needs work » Needs review
Issue tags: +D8 upgrade path, +D8MI, +sprint
FileSize
12.01 KB
3.59 KB

Marked #2358263: Adding missing configuration schema for views blocks a duplicate because the same fails can be reproduced here and they are covered by making DisplayBlockTest strict on config schema.

Incorporated the fixes from there. Also trying to remove the fields in the test blocks because there are no link_to_node title fields in views_test_data (no title field even!).

Status: Needs review » Needs work

The last submitted patch, 17: 2379697-block-schema-fix-17.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
14.35 KB
2.34 KB

This is starting to get a bit unwieldy as the test views that are imported from views_test also expose their problems here... Anyway, we can get going with fixing those fails so long as there is an end in sight :D Fixing fails from testbot related to missing plugin_id's (no typing for schema on test fields).

Status: Needs review » Needs work

The last submitted patch, 19: 2379697-block-schema-fix-19.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Postponed

So the fails here are due to the views wizard. Duh. Opened #2381973: View wizard creates 'invalid' views out of the box, missing plugin_ids, insecure permissions. Postponing on that one.

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
12.63 KB

Rerolled.

Status: Needs review » Needs work

The last submitted patch, 23: 2379697-block-schema-fix-23.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
14.63 KB
2.28 KB

We should place blocks with label and not title, see docs and code of drupalPlaceBlock.

Gábor Hojtsy’s picture

Note sure anymore that the test views were related, let's test without them. Will incorporate the fixes to #2325269: Test and fix views in test_views directories against their configuration schema unless required here. Should make reviewing the changes easier.

Status: Needs review » Needs work

The last submitted patch, 26: 2379697-block-schema-fix-26.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

Was a bit carried away with aiming for simplicity :D We do need the test views fixes.

Gábor Hojtsy’s picture

Reuploading #25 to avoid any confusion.

vijaycs85’s picture

Status: Needs review » Reviewed & tested by the community

Looks good as the changes are just adding strict config schema check and few missing field plugin elements in test... This is good to go.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed f355ada and pushed to 8.0.x. Thanks!

  • alexpott committed f355ada on 8.0.x
    Issue #2379697 by Gábor Hojtsy: Fix configuration schema issues in block...
Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, thanks!

Status: Fixed » Closed (fixed)

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