Add a ReadOnly constraint validation and use it automatically for read-only entity fields

Looking great!

1. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ReadOnlyConstraint.php
   @@ -0,0 +1,24 @@
   +  public $message = 'This field is read-only and can not be changed.';
   

   This message must list the field name. Otherwise it'll be infuriatingly confusing.

2. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ReadOnlyConstraintValidator.php
   @@ -0,0 +1,64 @@
   +   * Constructs a ValidReferenceConstraintValidator object.
   

   c/p remnant

3. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ReadOnlyConstraintValidator.php
   @@ -0,0 +1,64 @@
   +    // We only care about pre-existing entities.
   

   s/pre-existing/updating/

4. Most importantly: because we have no forms that allow you to modify read-only fields, I think this change is entirely transparent to Drupal UI users? Hence no BC break? :)

The fails in the moderation test coverage here were fixed by #2824912: The moderation_state field incorrectly reports being read-only, I suspect.

Yup, but in general we shouldn't add this validation to computed fields, so here's a general fix for it.

We shouldn't? Shouldn't we disallow sending values for computed fields? Aren't all computed fields read-only by definition too? I guess that for something like the "path" field, a default value can be computed, but it's still modifiable — is that why not all computed fields are read-only too?

Besides that last question needing a solid answer, and probably needing documentation in the constraint's code, this looks ready to me. Although I think that this does need sign-off from somebody with deep Entity system knowledge: I don't think I can RTBC this.

Let me introduce you to #2392845: Add a trait to standardize handling of computed item lists :D

Oh dear lord.

Here's the fix for the last test fail, we just need to update the test expectation since we're receiving an additional validation error now.

Oh, this makes sense!

+++ b/core/lib/Drupal/Core/Field/FieldItemList.php
@@ -284,6 +284,13 @@ public function getConstraints() {
+    // Add the read-only constraint automatically for fields that need it.
+    if (!$this->getFieldDefinition()->isComputed() && $this->getFieldDefinition()->isReadOnly()) {

If I read the comment, then I expect only the second condition to be applied. Because that first condition is also there, this needs a clarifying comment. If we should link to https://www.drupal.org/node/2392845 to improve this in the future, then that's fine.

NW just for that comment. Other than that, this looks ready IMO.

I asked @amateescu about berdir's concerns.

I can't really remember the exact discussion, but it was about the loadUnchanged() call, which is very costly because it bypasses all caches, and it would be called a lot of each field that was marked as required

The constraint loads the unchanged entity and compares if the value changed.

In my opinion, the constraint would always throw a violation if !$entity->isNew(). The value should not be considered valid if you're providing the value after the entity is created. Read-only values, as I have come to take them, are things you set when creating the entity.

For instance: bundles are read-only.

In Drupal Commerce a payment method's payment gateway reference is read-only. A payment transaction can reference a payment method and its payment gateway. These are read-only. Each of these fields is populated when the entity is created and does not change.

Rolling an updated patch.

Here is a reroll of #10. It takes into consideration #11 and doesn't check computed fields, as those wouldn't be receiving a value to be validated.

The constraint validator now merely checks if the entity is not new.

🤦‍♂️ I canceled the build.

+++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ReadOnlyConstraintValidator.php
@@ -0,0 +1,31 @@
+    if (!$value->getEntity()->isNew()) {
+      $this->context
+        ->buildViolation($constraint->message, ['%field_name' => $value->getName()])
+        ->atPath($value->getName())
+        ->setInvalidValue($value)
+        ->addViolation();
+    }

I was thinking too simplistically and this causes any later save to mark those fields as invalid.

This would be somewhat easier if we could access the $values property on fieldable entities.

  /**
   * The plain data values of the contained fields.
   *
   * This always holds the original, unchanged values of the entity. The values
   * are keyed by language code, whereas LanguageInterface::LANGCODE_DEFAULT
   * is used for values in default language.
   *
   * @todo: Add methods for getting original fields and for determining
   * changes.
   * @todo: Provide a better way for defining default values.
   *
   * @var array
   */
  protected $values = [];

  /**
   * The array of fields, each being an instance of FieldItemListInterface.
   *
   * @var array
   */
  protected $fields = [];

Could this be tackled with $entity->original or by loading the entity from storage to compare to?