Problem/Motivation

Lots of errors in #2183983: Find hidden configuration schema issues are related to entity form display and view display errors. Some problems identified:

- missing some core widget types such as email_default, string_textarea
- the base schema does not include settings, so each type needs to include a settings key (looooooots of cruft)
- that cruft is not even honest, those types have no settings, so should have no settings items (an empty sequence fits that)
- by including the settings key in the base schema, we make it possible to not need to define testing types such as field_plugins_test_text_widget

Proposed resolution

Add settings to base schema as empty list.
Remove superfluous cruft.
Add schema for email_default and string_textarea

Remaining tasks

Review. Commit.

User interface changes

None.

API changes

None.

Files: 
CommentFileSizeAuthor
#69 2368349-69.patch37.08 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,258 pass(es). View
#69 interdiff.txt2.92 KBGábor Hojtsy
#61 interdiff.txt2.44 KBGábor Hojtsy
#61 2368349-61.patch34.16 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,222 pass(es), 1 fail(s), and 0 exception(s). View
#59 interdiff.txt1.88 KBGábor Hojtsy
#59 2368349-59.patch31.72 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,258 pass(es), 3 fail(s), and 0 exception(s). View
#52 2368349-52.patch29.84 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,187 pass(es). View
#52 interdiff.txt4.04 KBGábor Hojtsy
#49 2368349-49.patch25.8 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,166 pass(es), 10 fail(s), and 0 exception(s). View
#49 interdiff.txt1.53 KBGábor Hojtsy
#47 2368349-2.47.patch25.7 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,151 pass(es). View
#47 44-47-interdiff.txt468 bytesalexpott
#44 2368349-2.44.patch25.7 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,147 pass(es). View
#44 41-44-interdiff.txt3.13 KBalexpott
#41 2368349-2.41.patch23.07 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,012 pass(es), 16 fail(s), and 7 exception(s). View
#41 29-41-interdiff.txt5.61 KBalexpott
#41 2368349-2-test-only.41.patch11.62 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,026 pass(es), 18 fail(s), and 9 exception(s). View
#29 interdiff.txt3.88 KBGábor Hojtsy
#29 2368349-entity-view-display-schemas-29.patch24.22 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,266 pass(es). View
#26 interdiff.txt1.65 KBGábor Hojtsy
#26 2368349-entity-view-display-schemas-26.patch24.15 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,174 pass(es). View
#21 2368349-entity-view-display-schemas-21-with-all-fixes.patch22.77 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,164 pass(es), 4 fail(s), and 0 exception(s). View
#20 interdiff.txt6.53 KBGábor Hojtsy
#20 2368349-entity-view-display-schemas-20.patch20.54 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,088 pass(es), 162 fail(s), and 0 exception(s). View
#19 interdiff.txt512 bytesGábor Hojtsy
#19 2368349-entity-view-display-schemas-19.patch14 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,012 pass(es), 164 fail(s), and 0 exception(s). View
#16 interdiff.txt537 bytesGábor Hojtsy
#16 2368349-entity-view-display-schemas-16.patch14.02 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,055 pass(es), 161 fail(s), and 0 exception(s). View
#14 interdiff.txt3.82 KBGábor Hojtsy
#14 2368349-entity-view-display-schemas-14.patch13.92 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
#11 2368349-entity-view-display-schemas-11-should-fail.patch10.13 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,939 pass(es), 55 fail(s), and 0 exception(s). View
#9 interdiff.txt938 bytesGábor Hojtsy
#9 2368349-entity-view-display-schemas-9.patch12.37 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,974 pass(es). View
#6 2368349-entity-view-display-schemas.patch11.45 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,808 pass(es), 2 fail(s), and 0 exception(s). View
#6 interdiff.txt2.04 KBGábor Hojtsy
#1 2368349-entity-view-display-schemas.patch11.15 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,944 pass(es), 49 fail(s), and 0 exception(s). View

Comments

Gábor Hojtsy’s picture

FileSize
11.15 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,944 pass(es), 49 fail(s), and 0 exception(s). View
Gábor Hojtsy’s picture

