Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler’s picture

Issue tags: +D8MI, +sprint

Adding D8MI and sprint tags. I think people could actually start working on this before the parent issue is in.

Gábor Hojtsy’s picture

A bunch of more tags that we identify these issues with :D

@swentel/@tstoeckler: is the structure of config well defined now?

yched’s picture

is the structure of config well defined now?

Yes :-)

Gábor Hojtsy’s picture

Looks like relevant parts of that patch:

core/modules/field/tests/modules/field_test_config/config/field.field.field_test_import.yml

id: field_test_import
uuid: fb38277f-1fd4-49d5-8d09-9d7037fdcce9
langcode: und
type: text
settings:
  max_length: '255'
module: text
active: 1
entity_types: {  }
storage:
  type: field_sql_storage
  settings: {  }
  module: field_sql_storage
  active: 1
locked: '0'
cardinality: '1'
translatable: false
indexes:
  format:
    - format

core/modules/field/tests/modules/field_test_config/config/field.instance.test_entity.test_bundle.field_test_import.yml

id: test_entity.test_bundle.field_test_import
uuid: 392b4e9d-6157-412e-9603-3d622512f498
langcode: und
field_uuid: fb38277f-1fd4-49d5-8d09-9d7037fdcce9
entity_type: test_entity
bundle: test_bundle
label: 'Test import field'
description: ''
required: 0
default_value: ''
default_value_function: ''
settings:
  text_processing: '0'
  user_register_form: false
widget:
  weight: '-2'
  type: text_textfield
  module: text
  settings:
    size: '60'
yched’s picture

Thanks @Gabor.
Sorry, I should have made the copy/paste work.

Just pushed a small change for the 'default_value' property in those sample files
It's an array, so when empty it gets exported as { } instead of ''

The link for the latest sample files in the sandbox are :
field.field.[field_name].yml
field.instance.[entity_type].[bundle].[field_name].yml

[edit: added those to the issue summary]

yched’s picture

The "tricky" bits in there are :

In field.field.[field_name].yml :
- settings: An array of settings defined by the field type ('settings' entry in hook_field_info)
The schema for this needs to be provided by the module providing the field type.
- storage.settings: An array of settings defined by the field storage type ('settings' entry in hook_field_storage_info)
The schema for this needs to be provided by the module providing the field storage.

In field.instance.[entity_type].[bundle].[field_name].yml :
- settings: An array of settings defined by the field type ('instance settings' entry in hook_field_info)
The schema for this needs to be provided by the module providing the field type.
- widget.settings: An array of settings defined by the widget ('settings' entry in the plugin annotation)
The schema for this needs to be provided by the module providing the widget.
- default_value: A numerically indexed array of field values.
Each value is a hash array of "columns", which depends on the field type ('columns' entry in hook_field_schema)
The schema for this needs to be provided by the module providing the field type.

Examples:
For a text_with_summary field :

$field['default_value'] = array(
  array(
    'value' => 'Some text 1',
    'summary' => 'Some other text 1',
    'format' => 'basic_html',
  ),
  array(
    'value' => 'Some text 2',
    'summary' => 'Some other text 2',
    'format' => 'full_html',
  ),
);

For a number_integer field :

$field['default_value'] = array(
  array(
    'value' => 42,
  ),
);
swentel’s picture

Issue tags: +Field API

tagging

yched’s picture

Status: Postponed » Active

Un-postponing.

andypost’s picture

Very interesting case is "swappable settings" CMI does not allow parts of config to be injected by other modules.

yched’s picture

@andypost: "CMI does not allow parts of config to be injected by other modules"

Not sure what you refer to exactly, but we made sure that the config schema syntax supports including schema snippets provided by other modules.

vijaycs85’s picture

Gábor Hojtsy’s picture

Status: Active » Needs review

Wow, thanks! That looks fancy :) @yched/@swentel what do you think?

yched’s picture

Status: Needs review » Needs work

