- 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
about 1 year 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 š
- First commit to issue fork.
- Merge request !11266Draft: Make `StringPartsConstraintValidator` work again ā thanks to @phenaproxima a... ā (Closed) created by bbrala
- š³š±Netherlands bbrala Netherlands
Scary rebase, so made a new branch first to see how it handles itself.
- š§šŖBelgium borisson_ Mechelen, š§šŖ
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.
- š³š±Netherlands bbrala Netherlands
Seems good enough, moving to main MR, shoudln't need 'Needs review' :)
- š³š±Netherlands bbrala Netherlands
Todo: re-addthe creation of the entity types removed in one of last 2 commits
- š³š±Netherlands bbrala Netherlands
Well at least tests are close to OK again so this can be worked on again.
- š§šŖBelgium wim leers Ghent š§šŖšŖšŗ
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!
- š³š±Netherlands bbrala Netherlands
Although there is some missing test:
- The constraint MatchesOtherConfigValue needs tests
- The constraint NoEntitiesExistYetWithHigherCardinality needs tests
Our tests are green :x
- š³š±Netherlands bbrala Netherlands
I've split two issues for the missing tests.