I don't have a great idea how to test these ones. The fails in #2183983: Find hidden configuration schema issues are all over the place, the fixes are also all over the place. They are *very* straightforward though.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests
[6:16pm] alexpott: GaborHojtsy: want we need is a test for all fields - the adds storage, a field, form display and view display
[6:16pm] alexpott: GaborHojtsy: and tests all the schema
[6:23pm] GaborHojtsy: alexpott: that creates one of each field?
[6:23pm] GaborHojtsy: alexpott: on some entity?
[6:23pm] alexpott: GaborHojtsy: something like that
[6:23pm] alexpott: GaborHojtsy: that way we'd have test coverage of all of this

The last submitted patch, 1: 2368349-entity-view-display-schemas.patch, failed testing.

Berdir’s picture

On the last comment, most field types have two tests:

- a kernel test, that just creates data and saves it. (e.g. NumberItemTest, and similar)
- a web test, that also tests widgets and formatters. (e.g. NumberFieldTest and similar)

We could try add schema validation to those. Having a single test that creates 15 different fields and tests them all seems crazy, as we already do that.

Maybe we can have a common base class for those tests, and have a tearDown() that validates this, if possible? We do something similar for php errors, so why not schema validation as well?

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
2.04 KB
11.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,808 pass(es), 2 fail(s), and 0 exception(s). View

In fact this removal of the arbitrary sequence of settings exposed lots of fails :) So looks like plenty of things to fix here. Checked settings for those field types.

Status: Needs review » Needs work

The last submitted patch, 6: 2368349-entity-view-display-schemas.patch, failed testing.

Gábor Hojtsy’s picture

I think the fails we already have here showcase that the existing schema validation works very well if instead of an obscure sequence of strings, we define them with their actual keys. So in my view the existing fails may be well enough of a proof, no?

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
12.37 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,974 pass(es). View
938 bytes

The tests even found this :)

vijaycs85’s picture

wow, that's very nice cleanup. Thanks @Gábor Hojtsy :) +1 to add more tests.

Gábor Hojtsy’s picture

FileSize
10.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,939 pass(es), 55 fail(s), and 0 exception(s). View

I would argue this does not need more tests. The reason the tests were not failing is that the form modes and view modes had this section for settings that slurped up all subkeys whetever they were:

entity_view_display.field.datetime_plain:
  type: entity_field_view_display_base
  label: 'Datetime plain display format settings'
  mapping:
    settings:
      type: sequence
      label: 'Settings'
      sequence:
        - type: string

This is a classic old example of a "I have no settings" definition from before we made the sequence key of the sequence type optional (and resolve to undefined types for the children). Once we introduce a settings key globally for the base type of the entity view and form elements, any key under them that was not defined starts to throw errors big time as shown above. This was already demonstrated in #6 and #1 for several types but not the string_textarea and email_default types because those were already added. Here is a test targeted patch that does not include those fixes or any of the other fixes either.

It just removes the "anonymous sequences" and adds empty sequence definitions, which thus fails with the existing tests for each entity view or form field section that does have specific keys defined (that is should not have empty settings).

This should fail for all the items fixed in #9:

- entity_form_display.field.string_textarea
- entity_form_display.field.email_default
- entity_form_display.field.boolean_checkbox (promoted, sticky)
- entity_view_display.field.comment_default
- entity_view_display.field.image

Given we already have tests for field types, the definition of their entity view/form settings as an empty sequence is enough to fail on all the errors. Not defining their settings key or defining it as an anonymous sequence of strings ate up all the settings and obscured the fails. A sequence that was defined to not have items fails very well already if it has items for some reason.

Going forward we should consciously remove other instances of anonymous sequences which were used to define sequences we want to be empty, and use sequences that are defined to be empty, so that we get all the fails if they contain elements.

Status: Needs review » Needs work

The last submitted patch, 11: 2368349-entity-view-display-schemas-11-should-fail.patch, failed testing.

Gábor Hojtsy’s picture

Well, right that did not fail for string_textarea and email_default. Thats interesting because EmailFieldTest creates email_default fields. I could not find a test that creates string_textarea fields. Which should be that one? @Berdir?

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
13.92 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
3.82 KB

All right. The existing fails are due to those fields appearing in default config where they are already tested. I added that base test class with a tearDown() and added to a few field tests I found. As said, I did not find the string field test. There are also tests that inherit from other base classes, such as the options field test :/

This still does not include the actual fixes, so we can see if there are more fails or not. The email one should finally fail. I don't know what else may start failing in the other displays.

