Add new `ImmutableProperties` constraint

Fixed typos.

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

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 👍

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 😊

🚢

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 lot of this MR is overriding this method in subclasses 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 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.

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: 🐛 Entity + Field + Property validation constraints are processed in the incorrect order Needs work .

This won't break contrib tests.

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

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

Saving issue credits

Pushed to 11.x and published the CR.

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