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

Is this currently blocked on writing and implementing the proposed UniqueFieldCombination?

Needs to be rebased now that 📌 Add config validation for the allowed characters of machine names Fixed 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 a majority of failures is 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 🐛 [PP-1] Creating comment field w/o a comment type is allowed Postponed , which further confirms the need for this 🤓

I won't have time to finish my investigation into 🐛 Field [storage] config have incomplete settings until they are saved Fixed 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 🐛 Field [storage] config have incomplete settings until they are saved Fixed , 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.

After wrangling this for a whole day, I figured...what the hell is the point of adding that addDependency() call, which breaks a bunch of tests in weird ways due to the inherent strangeness of field config architecture, when we could just work around it in the relevant test, and explain our reasoning with a comment?

Moving to NW for #26 comment.

Also are additional tests needed?

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

While waiting for 🐛 Field [storage] config have incomplete settings until they are saved Fixed 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: 📌 Add new `ImmutableProperties` constraint Fixed + 📌 Add new `EntityBundleExists` constraint Fixed .

This is now postponed on:

1. 🐛 Field [storage] config have incomplete settings until they are saved Fixed
2. 📌 Add new `ImmutableProperties` constraint Fixed
3. 📌 Add new `EntityBundleExists` constraint Fixed

🐛 Field [storage] config have incomplete settings until they are saved Fixed landed 👍

📌 Add new `ImmutableProperties` constraint Fixed landed.

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

From 20 files, +851, -4 to 11 files, +567, -21 — thanks to

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

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.

Just to note it in writing: I'm not marking the following field setting as fully validatable right now, because their supported default values (for example, IntegerItem's default min setting is '' (empty string), which is nonsense from a configuration standpoint) conflict with the logic in \Drupal\Core\Config\StorableConfigBase::castValue(), which specifically treats empty strings as NULL for integer and floats.

I'm not exactly sure what the correct fix for this is, but it's definitely not something to fix in this issue without blowing up the scope.

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

Scary rebase, so made a new branch first to see how it handles itself.

I don't see the tests being run on drupal ci, so setting this to needs review to see if that triggers them to start.

bbrala → closed merge request !11266

Seems good enough, moving to main MR, shoudln't need 'Needs review' :)

Todo: re-addthe creation of the entity types removed in one of last 2 commits

Well at least tests are close to OK again so this can be worked on again.

Answered some of the comments.

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 📌 Provide visibility into which (core) field type props can be mapped into Content Type Templates vs not Active , fixing this would allow me to remove those exclusions!

Although there is some missing test:

1. The constraint MatchesOtherConfigValue needs tests
2. The constraint NoEntitiesExistYetWithHigherCardinality needs tests

Our tests are green :x

I've split two issues for the missing tests.

1. ✨ Implement MatchesOtherConfigValue tests Active
2. ✨ Implement NoEntitiesExistYetWithHigherCardinality tests Active

While working on ✨ Implement MatchesOtherConfigValue tests Active i kinda concluded it seems kinda unneeded since there are loads of places where this validator is triggered in the tests showing it working.

Examples:
https://git.drupalcode.org/project/drupal/-/merge_requests/3047/diffs#25...
https://git.drupalcode.org/project/drupal/-/merge_requests/3047/diffs#25...
https://git.drupalcode.org/project/drupal/-/merge_requests/3047/diffs#25...

And quite a few more.

Posponed on #351303: Doesn't work in FF3 →

@bbrala: I think you linked to the wrong issue there :)

yeah just noticed, thanks!