Problem/Motivation

A big set of fails in #2183983: Find hidden configuration schema issues shows there are missing field.*.storage_settings schemas (for fields that don't have storage settings). Unfortunately it is not possible to provide such a wildcard fallback pattern with an empty sequence because wildcards would only be allowed at the end. So the only way to fix those issues without changing the naming of these field schema components is to copy paste empty settings sequences to all the schemas that need them. That sounds like a waste of time and wrong DX. Also the dynamic portion in the middle is not consistent with other dynamic typing in config schemas.

Proposed resolution

1. We need to rework the internal schema names related to fields to be consistent with other names with dynamic components and to be able to provide a wildcard fallback.

2. This allows the introduction of an empty sequence fallback for field.storage_settings.*. If the fields that fall back to this schema do have settings, that will fail on any schema testing (including #2183983: Find hidden configuration schema issues). Those (test) fields that actually don't have settings don't need to define this schema. (I kept the schema definitions of empty sequence storage settings for some core fields because they also relabel the settings key to be more specific).

Remaining tasks

Review. Commit.

User interface changes

None.

API changes

The schema changes only concern keys used internally in schemas. Modules defining fields / field schemas are affected but no configuration keys need to change.

Files: 
CommentFileSizeAuthor
#60 interdiff.txt782 bytesyched
#60 2370305-field-type-schema-60.patch30.19 KByched
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,717 pass(es). View
#53 2370305-field-type-schema-53.patch30.15 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2370305-field-type-schema-53.patch. Unable to apply patch. See the log in the details link for more information. View
#51 2370305-field-type-schema-48.patch30.27 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2370305-field-type-schema-48_0.patch. Unable to apply patch. See the log in the details link for more information. View
#49 interdiff.txt7.18 KBGábor Hojtsy
#49 2370305-field-type-schema-49.patch30.46 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,472 pass(es). View
#48 interdiff.txt938 bytesGábor Hojtsy
#48 2370305-field-type-schema-48.patch30.27 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,423 pass(es). View
#46 interdiff.txt541 bytesGábor Hojtsy
#46 2370305-field-type-schema-46.patch30.31 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,364 pass(es). View
#44 interdiff.txt13.44 KBGábor Hojtsy
#44 2370305-field-type-schema-44.patch30.3 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,332 pass(es), 72 fail(s), and 0 exception(s). View
#43 interdiff.txt2.39 KBGábor Hojtsy
#43 2370305-field-type-schema-43.patch25.52 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,380 pass(es). View
#40 interdiff.txt3.56 KBGábor Hojtsy
#40 2370305-field-type-schema-39.patch23.12 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,381 pass(es), 4 fail(s), and 4 exception(s). View
#35 interdiff.txt3.23 KBGábor Hojtsy
#35 2370305-field-type-schema-35.patch21.17 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,892 pass(es), 880 fail(s), and 2,112 exception(s). View
#34 2370305-field-type-schema-34.patch17.94 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,387 pass(es). View
#34 interdiff.txt5.14 KBGábor Hojtsy
#33 interdiff.txt1.05 KBGábor Hojtsy
#33 2370305-field-type-schema-33.patch17.79 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,390 pass(es). View
#27 2370305-field-type-schema-27.patch17.7 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,376 pass(es), 12 fail(s), and 0 exception(s). View
#22 interdiff.txt1.03 KBGábor Hojtsy
#22 2370305-field-type-schema-22.patch17.68 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2370305-field-type-schema-22_0.patch. Unable to apply patch. See the log in the details link for more information. View
#21 2370305-field-type-schema-22.patch17.58 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,163 pass(es). View
#21 interdiff.txt2.14 KBGábor Hojtsy
#20 2370305-field-type-schema-20.patch16.52 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,171 pass(es). View
#20 interdiff.txt506 bytesGábor Hojtsy
#16 interdiff.txt481 bytesGábor Hojtsy
#16 2370305-field-type-schema-15.patch16.48 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,175 pass(es), 2 fail(s), and 0 exception(s). View
#15 interdiff.txt6.88 KBGábor Hojtsy
#15 2370305-field-type-schema-14.patch16.42 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,156 pass(es), 2 fail(s), and 0 exception(s). View
#13 2370305-field-type-schema-13.patch13.26 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,147 pass(es), 2 fail(s), and 0 exception(s). View
#8 interdiff.txt8.08 KBGábor Hojtsy
#8 2370305-field-type-test-schema-8.patch25.49 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,434 pass(es), 3 fail(s), and 0 exception(s). View
#7 interdiff.txt487 bytesGábor Hojtsy
#7 2370305-field-type-test-schema-7.patch21.23 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,334 pass(es). View
#5 interdiff.txt9 KBGábor Hojtsy
#5 2370305-field-type-test-schema-5.patch21.23 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,325 pass(es), 6 fail(s), and 0 exception(s). View
#3 interdiff.txt1.58 KBGábor Hojtsy
#3 2370305-field-type-test-schema-3.patch12.46 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,310 pass(es). View
#1 2370305-field-type-test-schema.patch11.16 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,304 pass(es), 4 fail(s), and 0 exception(s). View

Comments

Gábor Hojtsy’s picture

Status: Postponed » Needs review
FileSize
11.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,304 pass(es), 4 fail(s), and 0 exception(s). View

Here is a roll of the patch from #2368349: Entity view and form display configuration schemas are too verbose / key ones missing with only the tests from there to target field.field.* and field.storage.*. Since that patch is not committed, this should only be considered as a fail generator. Once #2368349: Entity view and form display configuration schemas are too verbose / key ones missing is committed, we can make that part only a couple lines of diff :)

Status: Needs review » Needs work

The last submitted patch, 1: 2370305-field-type-test-schema.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
12.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,310 pass(es). View
1.58 KB

Fixes for all the fails:

1. Integer fields have a 'size' setting that was not in the schema. I assume that was added since the schema.

  public static function defaultFieldSettings() {
    return array(
      'min' => '',
      'max' => '',
      'prefix' => '',
      'suffix' => '',
      // Valid size property values include: 'tiny', 'small', 'medium', 'normal'
      // and 'big'.
      'size' => 'normal',
    ) + parent::defaultFieldSettings();
  }

2. NumberFieldTest sets a decimal_separator field setting for the test field, but that is a display setting, not a field setting.

3. ImageFieldDisplayTest.php sets a 'description' field setting key for image fields, but there is no such setting at all among field settings.

Gábor Hojtsy’s picture

Issue summary: View changes

Yay, that was easier than expected :) Patch to review is the interdiff in #3.

