
Problem/Motivation
Over at #3324140: Convert field_storage_config and field_config's form validation logic to validation constraints, we identified the need for a ImmutableProperties
constraint for Field(Storage)Config
and BaseFieldOverride
config entities.
For example, for the latter, the following config entity properties are considered immutable: field_name
, entity_type
, bundle
and field_type
.
However, there are at least a few other uses for this:
Editor
config entity (editor.editor.*
):format
MediaType
config entity (media.type.*
):source
EntityViewDisplay
config entity (core.entity_view_display.*.*.*
):targetEntityType
- et cetera.
Steps to reproduce
N/A
Proposed resolution
- Extract
ImmutablePropertiesConstraint(Validator)
from that issue - Add explicit test coverage for the new constraint
- Adopt in all config entities in core where it makes sense. In particular, all config entities' ID keys should automatically be immutable by default. That's because config entities are referenced by their IDs in many, many places, explicitly and implicitly, and there are countless places in core where changing a config entity's ID can break your site in unexpected ways. (It would even break config dependencies! ⚠️) So in general, a config entity's ID should never be changed. There will be a way to override this restriction for individual config entity types.
Remaining tasks
- ✅ Constraint + test coverage
- ✅ Adopt in all relevant config entities
- ✅ Update corresponding
ConfigEntityValidationTestBase
subclasses - ✅ Create change record
User interface changes
None.
API changes
None.
Data model changes
None.
Release notes snippet
None.
Issue fork drupal-3382580
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
wim leersComment #3
phenaproximaComment #4
wim leersFixed typos.
Comment #6
phenaproximaUpdating the issue summary because we now have the constraint here, with explicit test coverage.
Comment #7
phenaproximaComment #8
wim leersLooking AMAZING 🤩
💯 — maybe worth adding to the issue summary? 😄
Basically only questions that should help get this in a state where any core committer will feel comfortable committing this 👍
Comment #9
phenaproximaUpdated the IS to explain the rationale for config entity IDs being immutable by default.
Comment #10
phenaproximaComment #11
phenaproximaComment #12
phenaproximaComment #13
wim leersLooking awesome. Some nits, confirmed your suggested clarification, and requested test coverage for 2 more edge cases. Once that's taken care of, this is RTBC IMHO 😊
Comment #14
phenaproximaComment #15
wim leers🚢
Comment #16
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI like this MR a lot! I just have one question about it:
It seems like a good chunk of this MR is adding overrides of this method in subclasses in order to avoid triggering any validation errors except this one. However, could we instead of asserting that this is the only error, could we simply assert that this error is one of the errors, without the assertion failing if there are other validation errors as well? Then, we wouldn't need as many overrides preventing the other errors. Or, is there a compelling reason to assert that this is the only error?
My main concern with requiring overrides to prevent other errors within tests is that then potentially breaks existing contrib tests and/or adds a bit more work for contrib to start using this constraint. If there's not much that we gain from asserting that this is the only error, then I'd prefer to reduce as much friction as possible for contrib to adopt this.
Comment #17
wim leers\Drupal\KernelTests\Core\Config\ConfigEntityValidationTestBase
and all its subclasses are new, and they exist solely to help shepherd all config entity types in Drupal core to full validatability.That is a fair point. I previously asked the same question: https://git.drupalcode.org/project/drupal/-/merge_requests/4624#note_202156
For example if you remove the override at
\Drupal\Tests\field\Kernel\Entity\FieldConfigValidationTest::testImmutableProperties()
you'll get:… because
on_label
andoff_label
no longer make sense for the new (nonsensical) field type.Of course, the real problem is that we're executing validation constraints all at once, not in a conditional way. In this context,
ImmutableProperties
must run first, and if it is violated, then no other constraints even make sense to execute! For that, we'd need\Drupal\Core\TypedData\Validation\RecursiveContextualValidator::validate()
to not do this:Then we could use https://symfony.com/doc/current/reference/constraints/When.html — we could automatically apply
When
to run after the immutable properties are at least validat, because if they're not, there's no point in validating dependent things.An alternative approach would be to make
ImmutableProperties
use a non-default group, and then specify a sequence that the Symfony validator must respect.To then answer your actual question:
This won't break contrib tests.
Marking
to address @lauriii's remark on the MR.Comment #18
phenaproximaComment #19
lauriiiMy feedback has been addressed. Also #17 makes sense to me. That's also how we've approached this in other config validation issues.
Comment #20
effulgentsia CreditAttribution: effulgentsia at Acquia commentedSaving issue credits
Comment #22
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPushed to 11.x and published the CR.