Problem/Motivation

#2183983: Find hidden configuration schema issues still found the following missing configuration schema elements for fields:

  • field.formatter.settings.field_test_default
  • field.formatter.settings.field_plugins_test_text_formatter
  • field.formatter.settings.field_empty_setting
  • field.formatter.settings.field_test_with_prepare_view
  • field.formatter.settings.link_separate
  • field.widget.settings.link_default
  • field.widget.settings.test_field_widget
  • field.widget.settings.text_textfield
  • field.widget.settings.field_plugins_test_text_widget
  • field.storage_settings.shape
  • field.storage_settings.test_field_with_dependencies
  • field.storage_settings.hidden_test_field
  • field.field_settings.test_field_with_dependencies
  • field.field_settings.hidden_test_field
  • entity_view_display.third_party.field_test

Proposed resolution

Add strict schema checking at appropriate tests.
Fix missing schemas, migrations or tests as appropriate.
Review.
Commit.

Remaining tasks

Review.
Commit.

User interface changes

None.

API changes

Missing config schemas added, incorrect inheritance in config schemas fixed.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Issue summary: View changes
FileSize
7.38 KB

So I tried looking for tests that have the least *other* fails besides the ones we are looking for here, so we can focus on fixing these while having test coverage. Unfortunately field.widget.settings.link_default is only found in the *big* Drupal6MigrateTest, which has lots of other fails, so I would be weary to cover all of it here. It does not appear with any other tests (not even individual Migrate tests). So we may need to do that one later.

So here is a failing test.

Gábor Hojtsy’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: 2387141-fail-fields.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
4.43 KB
11.82 KB

Here is an attempt to fix most of them!

Added schema in field_plugins_test module:

  • field.formatter.settings.field_plugins_test_text_formatter
  • field.widget.settings.field_plugins_test_text_widget

Added schema in field_test module:

  • field.formatter.settings.field_test_default
  • field.formatter.settings.field_empty_setting
  • field.formatter.settings.field_test_with_prepare_view
  • field.widget.settings.test_field_widget
  • field.storage_settings.hidden_test_field

Added schema in entity_test module:

  • field.storage_settings.shape

Problematic items were:

  • field.formatter.settings.link_separate and field.widget.settings.text_textfield - these are already defined, will need to figure out why are those tests not dependent on the respective modules.
  • field.storage_settings.test_field_with_dependencies - this is not defined but has no setting, so it should fall back on the wildcard field.storage_settings.* provided in core.data_types.schema.yml. Need to figure out why that is not happening.
  • field.widget.settings.link_default - as I updated the summary above, this is not covered by tests here because it only seems to appear with the full migrate 6 test suite which has lots of other unrelated problems.

Let's see what does the testbot have to say.

Status: Needs review » Needs work

The last submitted patch, 5: 2387141-fix-missing-fields-5.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
13.93 KB
3.33 KB

Fixes for most fails:

1. Typo in definition of field.widget.settings.test_field_widget (maping vs. mapping)
2. test_field_with_dependencies actually extends from test_field, this needs to be reflected.
3. EntityDisplayTest.php adds an "arbitrary" setting on a component. This should be a third party setting that needed a schema provided also.
3. Found the problem with link_separate also, it extends from the base link field, we should extend too in schema.

Gábor Hojtsy’s picture

Issue summary: View changes
FileSize
17.05 KB
3.12 KB

#2183983: Find hidden configuration schema issues is not reproducing the link_default fail anymore, so removing that from the summary. However the same rolled with these fixes still reproduces some entity view/form mode fails in the tests that I am brining in for testing here as well. (Also some in migrate, but that should be its own issue).

Gábor Hojtsy’s picture

Actually MigrateFieldWidgetSettingsTest has pretty self contained fails for link_default field settings. We can include that too.

The last submitted patch, 8: 2387141-fix-missing-fields-8.patch, failed testing.

Gábor Hojtsy’s picture

Fixes for those fails:

1. SelectionTest.php (entity_reference) was testing a target_bundles option on a Views selection handler, while that option does not exist on Views selection handlers.
2. FormTest.php (field) set default values to NULL while they should be an empty array.
3. FieldInstanceWidgetSettings.php (migrate) provided default for the nonexistent placeholder_uri setting; placeholder_url is correct
4. options_config_install_test default configs included third party settings (certainly saved with a test module on); this is already tested elsewhere so we don't need it here (otherwise we would need to include the entity_test module which has the schema for these bits but also has lots of other code and alters we don't need here)
5. QuickEditTestBase.php attempts to set labels on entity form mode components but those are already on the field setting (where it also sets it before), not in component settings.
6. EntityQueryRelationshipTest.php provides default vocabulary config in a singular format while default values are multivalued.

Gábor Hojtsy’s picture

Issue summary: View changes

Updated the summary again.

The last submitted patch, 9: 2387141-fix-missing-fields-9.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 11: 2387141-fix-missing-fields-11.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
2.61 KB
23.86 KB

The unicorn editor needs config schema. This will not yet pass due to side effects I think. May actually need API changes :/ Changed the test setting to ponies_too for easier schema writing.

Status: Needs review » Needs work

The last submitted patch, 15: 2387141-fix-missing-fields-15.patch, failed testing.

dawehner’s picture

@Gábor Hojtsy
Please ping me when you want a review here.

+++ b/core/modules/editor/tests/modules/config/schema/editor_test.schema.yml
@@ -0,0 +1,9 @@
+# Schema for the configuration files of the Editor test module.
+
+editor.settings.unicorn:
+  type: mapping
+  label: 'Unicorn settings'
+  mapping:
+    ponies_too:
+      type: string
+      label: 'Ponies too'
diff --git a/core/modules/editor/tests/modules/src/Plugin/Editor/UnicornEditor.php b/core/modules/editor/tests/modules/src/Plugin/Editor/UnicornEditor.php

unicorns, pones AND llamas. Pure anarchy, we have to create a standard issue on the technical working group :P

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
24.6 KB
758 bytes

So editor altering is not schema compatible the same way views display extenders are not :/ :/ Opened #2389697: Editor settings altering not needed (and not compatible with config schema assumptions), exempting that test here for now. That should make this green and hopefully RTBC. Further work on the editor problem should continue in #2389697: Editor settings altering not needed (and not compatible with config schema assumptions).

Gábor Hojtsy’s picture

@dawehner, @all: this should be ready for review, I think it should be RTBC and get in :) The changes are very straightforward. Happy to explain any but I'll mostly be traveling today, so will not be easy to find online.

Gábor Hojtsy’s picture

Turns out #2389697: Editor settings altering not needed (and not compatible with config schema assumptions) is not so critical. Postponed on this one then, since it will need to clean up some of the test code that this one touches. I think its better to keep that as its own issue for reference, google juice, etc. for those who are looking to why they cannot add arbitrary settings on editors. For now, let's get this in :)

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Can't find anything to complain about!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +Ghent DA sprint

@Wim Leers neither can I.

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

  • alexpott committed b175ecc on 8.0.x
    Issue #2387141 by Gábor Hojtsy: Missing field configuration schemas...
Gábor Hojtsy’s picture

Superb, thanks!

Status: Fixed » Closed (fixed)

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