Gábor Hojtsy’s picture

FileSize
21.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,325 pass(es), 6 fail(s), and 0 exception(s). View
9 KB

Actually the next set of fails in #2183983: Find hidden configuration schema issues shows there are lots of fails related to missing field.*.storage_settings schemas. Unfortunately it is not possible to provide such a wildcard fallback pattern with an empty sequence because wildcards would only be allowed at the end. So we need to rework the internal schema names of field storage settings. (We should rename the others also for consistency, but for now let's just do one step at a time).

This allows the introduction of an empty sequence for field.storage_settings.*. If the fields that fall back to this schema do have settings, that will fail on #2183983: Find hidden configuration schema issues but at least those (test) fields that actually don't have settings don't need to define this schema. (OTOH kept the schema definitions of empty sequence storage settings for some core fields because they also relabel the settings key to be more specific).

It should be a policy to put dynamic parts of schema elements at the end so we can do these fallbacks.

Status: Needs review » Needs work

The last submitted patch, 5: 2370305-field-type-test-schema-5.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
21.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,334 pass(es). View
487 bytes

Typo.

Gábor Hojtsy’s picture

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

Also refactoring field settings the same way and adding default fallback empty sequence in the same way as storage settings.

Status: Needs review » Needs work

The last submitted patch, 8: 2370305-field-type-test-schema-8.patch, failed testing.

The last submitted patch, 8: 2370305-field-type-test-schema-8.patch, failed testing.

Gábor Hojtsy’s picture

Title: Field type configuration schemas are not tested » Refactor field type configuration schemas for DX, easier to find errors