Status: Needs review » Needs work

The last submitted patch, 14: 2368349-entity-view-display-schemas-14.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
14.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,055 pass(es), 161 fail(s), and 0 exception(s). View
537 bytes

Wow, Uncaught exception 'LogicException' with message 'Missing phpDoc on Drupal\field\Tests\FieldItemTestBase.' :)

Berdir’s picture

Not correct, a base class must be abstract only actual test classes should have a @group.

Status: Needs review » Needs work

The last submitted patch, 16: 2368349-entity-view-display-schemas-16.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
14 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,012 pass(es), 164 fail(s), and 0 exception(s). View
512 bytes

@Berdir: indeed. It still stands that the string field type does not seem to have tests. We need to introduce one looks like.

Gábor Hojtsy’s picture

Issue tags: -Needs tests
FileSize
20.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,088 pass(es), 162 fail(s), and 0 exception(s). View
6.53 KB

Created a very quick StringFieldTest by copying the relevant part of TextFieldTest to this. I made StringFieldTest extend from FieldItemTestBase, so it gets all the schema checking. Then TectFieldTest lost its common parts with StringFieldTest (testing the no-formatting widgets and most of the setup method). I think this test structure shows the common parts very well, right?

This now fails properly for the string type locally too. So this will be the "test only patch". Note that it NEEDS to include the removal of the eat-all-anonymous-sequences for it to fail properly otherwise those would eat the named settings and not fail. Without those changes, this patch would only fail for the string type and email type that was not explicitly defined before (and therefore had no settings key in the fallback definition).

Gábor Hojtsy’s picture

FileSize
22.77 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,164 pass(es), 4 fail(s), and 0 exception(s). View

And now including all the fixes again that were in #9.

The last submitted patch, 19: 2368349-entity-view-display-schemas-19.patch, failed testing.

The last submitted patch, 20: 2368349-entity-view-display-schemas-20.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 21: 2368349-entity-view-display-schemas-21-with-all-fixes.patch, failed testing.

Gábor Hojtsy’s picture

Yay new fails in datetime and responsive images. Yay for tests :) Now onto fixing them.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
24.15 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,174 pass(es). View
1.65 KB

So those fails are not schema fails per say, they are incorrect data in the test. The datetime test modifies the display type without removing the setting that is not valid for the new field type (which cannot have settings). The responsive image fail is an extra key in the display data that is not supposed to be there.

I think this will finally be green and is complete for the scope of this issue.

Berdir’s picture

