Convert field_storage_config and field_config's form validation logic to validation constraints

Working on this 🤓🥳

Analysis completed. Pushed as a commit that says what constraints we should implement.

… and it shockingly passed tests! Clearly Drupal does not verify that validation constraints defined in the config schema actually exist 😱

Created #3324526: Validation constraints in config schema that don't exist should trigger error for that.

If I try to run the test introduced by #3324984: Create test that reports % of config entity types (and config schema types) that is validatable while this merge request is applied, I get an error message. Turns out that there's a wrong config schema change here 😅

But … that really just points out how weak the DX is around config schemas 🫣

Pushed fix: e21361724e.

How much progress does this MR help us make? Let's use #3324984: Create test that reports % of config entity types (and config schema types) that is validatable to answer that question!

Before

Config entity types
  ℹ️   0.00% validatable (0 of 30 config entity types)
  ℹ️  24.05% average config entity type validatability
…
  🔴   0% core.base_field_override.*.*.*
  🔴   0% field.field.*.*.*
  🟡  44% field.storage.*.*
…
  ℹ️ 35.40% validatable property paths (194 of 548 property paths — this excludes property paths for base types)
…
Config types
  ℹ️   4.62% validatable (29 of 628 config types — excludes base types)
  ℹ️  25.94% average config type validatability
…
Property paths
  ℹ️ 36.68% validatable property paths (1369 of 3732 property paths — this excludes property paths for base types)
…

After

Config entity types
  ℹ️   6.67% validatable (2 of 30 config entity types)
  ℹ️  31.04% average config entity type validatability
…
  🟢 100% core.base_field_override.*.*.*
  🟢 100% field.field.*.*.*
  🟡  54% field.storage.*.*
…
  ℹ️ 37.04% validatable property paths (203 of 548 property paths — this excludes property paths for base types)
…
Config types
  ℹ️   4.94% validatable (31 of 628 config types — excludes base types)
  ℹ️  26.28% average config type validatability
…
Property paths
  ℹ️ 37.14% validatable property paths (1386 of 3732 property paths — this excludes property paths for base types)
…

Conclusion: the test coverage is able to detect and identify that there is indeed significant progress, but also that the validation constraints on field.storage.*.* are still inadequate. It even shows exactly what's missing:

  ✅ field.storage.*.*
      ✅ cardinality
      ✅ custom_storage
      ✅ entity_type
      ✅ field_name
      ✅ id
      ✅ indexes
        ✅ [%unknown_key%]
          ❌ [%unknown_key%]
      ✅ locked
      ✅ module
      ✅ persist_with_no_fields
      ✅ settings
        ❌ [%parent.type]=changed
        ✅ [%parent.type]=comment
          ❌ comment_type
        ❌ [%parent.type]=created
        ❌ [%parent.type]=daterange
        ✅ [%parent.type]=datetime
          ❌ datetime_type
        ✅ [%parent.type]=decimal
          ❌ precision
          ❌ scale
        ❌ [%parent.type]=email
        ✅ [%parent.type]=entity_reference
          ❌ target_type
        ❌ [%parent.type]=file
          ✅ display_default
          ✅ display_field
          ❌ uri_scheme
        ❌ [%parent.type]=float
        ❌ [%parent.type]=image
          ❌ default_image
        ✅ [%parent.type]=integer
          ❌ size
          ✅ unsigned
        ❌ [%parent.type]=link
        ✅ [%parent.type]=list_float
          ✅ allowed_values
            ✅ [%unknown_key%]
              ❌ label
              ❌ value
          ❌ allowed_values_function
        ✅ [%parent.type]=list_integer
          ✅ allowed_values
            ✅ [%unknown_key%]
              ❌ label
              ❌ value
          ❌ allowed_values_function
        ✅ [%parent.type]=list_string
          ✅ allowed_values
            ✅ [%unknown_key%]
              ❌ label
              ❌ value
          ❌ allowed_values_function
        ❌ [%parent.type]=password
        ✅ [%parent.type]=string
          ✅ case_sensitive
          ✅ is_ascii
          ❌ max_length
        ✅ [%parent.type]=string_long
          ✅ case_sensitive
        ✅ [%parent.type]=text
          ❌ max_length
        ❌ [%parent.type]=text_long
        ❌ [%parent.type]=text_with_summary
        ❌ [%parent.type]=uri
          ✅ case_sensitive
          ❌ max_length
      ✅ translatable
      ✅ type