The fixes are duplicate of #2368349: Entity view and form display configuration schemas are too verbose / key ones missing. I think for DX and finding errors, it better if we refactor this the way proposed here, so actually keeping it open for that.

Gábor Hojtsy’s picture

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

Rolled a version with only the refactors of the schema and the minor label / empty mapping to empty sequence fixes.

Status: Needs review » Needs work

The last submitted patch, 13: 2370305-field-type-schema-13.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
16.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,156 pass(es), 2 fail(s), and 0 exception(s). View
6.88 KB

Also refactored the field.*.value schemas to field.value.* for consistency. The datetime and especially the file schemas look veeeery suspicious to not be correct.

Gábor Hojtsy’s picture

FileSize
16.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,175 pass(es), 2 fail(s), and 0 exception(s). View
481 bytes

Add a wildcard element as well.

The last submitted patch, 15: 2370305-field-type-schema-14.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 16: 2370305-field-type-schema-15.patch, failed testing.

xjm’s picture

Issue tags: +D8 upgrade path
Gábor Hojtsy’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
506 bytes
16.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,171 pass(es). View

Duh, the fail was due to my misnamed rename.

Gábor Hojtsy’s picture

FileSize
2.14 KB
17.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,163 pass(es). View

So I looked into the datetime and file field default value schema. Datetime has these keys (when set to current date, same keys when set to a concrete date):

default_value:
  -
    default_date_type: now
    default_date: now

This is TOTALLY not how it is in the schema. In fact the schema defines two type subkeys for the sequence which is totally bogus.

As for file fields, they don't support a default file, so that should be an empty sequence.

Finally, I looked at image fields for comparison and noticed we have a data type we can fully reuse for the default image mapping, no need to redefine that.

Gábor Hojtsy’s picture

FileSize
17.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2370305-field-type-schema-22_0.patch. Unable to apply patch. See the log in the details link for more information. View
1.03 KB

Found a typo in my float label change and fixing two minor comment labels and removing an anonymous sequence that I found on comment settings. I think this well concludes the scope. It would be dangerous to go further for reviewability.

Also in terms of test coverage, we are almost only do renames here, and the rest of the changes are more restrictive than before so if they pass current tests, then they are good as far as our tests go.

Status: Needs review » Needs work

The last submitted patch, 22: 2370305-field-type-schema-22.patch, failed testing.

yched’s picture

Great cleanup!

Couple thoughts :

- Like we did in #2368349: Entity view and form display configuration schemas are too verbose / key ones missing, we should probably make sure we filter out unknown field / storage settings on preSave() in FieldConfigBase / FieldStorageConfig ?