Yeah, we only added those tests for existing field types when porting them to the new API but not for those types that were added initially.

  1. +++ b/core/modules/datetime/src/Tests/DateTimeFieldTest.php
    @@ -9,6 +9,7 @@
    +use Drupal\field\Tests\FieldItemTestBase;
     use Drupal\simpletest\WebTestBase;
    
    @@ -17,7 +18,7 @@
    -class DateTimeFieldTest extends WebTestBase {
    +class DateTimeFieldTest extends FieldItemTestBase {
     
    

    WebTestBase use is not removed here.

  2. +++ b/core/modules/field/src/Tests/FieldItemTestBase.php
    @@ -0,0 +1,41 @@
    + * Base class to automatically assert entity display schemas.
    + */
    +abstract class FieldItemTestBase extends WebTestBase {
    

    I think FieldTypeTestBase would be a better name for this...

  3. +++ b/core/modules/field/src/Tests/FieldItemTestBase.php
    @@ -0,0 +1,41 @@
    +    // Assert schemas for all entity view and form displays.
    +    $names = array_merge(
    +      $this->container->get('config.storage')->listAll('core.entity_view_display.'),
    +      $this->container->get('config.storage')->listAll('core.entity_form_display.')
    +    );
    

    The scope of this issue is specific to form/view displays, but what about extending this to also cover field and field storage configuration? That will us give more test coverage for those cases, which seems like a good idea. And might also uncover more schema issues.

Can't comment on the schema changes, but the fixes look good to me.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

@Berdir: I'll do 1 and 2 once I get a chance. Agreed. On 3, I am happy to open a followup, the schema bug finder found plenty issues in field schemas AFAIR so that is a logical next step. Happy to work on that too :)

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
24.22 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,266 pass(es). View
3.88 KB

Fixed those two and found 2 more unused WebTestBase uses. AFAIS this should be complete and done :) I'll open that followup next and postpone on this one.

Gábor Hojtsy’s picture

Now opened #2370305: Refactor field type configuration schemas for DX, easier to find errors and postponed on this issue.

Looks good to go? :)

vijaycs85’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. Let's get this in.

alexpott’s picture

+++ b/core/modules/field/src/Tests/FieldTypeTestBase.php
@@ -0,0 +1,41 @@
+  protected function tearDown() {
+    // Assert schemas for all entity view and form displays.
+    $names = array_merge(
+      $this->container->get('config.storage')->listAll('core.entity_view_display.'),
+      $this->container->get('config.storage')->listAll('core.entity_form_display.')
+    );
+
+    $factory = $this->container->get('config.factory');
+    /** @var \Drupal\Core\Config\TypedConfigManagerInterface $typed_config */
+    $typed_config = $this->container->get('config.typed');
+    foreach ($names as $name) {
+      $config = $factory->get($name);
+      $this->assertConfigSchema($typed_config, $name, $config->get());
+    }
+
+    parent::tearDown();
+  }

This would seem very susceptible to losing test coverage over time. For example, if a test is changed to delete the displays as part of the test then suddenly something that was tested now is not. And it would be hard to spot.

Gábor Hojtsy’s picture

@alexpott: do you mean we should create tests than that copy-paste essentially all field type initialisation code or somehow tries doing field config with all defaults? Berdir argued above that would be lots of duplicates.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I've got an idea - working on a patch.

Gábor Hojtsy’s picture

FYI the followup at #2370305: Refactor field type configuration schemas for DX, easier to find errors already has a complete patch and is green :) Needs review. (The test part is dependent on this test though).

Gábor Hojtsy’s picture

Also even if new methods of testing are introduced, it would be important to keep the string field test (and text field test inheriting from it) from this patch because that is plugging a hole pretty much in field testing in general.

xjm’s picture

Component: configuration system » entity system
Priority: Major » Critical

Promoting to critical since #2183983: Find hidden configuration schema issues was postponed on this issue. Also I think this is techically an entity system bug.

alexpott’s picture

re #34 the idea grew to become a separate issue since it will easier to review as a standalone patch see #2371843: Add event listener to check schema on config save.

jibran’s picture

Added the related issue.

yched’s picture

Note than no accurate schema for EVD and EFD can be written until #1875974: Abstract 'component type' specific code out of EntityDisplay.
The Displays store several types of components with sub-entries that depend on the type of the component (field, extra field currently + contrib component types). Yet the current code doesn't store the component type.

alexpott’s picture

Status: Needs work » Needs review
FileSize
11.62 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,026 pass(es), 18 fail(s), and 9 exception(s). View
5.61 KB
23.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,012 pass(es), 16 fail(s), and 7 exception(s). View

CHanged the patch around to use the schema checking exception introduced by #2371843: Add event listener to check schema on config save

The test only patch is the patch without any schema fixes.

Status: Needs review » Needs work

The last submitted patch, 41: 2368349-2.41.patch, failed testing.

The last submitted patch, 41: 2368349-2-test-only.41.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.13 KB
25.7 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,147 pass(es). View

This should fix up the schema fails.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

This was RTBC in #31. The interdiffs since then look great, so back to RTBC.

Just a couple questions, but both can be followup material.

  1. +++ b/core/modules/image/config/schema/image.schema.yml
    @@ -60,6 +62,10 @@ image.effect.image_scale:
    +# The image desaturate has no settings.
    +image.effect.image_desaturate:
    +  type: ignore
    

    Should we have something like a none or
    empty type to enforce emptiness instead of allowing it to be anything?

  2. +++ b/core/modules/image/src/Tests/ImageFieldTestBase.php
    @@ -81,13 +81,15 @@ function createImageField($name, $type_name, $storage_settings = array(), $field
    +    $description = !empty($field_settings['description']) ? $field_settings['description'] : '';
    +    unset($field_settings['description']);
         $field_config = entity_create('field_config', array(
    ...
           'required' => !empty($field_settings['required']),
    -      'description' => !empty($field_settings['description']) ? $field_settings['description'] : '',
    +      'description' => $description,
           'settings' => $field_settings,
         ));
    

    Should we make the same change (i.e., add an unset()) for 'required' as well? For that matter, should we change the signature of createImageField() to pass in $description and $required separately from $field_settings?

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

@effulgentsia:

1. We can only have empty lists :) Keys that always need to be empty valued should not be saved into config, right? This case, the ignore type is used for an empty list as well. The correct type to use for that is sequence (without any further definition for items). In that case if the list has any items, config identifies them with an 'undefined' type and will throw errors. Defining it as 'ignore' means IF it has items, config will not throw errors, even though that should not be valid. Ignore removes the item from validation, while defining it as an empty sequence keeps it in the validation proper. So the correct type to use would be sequence. This should be fixed in this issue since its introduced in this issue.

2. I don't have a strong feeling on that one and agreed can be a followup.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
468 bytes
25.7 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,151 pass(es). View
  1. Changed ignore to sequence. So fixed.
  2. This is test code - don't particularly care at this point - and not introduced by this issue. I agree that this is followup material.

Such a minor change that I'm setting this to rtbc.

Gábor Hojtsy’s picture

Agreed on RTBC.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.53 KB
25.8 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,166 pass(es), 10 fail(s), and 0 exception(s). View

Wait, I looked at the issues found in #2370305: Refactor field type configuration schemas for DX, easier to find errors because I realized those changes should not be necessary once this lands, since that would have expanded the schema test coverage, but then this did a lot more so.

BUT the fixes in that patch does not align with fixes here.

1. The image field description related fix is different there but is better here.

2. The decimal separator on decimal fields fix however is not correct here. DecimalItem does not have any field settings for decimal separators. It is incorrectly passed in in the test and just ends up in the settings accidentally. The correct fix is not to set nonexistent field settings, not to document that they exist :)