W00t ! Looks nice indeed, thanks @vijaycs85 !
Side note : could you edit your #11 and remove the embedded screenshots ? They are awesome, but so big they make scrolling back and forth a bit tedious - having them is links should be fine :-)

Couple remarks :

field.field.*:
+    indexes:
+      type: field.index.[%parent.type]
+      label: 'Indexes'
+      sequence:
+        - type: string
+          label: 'Index'

The type doesn't look good, 'indexes' is just an array of strings, there should be no need to route to a field.index.[%parent.type]

field.instance.*.*.*:
+    default_value:
+      type: field_instance.[%parent.widget.type].default

- 'default_value' is actually a sequence of values, each value being a mapping that depends on the field type (see #6 above)
- %parent.widget.type is wrong, this depends on the field type, not the widget type. Same for 'settings'.

The probem is that the field type is actually not present in the field.instance.*.*.*.yml file itself, it is in the field.field.[field_id].yml file. So, out of reach for the current schema include mechanism.
And the instance file doesn't even contain 'field_name', for that matter - it only contains field_uuid.

Possible approaches:
a) We could duplicate the 'field_type' info in the instance file, but I'm not too fond of the idea :-(.
That's not something you'd want to actually configure here, and introduces the possibility of mismatches.
b) There are talks about having the instance file contain the field_id (machine name) instead of the field_uuid.
*If* we had that, would it be possible to have a syntax like
field_instance.["the value of 'type' in field.field.[%parent.field_id].yml"].settings
I.e routing based on a value in a different file, rather than a value in the same file ?

Gábor Hojtsy’s picture

@yched: well, I think dynamic types are complex as-is probably, referring values from other files would be a pretty big addition (and possible DX WTF). Can't we somehow get the data we need in this file, its just a simple key? (And has been available everywhere else where we needed to introduce schemas) :)

yched’s picture

- 'default_value' is actually a sequence of values, each value being a mapping that depends on the field type (see #6 above)

OK, I see that you take care of this in each 'field_instance.*.default' schema.
If doable, I'd suggest putting the "sequence" info directly in field.instance.*.*.* : 'default_value', and route to a 'field_instance.*.value'

vijaycs85’s picture

Status: Needs review » Needs work

Thanks for the review @yched. here is some updates on #13 and #15

1. side note - Fixed.
2. The type doesn't look good, 'indexes' is just an array of strings, there should be no need to route to a field.index.[%parent.type] - field.field.field_image.yml is the only settings got index and it looks like below:

indexes:
  fid:
    - fid

So I'm not sure how to get the 'fid' part for different type with out dynamic type. or we might need to fix the saving part so that, it would save just:

indexes:
  - fid

3. If doable, I'd suggest putting the "sequence" info directly in field.instance.*.*.* : 'default_value', and route to a 'field_instance.*.value' - This is default for text area

    summary: ''
    value: "<p>testing your details...</p>\r\n"
    format: basic_html

I guess, these fields would change for different field type. So not sure about *.value change

yched’s picture

Can't we somehow get the data we need in this file, its just a simple key? (And has been available everywhere else where we needed to introduce schemas) :)

Well, it's just because we haven't encountered the use case before ;-), but I'm not sure the pattern is that exceptionnal.

Frankly, I'm torn on that one :-./
Sure, we can easily hack a quick workaround that would duplicate the 'type' of the field, I'll try to come up with some code shortly.

But denormalizing config data seems like a bad idea, makes the whole system more fragile, and will IMO confuse people when they look at the content of their config files and derive a mental model. An instance has no "type" of its own, and putting one is cheating, really.

yched’s picture

re @vijaycs85 #16:

- indexes:
What you find in there is an array similar to the 'indexes' entry in hook_schema():