- it's a shame the only way to describe an empty mapping is to pretend it's a 'sequence' :-(. Why can't schema understand empty mappings ?
Having field.field_settings.* described as a 'sequence' is kind of a lie, and having each field.field_settings.[%field_type] say "actually no, mine is a mapping" is a bit hackish. It's a mapping in all cases, and in some cases it's empty.

Ideally, we'd have :

field_config_base:
  ...
  settings:
      type: mapping
      mapping: field.field_settings.[%parent.field_type]
field.field_settings.*:
  (how do you say "empty" ?)
field.field_settings.some_field_type:
  - setting_foo: ...
  - setting_bar: ...

But if not doable for some reason (I'm not fully familiar with what schema structuring lets you express), at least having them all say they are 'mappings' would be less confusing.

The last submitted patch, 22: 2370305-field-type-schema-22.patch, failed testing.

Gábor Hojtsy’s picture

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

Just a reroll first.

Status: Needs review » Needs work

The last submitted patch, 27: 2370305-field-type-schema-27.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 27: 2370305-field-type-schema-27.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 27: 2370305-field-type-schema-27.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
17.79 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,390 pass(es). View
1.05 KB

Fixing the errors for now. The cleanup and tests helped the missing comment setting surface as well as a typo in my date field schema fix. Should hopefully pass now.

Gábor Hojtsy’s picture

FileSize
5.14 KB
17.94 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,387 pass(es). View

Now modifying empty settings to be mapping as per @yched's suggestion. We can do that. I erroneously assumed we can only do empty lists as sequences but then checking the code, there is no reason to not use empty mappings.

That required changing the "anonymous string array" on email storage/field settings, float storage settings to empty mapping (it does not have settings) and email storage settings to a mapping with an unsigned key :) The rest is just sequences to mappings.

@yched: one key thing here is the default field values define the whole container numeric indexed array as well in each instance (and therefore their empty variants are defined as empty sequences correctly). However is it possible for fields to define their default value structure outside of the array element? I mean should we expose this data definition level in the schema or just let the array element be defined for each field type, not the outer sequence? In short, which one should it be?

field.value.text_long:
  type: sequence
  label: 'Default value'
  sequence:
    - type: mapping
      label: 'Default'
      mapping:
        value:
          type: text
          label: 'Value'
        format:
          type: string
          label: 'Text format'

or

field.value.text_long:
  type: mapping
  label: 'Default'
  mapping:
    value:
      type: text
      label: 'Value'
    format:
      type: string
      label: 'Text format'
Gábor Hojtsy’s picture

FileSize
21.17 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,892 pass(es), 880 fail(s), and 2,112 exception(s). View
3.23 KB

Addressing the rest of @yched's review in #24. Adding preSave() in FieldConfigBase to filter settings and consolidate the new/updated preSave() code common on FieldStorageConfig to preSave() so it filters and adds defaults there.

The only outstanding question then is mine from #34 and any fails :)

Status: Needs review » Needs work

The last submitted patch, 35: 2370305-field-type-schema-35.patch, failed testing.

yched’s picture

@Gábor #34 : whatever the field type, FieldConfig->default_value is always a numeric array of values (each value being a mapping dependant on the field type)

So the second approach is more correct :

field.field.*.*.*:
  ...
  default_value:
    type: sequence
    label: 'Default value'
    sequence:
      - type: field.value.[field_type]

field.value.text_long:
  type: mapping
  /* no need for a "label" here, if we can avoid it ? */
  mapping:
    property_1: ...
    property_2: ...

re #35:

+++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
@@ -270,6 +270,15 @@ public function postCreate(EntityStorageInterface $storage) {
+  public function preSave(EntityStorageInterface $storage) {
+    // Filter out unknown settings.
+    $default_settings = $this->getFieldStorageDefinition()->getSettings();
+    $this->settings = array_intersect_key($this->settings, $default_settings);

Sounds wrong, probably accounts for lots of the fails:
- Only intersect, no "+ defaults" here ?
- We shouldn't be asking for the *storage* settings there, we don't want to save the storage settings in the field.

Why not simply :

$default = $field_type_manager->getDefaultFieldSettings();
$this->settings = array_intersect($this->settings, $default) + $default;

like you do in FieldStorageConfig ?

+ actually, the "merge defaults" is currently done separately in FieldConfig::preSave() and BaseFieldOverride::preSave(), so it should be updated there to "intersect + merge" instead of just "merge".

As a followup, it's very likely that this should indeed be moved up to a parent preSave() in FieldConfigBase, but there might be gremlins there, so, it's probably simpler to not refactor here and just change the code where it currently is ?

Gábor Hojtsy’s picture

@yched: so the code in FieldConfigBase::getSetting() and getSettings() both merge the storage settings:

  public function getSettings() {
    return $this->settings + $this->getFieldStorageDefinition()->getSettings();
  }
  public function getSetting($setting_name) {
    if (array_key_exists($setting_name, $this->settings)) {
      return $this->settings[$setting_name];
    }
    else {
      return $this->getFieldStorageDefinition()->getSetting($setting_name);
    }
  }

So I used the same logic in my preSave that I added. Looks like that is wrong and I should use getDefaultFieldSettings() which is BTW not yet even used in FieldConfigBase?! Also I don't see any effort to merge in default settings here, which is why I did not do that. I don't want to change the behavior as to what extent are defaults saved if they are not already merged because that does not seem to be in scope to filtering :) Do you suggest we should do that for consistency even though it is not done yet?

yched’s picture

FieldConfigBase::getSettings() is a runtime getter that returns the merge of the field settings ($this->settings) and the storage settings - but $this->settings itself is only ever about field settings, not storage.

Merging defaults is done at save-time : look at the existing preSave() implementations in FieldConfig and BaseFieldOverride. That's IMO that merge code that should be modified where it currently is (filter + merge instead of just merge) ?

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
23.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,381 pass(es), 4 fail(s), and 4 exception(s). View
3.56 KB

Implemented the field settings filtering in BaseFieldOverride.php and FieldConfig.php.

yched’s picture

Side note : #2376689: IntegerItem 'size' setting should be a storage setting will slightly conflict - no biggie.

Status: Needs review » Needs work

The last submitted patch, 40: 2370305-field-type-schema-39.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
25.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,380 pass(es). View
2.39 KB

The remaining fails are about image field column groups, which migrations think that should be duplicated in a column_groups field setting (which is nonexistent, column groups exist in field type info not in field settings). Those keys/values are already in the widget settings, no need to duplicate under the image field where they are not valid.

Yay for tests and settings filtering :) :)