Closing #2370305: Refactor field type configuration schemas for DX, easier to find errors down as duplicate.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Nice catch.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 49: 2368349-49.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
4.04 KB
29.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,187 pass(es). View

Some newly updated core config used incorrect data types for display_label on checkboxes and pager_id on comments. These were undefined settings keys before this patch and therefore saved as string prior to this patch. Now that they are defined, validating them from default config will fail because they need to be bool and integer respectively.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Yep - yay for Drupal\config\Tests\DefaultConfigTest

yched’s picture

+++ b/core/config/schema/core.data_types.schema.yml
@@ -650,6 +650,9 @@ field.integer.field_settings:
+    size:
+      type: string
+      label: 'Database storage size'

Wow - it's really wrong to have "Database storage size" as a *field* setting rather than a *storage* setting - but that's indeed what IntegerItem does.

Opened #2376689: IntegerItem 'size' setting should be a storage setting for this.

yched’s picture

Assigned: Unassigned » yched
Status: Reviewed & tested by the community » Needs review

Sorry, hold on a bit plz. Reviewing, but cannot post atm.

xjm’s picture

Issue tags: +D8 upgrade path
yched’s picture

So I was bummed by the organization of the schema info: it lets each widget/formatter plugin completely override the schema for $display->content[$field_name] (which is a keyed array with weight, plugin id, label and settings).
This is wrong and not in-line with the actual data model : the widget/formatter only gets to say what's inside 'settings', what's in the level above is not its decision.
This is also not consistent with the way field type plugins express the schema for their settings - field.[field_type].settings is *only* the mapping of settings.

Then I noticed this is already the case in HEAD and is not introduced here - however, getting this straight later will be an API change (changes the way widgets/formatters provide schema info). So - should we fix this as part of the critical here ?

yched’s picture

Assigned: yched » Unassigned