array(
  'arbitrary_name_for_index_1' => array('column_1', 'column_2),
  'arbitrary_name_for_index_2' => array('column_3'),
)

I'm not sure how this can be expressed in terms of config schema, but it's definitely not tied to the field type. (I mean, the set of valid columns is dependent on the field type, but that's out of scope here).

- default_value:

I guess, these fields would change for different field type

Absolutely, hence my proposal of field_instance.*.value.
Something like :

field.instance.*.*.*:
  default_value:
    type: sequence
    label: 'Default value'
    sequence:
      - type: field_instance.[%parent.field_type].value

If that's doable ?
(well, aside that we don't have [%parent.field_type] yet, that's #17)

(also, typo :+ label: 'Defaule value'

yched’s picture

Attached patch adds a 'field_type' entry in field.instance.*.yml files. Still eww, though :-/.

Don't want to interfere with @vijaycs85, so I didn't fix the schema file to use it (field.instance.*.*.*: 'default_value' and 'settings')

yched’s picture

Oops, fix wrong parenthesing.

Gábor Hojtsy’s picture

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
@@ -314,9 +314,9 @@
+    // Adiitionally, include the field type, that is needed to be able to

typo :)

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
12.94 KB

Hi @yched, on #18:

1. FIXED: Index - it is indexed array now

2. NOT FIXED: Default value - Can't fix as we have issue with using %parent inside sequence. As explained here: https://gist.github.com/vijaycs85/5402151

3. FIXED - typo in #21

Can't take diff because of mixed commits.

yched’s picture

#22 looks nice.

The fact that doing something like this:

field.instance.*.*.*:
  default_value:
    type: sequence
    label: 'Default value'
    sequence:
      - type: field_instance.[%parent.field_type].value

doesn't work is a bit unfortunate. It means we have to trust each and every 'field_instance.*.value' snippet to duplicate the "type: sequence, label: 'Default value'" info correctly.
Plus, technically, It's the container ($instance ConfigEntity) that states that $instance->default_value, $instance->settings... are arrays. It's not true that each field type can decide what happens in there. Would be better if the structure of schema reflected that.
Is there an issue open for this limitation somewhere ?

Other than that, the includes right now are:
field.settings.[field_type]
field.storage.[storage_type].settings
field_instance.[field_type].default
field_instance.[field_type].settings
field_instance.[widget_type].widget.settings

Consistency can be improved IMO, + we shouldn't tie naming to "the place in the field.field.* or field.instance.* YML files where they are included". Because they might very well be included in other, unrelated config files, be it in future developments in core, or 3rd party contrib ConfigEntities.
In short, naming should be about "what this is", not "where it's used", because the latter is something you can't really know.

I'd suggest :
field.settings.[field_type] --> field.[field_type].settings
field.storage.[storage_type].settings --> field_storage.[storage_type].settings
field_instance.[field_type].default --> field.[field_type].value
field_instance.[field_type].settings --> field.[field_type].instance_settings
field_instance.[widget_type].widget.settings --> field_widget.[widget_type].settings

Then, this means :
- each field type needs to provide 3 schema snippets:
field.[field_type].settings
field.[field_type].instance_settings
field.[field_type].value

- each storage type needs to provide
field_storage.[storage_type].settings

- each widget type needs to provide
field_widget.[widget_type].settings

That's a lot of config snippets to add, and the patch only provides a few of them right now.
We probably don't want to add all of them in one single patch ? How do you guys want to proceed ? One issue per module providing a [field type | widget | storage] ?

Gábor Hojtsy’s picture

IMHO if those are really a lot, let's pick a field type, say text, that is already used in tests to do in this patch at least. If the schemas will not be much more than is in the patch right now, then it looks like it can be done all at once. It is not at the complexity as views :)

Gábor Hojtsy’s picture

@yched: also I think %parent.something works, that is used in the patch, right? What @vijaycs85 pointed out is that %parent.%parent.something does not work for some reason, although as per the original patch promise, it should. Opening a new issue for that is a good idea IMHO. If we need that kind of construct, then maybe we'll need to postpone this on that.

vijaycs85’s picture

  1. doesn't work is a bit unfortunate. It means we have to trust each and every 'field_instance.*.value' snippet to duplicate the "type: sequence, label: 'Default value'" info correctly.
    Plus, technically, It's the container ($instance ConfigEntity) that states that $instance->default_value, $instance->settings... are arrays. It's not true that each field type can decide what happens in there. Would be better if the structure of schema reflected that.
    Is there an issue open for this limitation somewhere ?

    NOT FIXED: - All valid points, opened new issue #1972816: Add support for %parent.%parent in config schema dynamic type names.
  2. Inlcudes
    1. FIXED: field.settings.[field_type] --> field.[field_type].settings - happy to change it until we don't get a field with name just 'field'. In that case it will fall under field.field.* instead of field.field.settings :)
    2. FIXED: field.storage.[storage_type].settings --> field_storage.[storage_type].settings
    3. FIXED: field_instance.[field_type].default --> field.[field_type].value - wouldn't it better to have field.[field_type].instance_value?
    4. FIXED: field_instance.[field_type].settings --> field.[field_type].instance_settings
    5. FIXED: field_instance.[widget_type].widget.settings --> field_widget.[widget_type].settings
  3. That's a lot of config snippets to add, and the patch only provides a few of them right now.
    We probably don't want to add all of them in one single patch ? How do you guys want to proceed ? One issue per module providing a [field type | widget | storage] ?

    NOT FIXED: Can go as per #24 :)
yched’s picture

- "field_instance.[field_type].default --> field.[field_type].value - wouldn't it better to have field.[field_type].instance_value ?"
Well, '.value' is fine IMO, because what those snippets describe is the schema of the field values for this field type. 'instance_value' has no real meaning.

- field.[field_type].settings clashes if field_type == 'field':
Yeah, I think we're safe here.

- I'm fine with going with just text.module for now. All field types & widgets, that means :
field.text.settings
field.text_long.settings
field.text_with_summary.settings
field.text.instance_settings
field.text_long.instance_settings
field.text_with_summary.instance_settings
field.text.value
field.text_long.value
field.text_with_summary.value
field_widget.text_textfield.settings
field_widget.text_textarea.settings
field_widget.text_textarea_with_summary.settings

Also, can we take care of the test storage backends defined in field_test_field_storage_info() in this initial patch ? They will be easily forgotten if pushed to followups.
field_storage.field_test_storage.settings
field_storage.field_test_storage_failure.settings
(note : those actually don't have settings, so I'm not sure if this still means they *have* to provide schema snippets ?)

- core/modules/field_sql_storage/config/schema/field_sql_scheam.schema.yml
Typo : scheam :-) + shouldn't this be field_sql_storage.schema.yml ?

Gábor Hojtsy’s picture

As for what *has* to provide a schema, the schema is currently not used for *anything* in core. The patch in #1905152: Integrate config schema with locale, so shipped configuration is translated is on the verge of RTBC, which will use schema structure and identify the text and label types for translation. It will not do anything with the rest of the schema. I'm not aware of other uses of the schema in core. http://drupal.org/project/config_inspector is a developer module based on the schemas and http://drupal.org/project/config_translation is an end user module for config translation. So people would use that to translate field settings in-place. It needs a small patch to support field schemas if/when available, so not directly testable with the patch.

I'm not aware of others. It was proposed the schemas could be used to validate config or enforce types of data, but all those got strong pushback from different people, so not seem to be likely to apply. So the primary consumer is multilingual for now, but theoretically anything else can happen. I know Alex Pott is very excited for schemas for different reasons.

yched’s picture

@Gabor: thanks for the clarification.

I'm just not sure what happens when generating the schema for some given field.field.foobar.yml in the following situation :

- field.field.* schema specifies:

settings:
  type: field.[%parent.type].settings

(or preferably

settings:
    type: sequence
    label: 'Field settings'
    sequence:
      - type: field.[%parent.%parent.type].settings

if #1972816: Add support for %parent.%parent in config schema dynamic type names gets resolved)
- [%parent.type] / [%parent.%parent.type] in this specific case resolves to 'some_field_type'
- field.some_field_type.settings cannot be found because the field type defines no settings and thus just didn't bother.
(and thus the actual value found in field.field.foobar.yml will be 'settings: { }')

--> Notices / warnings ? Everything works and the resulting schema is consistent with the actual content of the file ?

Sure, not forcing every field type / widget / ... to provide an empty schema snippet for their settings if they have no settings would be DX++++ :-)

vijaycs85’s picture

Thanks @yched and @Gábor Hojtsy. Here is quick updates on some of your points:

#27.1. FIXED - core/modules/field_sql_storage/config/schema/field_sql_scheam.schema.yml
Typo : scheam :-) + shouldn't this be field_sql_storage.schema.yml ?
- good catch :)

