Mark config schema types (field.field_settings.* and field.storage_settings.*)) for field types that have no settings as fully validatable

No breaks! I think this is ready for a review.

Just one nit. 🤓

I'm not sure I agree with wim's remark, it would lead to this same documentation being on all of those instances. I actually think this is rtbc.

I'm not sure how to set the status for this issue now.

Ok, looks great :)

RTBC +1. I like this!

However, I see a few that have been overlooked:

• field.storage_settings.daterange
• field.field_settings.daterange
• field.storage_settings.text_long
• field.formatter.settings.text_default

So … un-RTBC'd 😅

(Found these using the regex field.field_settings.[a-z]+\:\n type: in PHPStorm.)

But playing devil's advocate, we could delete all of these config schema types (field.field_settings.string: and instead do

field.storage_settings.*:
  type: mapping
  label: 'Settings'
  # By default, a field type has no settings, so this can safely be considered
  # fully validatable. A `FOO` field type that has one or more settings is
  # required to create a `field.storage_settings.FOO` config schema type, and
  # cannot possibly forget to do that, because validation errors would be
  # triggered if it ever tried to save a setting, since this mapping allows no
  # key-value pairs at all.
  constraints:
    FullyValidatable: ~

field.field_settings.*:
  type: mapping
  label: 'Settings'
  # By default, a field type has no settings, so this can safely be considered
  # fully validatable. A `FOO` field type that has one or more settings is
  # required to create a `field.field_settings.FOO` config schema type, and
  # cannot possibly forget to do that, because validation errors would be
  # triggered if it ever tried to save a setting, since this mapping allows no
  # key-value pairs at all.
  constraints:
    FullyValidatable: ~

and then simply delete all of the ones that define no settings at all. AFAICT that should work too? 🤔 And would reduce the total number of types, keep things simpler? Created alternative MR to try that, curious about the test results 😄

field.storage_settings.daterange
field.field_settings.daterange
field.storage_settings.text_long
field.formatter.settings.text_default

Did the first three. The last one is out of scope; this issue is only handling field.field_settings.* and field.storage_settings.*.

Looks like I made some mistakes in my alternative MR 😅

But what do you think about the concept?

I do kind of like the concept. I dig the idea that someone defining a new field type without additional settings can be guaranteed, with no additional effort, that they are conformant to config schema (which, as I've said elsewhere, is quite an arcane topic).

As long as we could reliably inform developers that, the moment they add settings that are stored in config, they need to define a schema type with constraints for it...

I feel like your concept, while a good idea, is best suited to a follow-up. Unless it's a relatively easy lift to get it working in your MR.

Unless it's a relatively easy lift to get it working in your MR.

I'm 90% confident that I just fixed my silly mistakes.

Green! 😄

• Original MR: 6 files, +72, -0
• Alternative MR: 6 files, +46, -71

phenaproxima → closed merge request !6617

I like your way better! It's simpler and more future proof. Closed mine out. We'll need an issue summary update here...

Issue summary updated.

Do we need a change record here? I'm not sure if config schema is really considered API, as such.

I'll assume we don't and just go ahead and RTBC this. I have no complaints about the MR.

I think we've had change records for a lot of other mark x as fully validatable issues recently, but I'm not sure if we actually need that.

We've only had CRs for:

• new config schema types, because these are new APIs (this doesn't do that)
• new/modified validation constraints, because these are new/expanded APIs (this doesn't do that either)
• config entity types that become fully validatable AND have config schema changes in the process (nope, this isn't happening here either)

So in this case, there's literally ZERO API impact, so there's also no need for this. 👍

field.storage_settings.password:
  type: field.storage_settings.string
  label: 'Password settings'

This has no settings so could be removed too?

I'm guessing so. Assigning to Wim for that, then I'll re-RTBC.

This has no settings so could be removed too?

No, because of:

  type: field.storage_settings.string

which looks like this:

field.storage_settings.string:
  type: mapping
  label: 'String settings'
  mapping:
    max_length:
      type: integer
      label: 'Maximum length'
    case_sensitive:
      type: boolean
      label: 'Case sensitive'
    is_ascii:
      type: boolean
      label: 'Contains US ASCII characters only'

I actually made the very same mistake 😄 And that resulted in test failures:

Drupal\KernelTests\Core\Field\FieldType\PasswordItemTest::testPreSavePreHashed
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for field.storage.entity_test.test_field with the following errors: field.storage.entity_test.test_field:settings.max_length missing schema, field.storage.entity_test.test_field:settings.is_ascii missing schema, field.storage.entity_test.test_field:settings.case_sensitive missing schema, 0 [settings] 'max_length' is an unknown key because type is password (see config schema type field.storage_settings.*)., 1 [settings] 'is_ascii' is an unknown key because type is password (see config schema type field.storage_settings.*)., 2 [settings] 'case_sensitive' is an unknown key because type is password (see config schema type field.storage_settings.*).

Whic is why I had to revert my overly eager simplifications 😅

OK, that makes sense! I didn't find any other issues when reviewing.

Committed and pushed 7e2b0d453c to 11.x and 8dd90d73a0 to 10.3.x. Thanks!

longwave → closed merge request !6978

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