Other than that :

  1. +++ b/core/modules/datetime/src/Tests/DateTimeFieldTest.php
    @@ -141,6 +150,7 @@ function testDateField() {
         $this->display_options['type'] = 'datetime_plain';
    +    $this->display_options['settings'] = array();
    

    OK, so saving a Display with a component that has widget/formatter settings unknown to the widget/formatter contradicts the schema.

    Then it seems EntityDisplay::setComponent() should only accept settings that are correct for the formatter / widget - since saving them would be invalid ?
    Could be done by changing this in (Widget|Formatter)PluginManager::prepareConfiguration() :

      - // Fill in default settings values for the widget.
      - $configuration['settings'] += $this->getDefaultSettings($configuration['type']);
      + // Filter out unknown settings, and fill in defaults for missing settings.
      +  $default_settings = $this->getDefaultSettings($configuration['type']);
      +  $configuration['settings'] = array_intersect_keys($configuration['settings'], $default_settings) + $default_settings;
    
  2. +++ b/core/modules/image/src/Tests/ImageFieldTestBase.php
    @@ -72,13 +81,15 @@ function createImageField($name, $type_name, $storage_settings = array(), $field
    +    $description = !empty($field_settings['description']) ? $field_settings['description'] : '';
    +    unset($field_settings['description']);
    

    Agreed with @effulgentsia that the createImageField() method is sick indeed, passing 'description' next to the actual field settings is really wrong.

    But yeah, fine with a followup - opened #2376899: ImageFieldTestBase::createImageField() takes a description in field settings errorneously

Gábor Hojtsy’s picture

FileSize
31.72 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,258 pass(es), 3 fail(s), and 0 exception(s). View
1.88 KB

@yched:

Re #57: This issue is set out to fix the schema errors not to refactor them completely for displays. If we are to do that then the actual changes will be impossible to review. It the same problem in #2370305: Refactor field type configuration schemas for DX, easier to find errors that the refactoring makes it easy to find errors but the more we fix in the issue the harder it is to review due to the whole rename process. As a sub-issue of #2183983: Find hidden configuration schema issues this issue is supposed to fix errors so we can increase our application of strict schema adherence. Fixing existing errors and adding test coverage is what gets us there and then we can refactor. Now in many cases we don't really know if the schemas are good or not so we need to fix and pass tests first.

Re #58/1: Implemented. Looking forward to any possible new fails for things we may not have recognized :)

Re #58/2: Thanks for opening the followup.

Status: Needs review » Needs work

The last submitted patch, 59: 2368349-59.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
34.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,222 pass(es), 1 fail(s), and 0 exception(s). View
2.44 KB

1. The MigrateDrupal6Test has been asserting setting for integer fields that are not there. Fixed test to be more focused.

2. DisplayApiTest uses a field_test_multiple field type with a made-up 'alter' key to then test altering support in field_test_entity_display_build_alter. By documenting this key on this field type, it will pass along properly. This is a one-off made-up key, not a generic feature, only for testing.

These should fix the test fails with the new stricter code. Good to go now? :)

Status: Needs review » Needs work

The last submitted patch, 61: 2368349-61.patch, failed testing.

yched’s picture

@Gábor : makes sense, thanks!
RTBC +1 when green again.

Gábor Hojtsy queued 59: 2368349-59.patch for re-testing.

Gábor Hojtsy queued 61: 2368349-61.patch for re-testing.

The last submitted patch, 59: 2368349-59.patch, failed testing.

The last submitted patch, 61: 2368349-61.patch, failed testing.

Gábor Hojtsy’s picture

So we need to figure out why my fix works in the context of the whole MigrateDrupal6Test suite but not when running standalone in MigrateFieldFormatterSettingsTest and then we would have cracked it. I have no idea yet, the number/integer field is core :/

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
2.92 KB
37.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,258 pass(es). View

One way to solve this is to assert migration results with settings only relevant for the given field type. Current tests share migration target test config between decimal and integer fields and attempt to set the same keys for both in migrations, which is not good :)

Would really love if we can stop extending the scope of this issue even further now.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

@Gábor Hojtsy yep I agree - let's get this done - scope has crept far enough.

fyi: when run as part of MigrateDrupal6Test the MigrateFieldFormatterSettingsTest::setUp() does not run - this the reason for the difference.

  • catch committed 03d3d53 on 8.0.x
    Issue #2368349 by Gábor Hojtsy, alexpott: Entity view and form display...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks! Agreed on keeping this moving to unblock other work.

@yched can you open the follow-up for the schema re-organisation?

Gábor Hojtsy’s picture

Yay! Posted two versions for the issue at #2376899: ImageFieldTestBase::createImageField() takes a description in field settings errorneously which was our first followup.

Also sent #2370305: Refactor field type configuration schemas for DX, easier to find errors for a retest, which is our next big schema issue to hunt down :)

yched’s picture

@catch: opened #2378055: Reorganise config schema for entity_form_display / entity_view_display.
Since that will be an API break, committers green-light is welcome over there :-)

Status: Fixed » Closed (fixed)

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