#28.1 WAD(working as design) - --> Notices / warnings ? Everything works and the resulting schema is consistent with the actual content of the file ? - Nope, we don't get warning. Schema knows { } as empty array and won't throw any error. For example, field.field.body.yml the settings is an empty array:

settings: {  }

and our schema is just a sequence.

field.text_with_summary.settings:
  type: sequence
  label: 'Default'
  sequence:
    - type: string

Also, this would be valid for your point:
- field.some_field_type.settings cannot be found because the field type defines no settings and thus just didn't bother.
(and thus the actual value found in field.field.foobar.yml will be 'settings: { }')
. So we can ignore field.text_with_summary.settings itself. I'm not very sure, whether we won't get settings for text_with_summary at all on any situation. So keep it there to check more.

vijaycs85’s picture

Created separate issue for text module related field schema (as listed by @yched in #27) at #1973402: Provide config schema to field types, widgets and storage in text module and list of all field related config schema issues at http://drupal.org/project/issues/search/drupal?issue_tags=Field%20config...

vijaycs85’s picture

Issue summary: View changes

Added links to the sample files

vijaycs85’s picture

Issue summary: View changes

Updated issue summary - Adding sub-issue section

Gábor Hojtsy’s picture

field.some_field_type.settings cannot be found because the field type defines no settings and thus just didn't bother.
(and thus the actual value found in field.field.foobar.yml will be 'settings: { }')

A base type of field.*.settings can be defined with type: sequence I believe and if the module does not define one for the field type, then the system would just use the defaut settings (otherwise they are merged in on top of the module's schema, providing defaults for anything not provided.

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary.

vijaycs85’s picture

Moving image and taxonomy_term_reference schema changes from this issue to its own issues and adding text all schema changes from #1973402: Provide config schema to field types, widgets and storage in text module

vijaycs85’s picture

Issue summary: View changes

Updated issue summary.

vijaycs85’s picture

Issue summary: View changes

Updated issue summary.

Status: Needs review » Needs work
Issue tags: -Configuration system, -D8MI, -Field API, -sprint, -language-config, -Configuration schema

The last submitted patch, 1953404-config-schema-field-33.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

vijaycs85’s picture

Issue summary: View changes

Updated issue summary.

vijaycs85’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system, +D8MI, +Field API, +sprint, +language-config, +Configuration schema
Gábor Hojtsy’s picture

Tagging with Field configuration schema that vijaycs85 used to set up all the other module specific issues as well.

Gábor Hojtsy’s picture

So now are we considering this mostly postponed on #1972816: Add support for %parent.%parent in config schema dynamic type names? Anything else we can do to move this forward?

vijaycs85’s picture

Its a valid point that we are repeating 4 lines of code in every field.instance provider and we need to make sure it is bug free (effect of #1972816: Add support for %parent.%parent in config schema dynamic type names). IMHO this is a DX improment than missing functionality and we are going to hold around 7 defects ( as this is the master) if we hold this. Wouldn't be nice to have *a* follow up issue to clean all instsnce after #1972816: Add support for %parent.%parent in config schema dynamic type names made into core?

Gábor Hojtsy’s picture

Looking at the %parent.%parent question, that sounds like a feature request for the schema system (or task if we consider this to be postponed on that). See http://drupal.org/node/1972816#comment-7320578 - that feature was not supposed to work, neither tested nor introduced earlier. So given that wen can solve this without complicating the schema system more, looks like we can continue here, get this in and the numerous followups for field types and return to the %parent.%parent problem if there is more interest in it.

Gábor Hojtsy’s picture

As far as I understand @yched in #1972816: Add support for %parent.%parent in config schema dynamic type names he'd rather see this postponed on that than introducing repetition in the schema, please verify my understanding.

yched’s picture

My point is that #1972816: Add support for %parent.%parent in config schema dynamic type names is a (MOSCOW) S rather than a W :-).

As to how we deal with it here, sure, being able to write those right the 1st time around seems better than having to go and fix them all again after, but not hell-bent on it, I can go with what you guys think is best.

Gábor Hojtsy’s picture

I think let's review this and get this in to unblock the army of other issues and contributors on them.

yched’s picture

OK :

+++ b/core/modules/field/config/schema/field.schema.ymlundefined
@@ -7,3 +7,130 @@ field.settings:
+    entity_types:
+      type: sequence
+      label: 'Entity types'
+      sequence:

Maybe rather "Allowed entity types" ?

+++ b/core/modules/field/config/schema/field.schema.ymlundefined
@@ -7,3 +7,130 @@ field.settings:
+    indexes:
+      type: sequence
+      label: 'Indexes'
+      sequence:
+        - type: sequence
+          label: 'Index'
+          sequence:
+            - type: string
+              label: 'Field'

Minor : should be "Indexes" and "Fields" (or maybe "Columns", since field is a bit clashing/recursive)

+++ b/core/modules/field/config/schema/field.schema.ymlundefined
@@ -7,3 +7,130 @@ field.settings:
+field.instance.*.*.*:
+  type: mapping
+  label: 'Field instance settings'

"Field instance settings" would be more specifically the 'settings' entry.
Maybe just "Field instance", or "Field instance definition" (but then "Field definition" for field.field.*/label) ?

+++ b/core/modules/field/config/schema/field.schema.ymlundefined
@@ -7,3 +7,130 @@ field.settings:
+    id:
+      type: string
+      label: 'Machine name'

No biggie, but not sure about that one.
"machine name" works for fields, if that's what's been used for other config entities so far.
But instances don't have a machine name of their own, the id here is "just" an id, because every config entity needs an id.
Is that ok if we use "Machine name" for fields and just "ID" for instances ?

+++ b/core/modules/field/config/schema/field.schema.ymlundefined
@@ -7,3 +7,130 @@ field.settings:
+    widget:
+      type: mapping
+      label: 'Widget settings'
+      mapping:

Same as above "Widget settings" would be more specifically the 'settings' sub-entry.
"Widget definition" ? Or just "Widget" ?

+++ b/core/modules/field_sql_storage/config/schema/field_sql_storage.schema.ymlundefined
@@ -0,0 +1,7 @@
+field_storage.field_sql_storage.settings:
+  type: sequence
+  label: 'Settings'
+  sequence:
+    - type: string

field_sql_storage actually has no settings.
If I get the discussions above right, this means field_sql_storage.schema.yml is not needed at all ?
At any rate, - type: string sounds like it has no meaning.

Same for field.text_with_summary.settings, field.text_long.settings

Or is it another consequence of the %parent.%parent thing, we *need* a dummy field_storage.field_sql_storage.settings snippet because the
+ type: sequence
+ label: 'Settings'
info cannot be held by field.field.* ?

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/FieldUpgradePathTest.phpundefined
index 4fbe45a..f748175 100644

Hard to review, can we reorder the file a bit ?
- Field types, in the order of text_field_info():
field.text.settings
field.text.instance_settings
field.text.value
field.text_long.settings
field.text_long.instance_settings
...
- Widgets
field_widget.text_textfield.settings
field_widget.text_textarea.settings
field_widget.text_textarea_with_summary.settings

+++ b/core/modules/text/config/schema/text.schema.ymlundefined
@@ -7,3 +7,135 @@ text.settings:
+field.text_with_summary.instance_settings:
+  type: mapping
+  label: 'Text area with a summary'
+  mapping:
+    text_processing:
+      type: string
+      label: 'Text processing'

'text_processing' type should be boolean (applies to the 3 field types)

+++ b/core/modules/text/config/schema/text.schema.ymlundefined
@@ -7,3 +7,135 @@ text.settings:
+field.text_with_summary.value:
+  type: sequence
+  label: 'Default value'
+  sequence:
+    - type: mapping
+      label: 'Default'
+      mapping:
+        summary:
+          type: string
+          label: 'Summary'
+        value:
+          type: text
+          label: 'Body'
+        format:
+          type: string

'value' should be first.

+++ b/core/modules/text/config/schema/text.schema.ymlundefined
@@ -7,3 +7,135 @@ text.settings:
+field_widget.text_textarea_with_summary.settings:
+  type: mapping
+  label: 'Text area widget settings'
+  mapping:
+    rows:
+      type: integer
+      label: 'Rows'
+    placeholder:
+      type: label
+      label: 'Placeholder'
+    summary_rows:
+      type: integer
+      label: 'Summary rows'

Nitpick : order - rows, summary_rows, placeholder ?

+++ b/core/modules/text/config/schema/text.schema.ymlundefined
@@ -7,3 +7,135 @@ text.settings:
+field.text.value:
+  type: sequence
+  label: 'Default value'
+  sequence:
+    - type: mapping
+      label: 'Default'
+      mapping:
+        value:
+          type: label
+          label: 'Value'

Misses a
+ format:
+ type: string
+ label: 'Text format'
Same for field.text_long.value

vijaycs85’s picture

Thanks for the review @yched and @Gábor Hojtsy. Here is some updates on #43:

#43.1 FIXED: Maybe rather "Allowed entity types" ?
#43.2 FIXED: Minor : should be "Indexes" and "Fields" (or maybe "Columns", since field is a bit clashing/recursive) - made it as column as it represent just one column.
#43.3 FIXED: "Field instance settings" would be more specifically the 'settings' entry. - made it as "Field instance".
#43.4 WAD: Is that ok if we use "Machine name" for the one and just "ID" for the other ? - IMHO, We have to replace "ID" as Machine name wherever applicable. the basic idea of these labels are to have what we see in UI.
#43.5 FIXED: "Widget definition" ? Or just "Widget" ? - Made as "Widget".
#43.6 NOT FIXED: field_sql_storage actually has no settings. - well, we are not very sure what settings we get here. if we are sure, it is going to be array of indexed array for all type of storage then yes, we face %parent.%parent issue here as well.
#43.7 FIXED: Can we reorder the file a bit ? - Fixed. Now it is:
field.text.settings
field.text.instance_settings
field.text.value

field.text_long.settings
field.text_long.instance_settings
field.text_long.value

field.text_with_summary.settings
field.text_with_summary.instance_settings
field.text_with_summary.value

field_widget.text_textfield.settings
field_widget.text_textarea.settings
field_widget.text_textarea_with_summary.settings

Is it OK or need to move widget with type?
#43.8 NOT FIXED: Same here, 'text_with_summary' has no 'field settings'
#43.9 FIXED: 'text_processing' type should be boolean (applies to the 3 field types)
#43.10 NOT FIXED: 'value' should be first. - This is the order we are getting in config and follow same in schema.

default_value:
  -
    summary: ''
    value: "<p>testing your details...</p>\r\n"
    format: basic_html

#43.11 NOT FIXED: Nitpick : order - rows, summary_rows, placeholder ? - Same as 10

widget:
  weight: '-4'
  type: text_textarea_with_summary
  settings:
    rows: '9'
    placeholder: 'place holder details'
    summary_rows: '3'
  module: text

#43.12 FIXED:
Misses a
+ format:
+ type: string
+ label: 'Text format'
Same for field.text_long.value

Good catch, added.

yched’s picture

Thanks @vijaycs85 !

#43.4 WAD: Is that ok if we use "Machine name" for the one and just "ID" for the other ? - IMHO, We have to replace "ID" as Machine name wherever applicable. the basic idea of these labels is to have what we see in UI.

Well, specifically, there is no UI for field.instance 'id' :-)
It's not a machine name :-), it's a string derived from other properties of the field and instance : [entity_type].[bundle].[field_name]. So 'name' really is misleading here.

#43.7 FIXED - Is it OK or need to move widget with type?

Nope, fine as is :-)

#43.10 NOT FIXED: 'value' should be first. - This is the order we are getting in config and follow same in schema.

There's no guaranteed order for the entries in there, we'll find them in any order, depending on the code that did the config save(). So for the schema let's pick the order that is the most natural, and that's the one provided by text_field_schema() : value, summary_value (only for text_with_summary), format.

Same for #43.11, the order is not guaranteed in actual config files, so the order used in Drupal\text\Plugin\field\widget\TextareaWithSummaryWidget's plugin annotations should be favored in the schema.

vijaycs85’s picture

Once again, thank you @yched for spending your time on this review. Here is some updates on #45:

#45.1 FIXED: Well, specifically, there is no UI for field.instance 'id' :-)
It's not a machine name :-), it's a string derived from other properties of the field and instance : [entity_type].[bundle].[field_name]. So 'name' really is misleading here. - Sorry about my misunderstanding there. Updated as "ID" now.
#45.2 FIXED: There's no guaranteed order for the entries in there, we'll find them in any order, depending on the code that did the config save(). So for the schema let's pick the order that is the most natural, and that's the one provided by text_field_schema() : value, summary_value (only for text_with_summary), format. - OK, we can make it, if we don't have a fixed one.
#45.3 FIXED: Same for #43.11, the order is not guaranteed in actual config files, so the order used in Drupal\text\Plugin\field\widget\TextareaWithSummaryWidget's plugin annotations should be favored in the schema. - Updated as in TextareaWithSummaryWidget.

swentel’s picture

I really need to get up to speed with config schema, I have almost no clue what's going on here, but I *do* like the progress and enthousiasm on this issue, so, really, thank you @vijaycs85!

yched’s picture

Label for id': it's the other way around ;-) - should be "Machine name" for field.field.*.yml, and "ID" for field.instance.*.*.*.yml.