(there are two missing pieces: indexes and the many possible per-field type settings — which indeed had a @todo in the current merge request, because I was unsure how to address it — now it's starting to become clear how we can address it! 👍)

and:

✅ field_config_base
      ✅ bundle
      ✅ default_value
        ✅ [%unknown_key%]
          ✅ [%parent.%parent.field_type]=boolean
            ❌ value
          ✅ [%parent.%parent.field_type]=changed
            ❌ value
          ✅ [%parent.%parent.field_type]=comment
            ❌ cid
            ❌ comment_count
            ❌ last_comment_name
            ❌ last_comment_timestamp
            ❌ last_comment_uid
            ❌ status
          ✅ [%parent.%parent.field_type]=created
            ❌ value
          ✅ [%parent.%parent.field_type]=daterange
            ❌ default_date
            ❌ default_date_type
            ❌ default_end_date
            ❌ default_end_date_type
          ✅ [%parent.%parent.field_type]=datetime
            ❌ default_date
            ❌ default_date_type
          ✅ [%parent.%parent.field_type]=decimal
            ❌ value
          ✅ [%parent.%parent.field_type]=email
            ❌ value
          ✅ [%parent.%parent.field_type]=entity_reference
            ❌ target_id
            ✅ target_uuid
          ❌ [%parent.%parent.field_type]=file
          ✅ [%parent.%parent.field_type]=float
            ❌ value
          ❌ [%parent.%parent.field_type]=image
          ✅ [%parent.%parent.field_type]=integer
            ❌ value
          ✅ [%parent.%parent.field_type]=link
            ✅ attributes
              ❌ title
            ✅ options
              ✅ absolute
              ✅ attributes
                ❌ [%unknown_key%]
              ❌ fragment
              ✅ https
              ✅ query
                ❌ [%unknown_key%]
            ❌ title
            ❌ uri
          ✅ [%parent.%parent.field_type]=list_float
            ❌ value
          ✅ [%parent.%parent.field_type]=list_integer
            ❌ value
          ✅ [%parent.%parent.field_type]=list_string
            ❌ value
          ✅ [%parent.%parent.field_type]=string
            ❌ value
          ✅ [%parent.%parent.field_type]=string_long
            ❌ value
          ✅ [%parent.%parent.field_type]=telephone
            ❌ value
          ✅ [%parent.%parent.field_type]=text
            ❌ format
            ❌ value
          ✅ [%parent.%parent.field_type]=text_long
            ❌ format
            ❌ value
          ✅ [%parent.%parent.field_type]=text_with_summary
            ❌ format
            ❌ summary
            ❌ value
          ✅ [%parent.%parent.field_type]=timestamp
            ❌ value
          ✅ [%parent.%parent.field_type]=uri
            ❌ value
      ❌ default_value_callback
      ✅ description
      ✅ entity_type
      ✅ field_name
      ✅ field_type
      ✅ id
      ✅ label
      ✅ required
      ✅ settings
        ✅ [%parent.field_type]=boolean
          ❌ off_label
          ❌ on_label
        ❌ [%parent.field_type]=changed
        ✅ [%parent.field_type]=comment
          ❌ anonymous
          ❌ default_mode
          ✅ form_location
          ❌ per_page
          ❌ preview
        ❌ [%parent.field_type]=created
        ❌ [%parent.field_type]=daterange
        ❌ [%parent.field_type]=datetime
        ✅ [%parent.field_type]=decimal
          ❌ max
          ❌ min
          ❌ prefix
          ❌ suffix
        ❌ [%parent.field_type]=email
        ✅ [%parent.field_type]=entity_reference
          ❌ handler
          ✅ handler_settings
            ❌ [%parent.handler]=default
              ✅ auto_create
              ❌ auto_create_bundle
              ✅ sort
                ❌ direction
                ❌ field
              ✅ target_bundles
                ❌ [%unknown_key%]
            ❌ [%parent.handler]=default:user
              ✅ filter
                ✅ role
                  ❌ [%unknown_key%]
                ❌ type
              ✅ include_anonymous
            ❌ [%parent.handler]=views
              ✅ view
                ✅ arguments
                  ❌ [%unknown_key%]
                ❌ display_name
                ❌ view_name
        ❌ [%parent.field_type]=file
          ✅ description_field
        ✅ [%parent.field_type]=float
          ❌ max
          ❌ min
          ❌ prefix
          ❌ suffix
        ❌ [%parent.field_type]=image
          ✅ alt_field
          ✅ alt_field_required
          ❌ default_image
          ❌ max_resolution
          ❌ min_resolution
          ✅ title_field
          ✅ title_field_required
        ✅ [%parent.field_type]=integer
          ❌ max
          ❌ min
          ❌ prefix
          ❌ suffix
        ✅ [%parent.field_type]=link
          ❌ link_type
          ❌ title
        ❌ [%parent.field_type]=list_float
        ❌ [%parent.field_type]=list_integer
        ❌ [%parent.field_type]=list_string
        ❌ [%parent.field_type]=password
        ❌ [%parent.field_type]=string
        ❌ [%parent.field_type]=string_long
        ✅ [%parent.field_type]=text
          ✅ allowed_formats
            ❌ [%unknown_key%]
        ✅ [%parent.field_type]=text_long
          ✅ allowed_formats
            ❌ [%unknown_key%]
        ✅ [%parent.field_type]=text_with_summary
          ✅ allowed_formats
            ❌ [%unknown_key%]
          ✅ display_summary
          ✅ required_summary
        ❌ [%parent.field_type]=uri
      ✅ translatable

That shows that in order for this issue to achieve actual validatability of FieldStorageConfig and FieldConfig entity types, we have a lot more work to do: we need to add validation constraints to all of the per-field type configurability too! 😳😳😳😳

But … of course … that actually makes sense 😅 That means we'll have to somehow split this issue up.

#2920682: Add config validation for plugin IDs landed, this MR needed to be updated. Just did that 👍 This MR is now a bit smaller again 🥳 Also addressed all of @larowlan's early observations 🤓

I believe the next piece we can reasonably extract (because it too would be useful for config other than Field(Storage)Config) would be:

1. ImmutableFields (at least one other example: Editor config entity's format)
2. MachineName (all over the place — at least one other example: FilterFormat config entity's format property) → already has an issue: #2920678: Add config validation for the allowed characters of machine names 🥳

I'm hoping @phenaproxima wants to take on MachineName next 🤓

The MR is failing for nonsensical reasons:

--- Errors ---
You are using the deprecated option "--no-suggest". It has no effect and will break in Composer 3.
> Drupal\Composer\Composer::ensureComposerVersion
Installing dependencies from lock file (including require-dev)
Verifying lock file contents can be installed on current platform.
- Required package "drupal/core" is in the lock file as "10.1.x-dev" but that does not satisfy your constraint "self.version".
- Required package "drupal/core-project-message" is in the lock file as "10.1.x-dev" but that does not satisfy your constraint "self.version".
- Required package "drupal/core-vendor-hardening" is in the lock file as "10.1.x-dev" but that does not satisfy your constraint "self.version".
This usually happens when composer files are incorrectly merged or the composer.json file is manually edited.
Read more about correctly resolving merge conflicts https://getcomposer.org/doc/articles/resolving-merge-conflicts.md
and prefer using the "require" command over editing the composer.json file directly https://getcomposer.org/doc/03-cli.md#require-r

… composer.json has not been touched at all. 😳

The latest commit I pushed to address @larowlan's UniqueField remark results in this locally:

Testing /Users/wim.leers/core/core/modules/field/tests/src/Kernel/Entity
.E....E..........                                                 17 / 17 (100%)

Time: 00:11.594, Memory: 10.00 MB

There were 2 errors:

1) Drupal\Tests\field\Kernel\Entity\FieldStorageConfigValidationTest::testImmutableFields with data set "entity_type" (array('entity_test_mul'))
Exception: The existing field_storage_config entity (ID: entity_test_mul.test) is being validated and it violates a uniqueness constraint: entity_test.test have the same value for the "field_name" field: "test".

…

2) Drupal\Tests\field\Kernel\Entity\FieldStorageConfigValidationTest::testEntityType
Exception: The existing field_storage_config entity (ID: strange_entity.test) is being validated and it violates a uniqueness constraint: entity_test.test have the same value for the "field_name" field: "test".

…

⇒ UniqueField is the wrong constraint here, so despite https://git.drupalcode.org/project/drupal/-/merge_requests/3047/diffs?co... technically being a bugfix, we can't actually use that constraint here. That's a mistake I made.

We need a new constraint, that uses \Drupal\Core\Entity\Plugin\Validation\Constraint\CompositeConstraintBase. Something like UniqueFieldCombination, which allows expressing "whatever values these fields contain, they need to be a unique combination across all entities of this type".

For FieldStorageConfig that'd be

UniqueFieldCombination:
  fields: [entity_type, field_name]

for FieldConfig that'd be

UniqueFieldCombination:
  fields: [entity_type, bundle, field_name]

(One could argue that these are enforced implicitly thanks to those exact unique combinations being used to construct the entity ID … but they are not the "entity keys" — the id is! Besides, that'd mean an error not at validation time, but post-validation: at the time when it being written to the database… that's not a good UX.)

Thoughts, @larowlan?

Needs to be rebased now that #2920678: Add config validation for the allowed characters of machine names is in.

#13: yes, it is. And everything else that still needs validation in those 2 config entities. Per #8:

  🟢 100% field.field.*.*.*
  🟡  54% field.storage.*.*

So it's especially the latter that still needs more attention.

It seems that many of the 222 failures are originating from the previously discovered bug in NotNullConstraintValidator — see #3364109-5: Configuration schema & required values: add test coverage for `nullable: true` validation support.

Borrowing the fix from that issue…

Implemented the UniqueFieldCombination constraint as discussed between @larowlan and I in #10 + #11.

Tests still needed. Clean-up likely still needed too. First trying to get this MR green and fully validating both config entity types…

And one more push on FieldConfig, which means FieldConfigValidationTest should be passing tests now too! 🤞

Next up:

• Implement BundleExists
• Implement Matches

Here's a new EntityBundleExists constraint, with explicit test coverage for both FieldConfigValidationTest and BaseFieldOverrideValidationTest.

Hopefully @phenaproxima will beat me to it, but the last remaining constraint we're missing is the Matches constraint. Commenting it out for now to observe how many failures remain.

Beyond Matches, we still need detailed test coverage for each of the individual constraints. But the end is in sight! 🥳

A lot of seemingly legitimate

Drupal\Core\Field\FieldException: Attempted to create an instance of field with name field_ibcu9gem on entity type node when the field storage does not exist.

test failures … 🤔 🕵️

Besides #20, the commits since #19 made BaseFieldOverrideValidationTest pass tests but not FieldConfigValidationTest. These 2 commits should make both pass.

Great! Now the prime remaining problem is indeed #20.

\Drupal\KernelTests\Core\Field\FieldSettingsTest::testConfigurableFieldSettings() is pretty weird 😳. It does:

    $field_storage = FieldStorageConfig::create([
…
    ]);
    $field = FieldConfig::create([
…
    ]);
    $field->save();

Note that only FieldConfig is saved, not FieldStorageConfig!

@lauriii just surfaced #3165822: [PP-1] Creating comment field w/o a comment type is allowed, which further confirms the need for this 🤓

I won't have time to finish my investigation into #2327883: Field [storage] config have incomplete settings until they are saved to get this to green prior to being AFK for the next 5 workdays. I'll be back Monday the 24th of July, so unassigning.

It's possible we'll first need to solve #2327883: Field [storage] config have incomplete settings until they are saved, to allow removing the one line that @phenaproxima just questioned in his review last night. That one line is currently required to not violate the RequiredConfigDependencies constraint. Uncomment that one line and you should see failures triggered by that validation constraint in FieldConfigValidationTest.

Will do a deep review of where we are right now, but HUGE thanks to @phenaproxima for getting this to green! 🥳

#2327883: Field [storage] config have incomplete settings until they are saved is now very close, so we hopefully will soon be unblocked here!

While waiting for #2327883: Field [storage] config have incomplete settings until they are saved to finally land, let's move forward with this one.

#26: that looks like a fine work-around — the best we can do for now! 👍 Thanks for figuring that out! 🧙😄
There's still plenty of @todos left. 😇
But there's also plenty of things that already exist in this MR that would also help other config become validatable. I identified two, and created new issues for them: #3382580: Add new `ImmutableProperties` constraint + #3382581: Add new `EntityBundleExists` constraint.

This is now postponed on:

1. #2327883: Field [storage] config have incomplete settings until they are saved
2. #3382580: Add new `ImmutableProperties` constraint
3. #3382581: Add new `EntityBundleExists` constraint

#2327883: Field [storage] config have incomplete settings until they are saved landed 👍

#3382580: Add new `ImmutableProperties` constraint landed.

#3382581: Add new `EntityBundleExists` constraint landed. Time to catch this MR up on ~5 months of upstream changes 😅

From 20 files, +851, -4 to 11 files, +570, -17 — thanks to

1. #2327883: Field [storage] config have incomplete settings until they are saved
2. #3382580: Add new `ImmutableProperties` constraint
3. #3382581: Add new `EntityBundleExists` constraint
4. #3364109: Configuration schema & required values: add test coverage for `nullable: true` validation support

Did a big push forward! One additional validation constraint, simplified another constraint that was added to this MR many months ago, removed ALL NotNull constraints thanks to FullyValidatable and triaged the remaining @todos.

#3421626: Mark config schema types (field.field_settings.* and field.storage_settings.*)) for field types that have no settings as fully validatable landed! Time for an update here 😄

So glad to see this moving forward again! 🤓

Also, I had to do this yesterday:

  protected static $configSchemaCheckerExclusions = [
    // @todo Core bug: this is missing config schema: ``type: field.storage_settings.uri` does not exist! This is being fixed in https://www.drupal.org/project/drupal/issues/3324140.
    'field.storage.entity_test.test_required__file_uri',
    'field.storage.entity_test.test_optional__file_uri',
    // @todo Core bug: this is missing config schema: ``type: field.storage_settings.uuid` does not exist! This is being fixed in https://www.drupal.org/project/drupal/issues/3324140.
    'field.storage.entity_test.test_required__uuid',
    'field.storage.entity_test.test_optional__uuid',
  ];

That's for XB over at #3512433: Provide visibility into which (core) field types (74%), field type props (63%) can be mapped into Content Type Templates vs not, and which field widgets (36%) are supported, fixing this would allow me to remove those exclusions!