Gábor Hojtsy’s picture

FileSize
30.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,332 pass(es), 72 fail(s), and 0 exception(s). View
13.44 KB

All right, finally the field values are now mappings and not sequences of mappings, we define the sequence for them since they should not define anything else anyway. IMHO we are done here so long as I did not make any mistakes and this is still green :)

Status: Needs review » Needs work

The last submitted patch, 44: 2370305-field-type-schema-44.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
30.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,364 pass(es). View
541 bytes

We need to look two parents up for the field type now that we put that under a sequence.

yched’s picture

Status: Needs review » Needs work

Awesome ! Thanks a lot @Gábor.

Needs a reroll after #2376689: IntegerItem 'size' setting should be a storage setting (moved 'unsigned' and 'size' to 'field.integer.storage_settings', that is renamed to 'field.storage_settings.integer' here)

  1. +++ b/core/config/schema/core.data_types.schema.yml
    @@ -378,12 +378,16 @@ field_config_base:
         default_value:
    -      type: field.[%parent.field_type].value
    +      type: sequence
    +      label: 'Default values'
    +      sequence:
    +        - type: field.value.[%parent.%parent.field_type]
    +          label: 'Default value'
    

    Looks like this second 'label' here is duplicated / overriden in field.value.* and in each actual field.value.[field_type]. Some of them could go ?

    Actually, there might be other places in config that store "field values", and thus use the field.value.* schema, for other stuff than "Default value".
    field_config_base.default_value *knows* it uses field.value.* for "Default values", but field.value.[field_type] itself doesn't know what it's used for. It just holds the schema for a field item of that field type. More on that in another point below.

  2. +++ b/core/modules/comment/config/schema/comment.schema.yml
    @@ -56,40 +56,38 @@ comment.type.*:
    +    status:
    +      type: integer
    +      label: 'Status'
    +    cid:
    +      type: integer
    +      label: 'Status'
    

    present in HEAD, but one of the labels lies :-)

  3. +++ b/core/modules/image/config/schema/image.schema.yml
    @@ -117,28 +117,9 @@ field.image.field_settings:
    -field.image.value:
    -  type: sequence
    +field.value.image:
    +  type: field_default_image
       label: 'Default value'
    -  sequence:
    -    - type: mapping
    -      label: 'Default image'
    -      mapping:
    -        fid:
    -          type: integer
    -          label: 'File ID'
    -        alt:
    -          type: string
    -          label: 'Alternative text'
    -        title:
    -          type: string
    -          label: 'Title text'
    -        width:
    -          type: integer
    -          label: 'Width'
    -        height:
    -          type: integer
    -          label: 'Height'
    

    Actually, as per the above, I think field.image.value should stay and field_default_image should go : there's a schema for "an image item", and some pieces of config (the 'default_image' settings) use it.

    In that same reasoning, there should be a field.value.file with an actual mapping for "the schema of a file value" - even though the field's default_value will in reality always be empty for image fields - that one might be for a followup though.

  4. +++ b/core/modules/datetime/config/schema/datetime.schema.yml
    @@ -8,23 +8,20 @@ field.datetime.storage_settings:
    +field.value.datetime:
    +  type: mapping
       label: 'Default value'
    -  sequence:
    -    - type: sequence
    -      label: 'Default value'
    -      sequence:
    -        - type: string
    -          label: 'Type'
    -        - type: string
    -          label: 'Value'
    +  mapping:
    +    default_date_type:
    +      type: string
    +      label: 'Default date type'
    +    default_date:
    +      type: string
    +      label: 'Default date value'
    

    OK, crap. Date fields do special stuff with their default values, so this is the correct schema for default values, but not the actual schema for a "date item".

    In fact, thing are a bit more complex than what I wrote in #34 ("whatever the field type, FieldConfig->default_value is always a numeric array of values") :-/.

    Proposal :

    field_config_base:
      ...
      default_value:
        field.default_value.[%parent.field_type]
    field.default_value.* :
      type: sequence
      label: 'Default values'
      sequence:
        - type: field.item.[%parent.%parent.field_type]
    

    Then :
    - each field type has to provide a field.item.[field_type] for "the schema for my items, whatever may use it"
    - by default this is what we use for the items found in field_config_base 'default_value'
    - but individual field types can override that and say that their 'default_value' is different (image and file can say that it's an empty sequence, date can say that its default values contain something different than its "items")

  5. Does that sound reasonable ?

Gábor Hojtsy’s picture

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

Rerolled following #2376689: IntegerItem 'size' setting should be a storage setting. Fixed all the comment labels from the CommentItem::propertyDefinitions labels.

I would really love to get this in because we are modifying way enough for this to conflict with whatever happens, and we are bazillions of config schema fails away from having correct schemas in core, so if we are to follow this detail in fixing them then I'll be busy with this meta issue in 2016.

What's the use case for a field.item.* structure? So far config is not used to store field data in core anywhere? Or is it? Where are those defined now then? Even if they do, why would we always fall back on that for default values and allow overrides via the sequence, not the item level?

Gábor Hojtsy’s picture

FileSize
30.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,472 pass(es). View
7.18 KB

Renaming field.value.* to field.default_value.* to disambiguate.

yched’s picture

Short answer : OK, sorry - yes, I'm indeed guilty of seeing this from the perspective of my system and not the critical meta issue. Sorry for being a pain.

I'll try to detail the rationale for the current field.value.[field_type] in HEAD and the proposal in #50.4, but I'm fine with refining this in a narrower followup. If possible, I'd rather keep field.value.* rather than field.default_value.*, though, for the same reasons below.

What's the use case for a field.item.* structure? So far config is not used to store field data in core anywhere? Or is it? Where are those defined now then?

Thing is : I don't know, and config schema is complex :-/ We have a collection of widely pluggable systems (fields, views - some in contrib: rules, panels...), talking about each other, and formalizing what info needs to be available for config schemas is not easy.
Views filters store a condition on a value of a field. I'm guessing Rules might have similar cases ?

I'm trying to avoid the situation where contrib field type "Foo" cannot be used by Panels or Rules or some other, less known contrib system because it fails to expose the schema that specific system needs - or avoid the explosion of schema snippets a field type needs to provide in various shapes for various specific systems, which would be brittle and eventually fall out of sync.

So I'm trying to come with "generic pieces of config schema a field type needs to provide, not hardcoded to some specific use by some specific ConfigEntity", because - well, you don't know who's gonna save "things about your field type" in some configuration somewhere.

You're a field type, what schema is there to know about you that differs from other field types ?
- your settings (field setting, storage settings)
- and the structure of your items

We have the former : that's field.(field|storage)_settings.[field_type]
The latter was was the intent of the current field.value.[field_type] in HEAD so far : not tied to the notion of "default value in FieldConfig entities", that is only one consumer for that information - but not always, some field types do differently.

So the proposal in #50.4 was an attempt to keep that structure:
- Have the "generic" info the schema of items of a field type. We already have it in current core, it's field.value.[field_type]. Renaming to item is just a proposal to remove the weirdness with of "default_value (singluar) is a list of several 'value'". "Item" is the general nomenclature in the code for what field.value.[field_type] is about.
- Use it by default for FieldConfig default_value.
- But keep a dedicated mounting point to let field types that do different stuff for their default values override that with something different.

Gábor Hojtsy’s picture

FileSize
30.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2370305-field-type-schema-48_0.patch. Unable to apply patch. See the log in the details link for more information. View

All right. Reuploading #48 then as the proposal for the scope of this patch. It is critical to resolve the schema bugs but it certainly is not critical to add in more reusable components for contrib. I see that you would understand field.value.* as being the value structure, which is why I renamed it to default_value to avoid confusion, because it is used for default values and contains the default value structures NOT the value structures. But if you want to keep it as the (for me) more confusing value name then that is fine for the scope of this issue.

I think we went as far and probably farther on this issue than we should have :)