yched’s picture

RTBC when #48 is fixed.

Now, opened #1975112: Config schema mechanisms cannot reflect alterable config trees for the elephant in the room :-/:

core/modules/text/config/schema/text.schema.yml
+field.text.instance_settings:
(...)
+    user_register_form:
+      type: boolean
+      label: 'Display on user registration form.'
vijaycs85’s picture

Thanks again @yched. Fixed #48 here...

On #47: You are welcome and thanks to @swentel and @yched for the amazing effort on fields and quick reviews :)

yched’s picture

Status: Needs review » Reviewed & tested by the community

Yay :-). Thanks @vijaycs85 !

yched’s picture

Rerolled + easier access to the field from the instance after #1967106: FieldInstance::getField() method.

yched’s picture

Oops, the new field_sql_storage.schema.yml was lost in last reroll.

Updated patch, also removes a series of empty lines in text.schema.yml

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

Thanks, superb! This lets us work on the army of followups for the rest of the fields at http://drupal.org/project/issues/search/drupal?issue_tags=Field%20config...

@vijaycs85: thanks!!

Gábor Hojtsy’s picture

Also any field modules missing from that list? :)

amateescu’s picture

Yep, entity reference..

swentel’s picture

Looks like we also miss telephone, email and link ?

vijaycs85’s picture

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.