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 BaseFieldOverrideconfig 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:

  1. Editor config entity (editor.editor.*): format
  2. MediaType config entity (media.type.*): source
  3. EntityViewDisplay config entity (core.entity_view_display.*.*.*): targetEntityType
  4. et cetera.

Steps to reproduce

N/A

Proposed resolution

  1. Extract ImmutablePropertiesConstraint(Validator) from that issue
  2. Add explicit test coverage for the new constraint
  3. 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

  1. ✅ Constraint + test coverage
  2. ✅ Adopt in all relevant config entities
  3. ✅ Update corresponding ConfigEntityValidationTestBase subclasses
  4. ✅ Create change record

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

None.

Issue fork drupal-3382580

Command icon 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

Wim Leers created an issue. See original summary.

wim leers’s picture

phenaproxima’s picture

Assigned: Unassigned » phenaproxima
wim leers’s picture

Issue summary: View changes

Fixed typos.

phenaproxima’s picture

Issue summary: View changes

Updating the issue summary because we now have the constraint here, with explicit test coverage.

phenaproxima’s picture

Assigned: phenaproxima » wim leers
Status: Active » Needs review
wim leers’s picture

Assigned: wim leers » phenaproxima
Status: Needs review » Needs work

Looking AMAZING 🤩

Config entities are used, and referenced, all over the freaking place. Throughout core, there are a ton of "hidden"/implicit points where changing the ID of another config entity will break stuff, sometimes badly. Therefore, I think it makes a lot of sense for all config entities to have immutable IDs by default, with a way to override that if necessary. I have implemented that.

💯 — 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 👍

phenaproxima’s picture

Issue summary: View changes

Updated the IS to explain the rationale for config entity IDs being immutable by default.

phenaproxima’s picture

Issue summary: View changes
phenaproxima’s picture

Title: Add new `ImmutableFields` constraint » Add new `ImmutableProperties` constraint
Issue summary: View changes
phenaproxima’s picture

Assigned: phenaproxima » wim leers
Issue summary: View changes
Status: Needs work » Needs review
wim leers’s picture

Assigned: wim leers » Unassigned
Issue summary: View changes
Status: Needs review » Needs work

Looking 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 😊

phenaproxima’s picture

Assigned: Unassigned » wim leers
Status: Needs work » Needs review
wim leers’s picture

Assigned: wim leers » Unassigned
Status: Needs review » Reviewed & tested by the community

🚢

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review

I like this MR a lot! I just have one question about it:

+  public function testImmutableProperties(array $valid_values = []): void {
...
+      $this->assertValidationErrors([
+        '' => "The '$property_name' property cannot be changed.",
+      ]);

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.

wim leers’s picture

Assigned: Unassigned » phenaproxima
Status: Needs review » Needs work
Related issues: +#2820364: Entity + Field + Property validation constraints are processed in the incorrect order

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.

My main concern with requiring overrides to prevent other errors within tests is that then potentially breaks existing contrib tests

\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.

Or, is there a compelling reason to assert that this is the only error?

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:

--- Expected
+++ Actual
@@ @@
 Array &0 (
     '' => 'The 'field_type' property cannot be changed.'
+    'settings' => Array &1 (
+        0 => ''on_label' is not a supported key.'
+        1 => ''off_label' is not a supported key.'
+    )
 )

… because on_label and off_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:

  public function validate($data, $constraints = NULL, $groups = NULL, $is_root_call = TRUE): static {
    if (isset($groups)) {
      throw new \LogicException('Passing custom groups is not supported.');
    }

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:

  1. I do think we should be as strict as we are here today, because those other validation errors make no sense.
  2. But to get rid of these work-arounds we should point to a follow-up. That already exists, fortunately: #2820364: Entity + Field + Property validation constraints are processed in the incorrect order.

This won't break contrib tests.


Marking Needs work to address @lauriii's remark on the MR.

phenaproxima’s picture

Assigned: phenaproxima » Unassigned
Status: Needs work » Needs review
lauriii’s picture

Status: Needs review » Reviewed & tested by the community

My feedback has been addressed. Also #17 makes sense to me. That's also how we've approached this in other config validation issues.

effulgentsia’s picture

Saving issue credits

  • effulgentsia committed 96248cfe on 11.x
    Issue #3382580 by phenaproxima, Wim Leers, lauriii: Add new `...
effulgentsia’s picture

Status: Reviewed & tested by the community » Fixed

Pushed to 11.x and published the CR.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.