Status: Needs review » Needs work

The last submitted patch, 51: 2370305-field-type-schema-48.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
30.15 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2370305-field-type-schema-53.patch. Unable to apply patch. See the log in the details link for more information. View

Rerolled to apply to current head.

Gábor Hojtsy’s picture

BTW for now I need to use the "old" field schema structures and names in other issues like #2379697: Fix configuration schema issues in block content (indirectly link and field test) modules until this lands, so that fuels my feeling of this needing to land sooner than later even more :)

yched’s picture

Status: Needs review » Reviewed & tested by the community

Thanks a lot @Gábor. I'll open a separate issue for the dafault_value / value thing.

yched’s picture

For some reason this doesn't apply at home (hunk about field.float.value fails) but I can't seem to find an actual conflict.
Asking for a re-test.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 53: 2370305-field-type-schema-53.patch, failed testing.

alexpott’s picture

Issue tags: +Needs reroll

Doesn't apply on my commit checkout so...

git ac https://www.drupal.org/files/issues/2370305-field-type-schema-53.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 30875  100 30875    0     0  67970      0 --:--:-- --:--:-- --:--:-- 70170
error: patch failed: core/config/schema/core.data_types.schema.yml:727
error: core/config/schema/core.data_types.schema.yml: patch does not apply
yched’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
30.19 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,717 pass(es). View
782 bytes

