- Merge request !3047Issue #3324140: Convert field_storage_config and field_config's form validation logic to validation constraints โ (Open) created by wim leers
- ๐ง๐ชBelgium borisson_ Mechelen, ๐ง๐ช
Is this currently blocked on writing and implementing the proposed
UniqueFieldCombination
? - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
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.
- last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago Build Successful - last update
over 1 year ago Build Successful - last update
over 1 year ago Build Successful - Assigned to wim leers
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
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โฆ
- last update
over 1 year ago 29,753 pass, 5 fail - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
- last update
over 1 year ago 29,777 pass, 4 fail - Issue was unassigned.
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
And one more push on
FieldConfig
, which meansFieldConfigValidationTest
should be passing tests now too! ๐คNext up:
- Implement
BundleExists
- Implement
Matches
- Implement
- Assigned to wim leers
- Issue was unassigned.
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Here's a new
EntityBundleExists
constraint, with explicit test coverage for bothFieldConfigValidationTest
andBaseFieldOverrideValidationTest
.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! ๐ฅณ - last update
over 1 year ago 29,750 pass, 11 fail - last update
over 1 year ago 29,772 pass, 8 fail - Assigned to wim leers
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
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 โฆ ๐ค ๐ต๏ธ
- last update
over 1 year ago 29,772 pass, 8 fail - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
- last update
over 1 year ago 29,797 pass, 7 fail - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
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, notFieldStorageConfig
! - last update
over 1 year ago 29,797 pass, 7 fail - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
@lauriii just surfaced ๐ [PP-1] Creating comment field w/o a comment type is allowed Postponed , which further confirms the need for this ๐ค
- Issue was unassigned.
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
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 inFieldConfigValidationTest
. - last update
over 1 year ago 29,804 pass, 5 fail - last update
over 1 year ago 29,810 pass, 4 fail - last update
over 1 year ago 29,878 pass, 2 fail - last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago 29,891 pass, 1 fail - last update
over 1 year ago 29,896 pass - Status changed to Needs review
over 1 year ago 2:14am 26 July 2023 - ๐บ๐ธUnited States phenaproxima Massachusetts
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?
- Status changed to Needs work
over 1 year ago 2:46pm 1 August 2023 - ๐บ๐ธUnited States smustgrave
Moving to NW for #26 comment.
Also are additional tests needed?
- Assigned to wim leers
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Will do a deep review of where we are right now, but HUGE thanks to @phenaproxima for getting this to green! ๐ฅณ
- Assigned to phenaproxima
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
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@todo
s 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 . - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
This is now postponed on:
- Status changed to Postponed
over 1 year ago 2:48pm 22 August 2023 - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
๐ Field [storage] config have incomplete settings until they are saved Fixed landed ๐
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
๐ Add new `ImmutableProperties` constraint Fixed landed.
- Assigned to wim leers
- Status changed to Needs work
11 months ago 8:58am 31 January 2024 - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
๐ Add new `EntityBundleExists` constraint Fixed landed. Time to catch this MR up on ~5 months of upstream changes ๐
- Issue was unassigned.
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
From
20 files, +851, -4
to11 files, +567, -21
โ thanks to - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
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 toFullyValidatable
and triaged the remaining@todo
s. - ๐บ๐ธUnited States phenaproxima Massachusetts
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 defaultmin
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.
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
๐ 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 ๐