Reroll.
Interdiff is the hunk I had to apply manually

yched’s picture

Also : #2380391: Fix storage settings StringLongItem just fixed some incorret settings in StringLongItem field type.
I opened #2381079: Adjust storage_settings schema for string_long field type to fix the schema accordingly, and postponed it on this one here.

effulgentsia’s picture

Priority: Major » Critical

According to #2183983-119: Find hidden configuration schema issues, this is blocking that issue, so raising to critical.

  • catch committed 7822393 on 8.0.x
    Issue #2370305 by Gábor Hojtsy, yched: Refactor field type configuration...
catch’s picture

Status: Reviewed & tested by the community » Fixed

This would be a big improvement on fragility despite the small API change for field formatters (and makes the API much more honest/learnable).

Since it's also blocking a critical issue now too, definitely fine during beta.

Committed/pushed to 8.0.x, thanks!

Gábor Hojtsy’s picture

Yay, thanks! Published the draft change records https://www.drupal.org/node/2381115 and https://www.drupal.org/node/2381105.

yched’s picture

jibran’s picture

+++ b/core/modules/field/src/Entity/FieldConfig.php
@@ -138,9 +138,13 @@ public function preSave(EntityStorageInterface $storage) {
+    // Filter out unknown settings and make sure all settings are present, so
+    // that a complete field definition is passed to the various hooks and
+    // written to config.
+    $default_settings = $field_type_manager->getDefaultFieldSettings($storage_definition->type);
+    $this->settings = array_intersect_key($this->settings, $default_settings) + $default_settings;

This change has broken the DER patch see #2346273-21: Allow user to select bundles per entity type on field settings page.
@yched could you please help me out there? So that I can fix the problem.

Status: Fixed » Closed (fixed)

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