Provide non-configurable field types for entity created, changed and timestamp fields

This makes a lot of sense, validation actually just being one of multiple advantages.

It also allows to standardize the update on preSave() logic, helps figuring out which is the changed field name (if you e.g. need it for an EFQ query) and possibly more.

Not sure I fully understand the validation changes, but it doesn't even matter that much how it's done, developers just need to set the field type and then it will work, if it's a custom constraint or not is kind of irrelevant for the API.

Started with this, added a timestamp, created and changed field. Did not yet change the validation constraint, just moved it into the class.

Would be good to get this in before: #2074475: Rename {file_managed}.timestamp to 'changed' and add a created timestamp which is a beta blocker.

Apart from the fact that I don't really see why that is a beta blocker, it doesn't really matter in which order it goes in. It would save a few lines in the other issue, yes, but either issue can easily be re-rolled if the other one gets committed.

Usually i'd agree that a prefix is clearer however it's a pretty common and understand to have changed/created fields so i'm not sure it matters much either way.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedItem.php
@@ -13,8 +13,11 @@
+ *   constraints = {
+ *     "ComplexData" = {"value" = {"EntityChanged" = TRUE}}
+ *   }

Out of interest, could you explain what this does? I can't find much/any docs on yml, complex data and constraints. Eg how did you know to use the EntityChanged key?

Didn't fix the empty array thing in annotations?

#1848570: Upgrade to Doctrine\Common 2.4

So those overrides shouldn't be necessary anymore...

This should fix most tests, also removing the empty array hack.

I'm not keen enough to RTBC myself, but I couldn't find anything to complain about.

Also, this patch is **awesome**. Really, really awesome!

This is actually not supposed to be green yet :)

See #2005716-129: Promote EntityType to a domain object, there are tests that alter the node entity class to prevent changed from being overridden, so this shouldn't pass, strange. Maybe the test run was so slow that it didn't matter.

There might also be some other changed related tests, that could be affected by this, I know there's one that changes the changed date in a presave hook.

13: changed-field-type-2145103-13.patch queued for re-testing.

Noticed that the change in ConstraintManager should be unnecessary now and found another snippet that can be removed.

@fago initially said that he wants to to the validation part differently, is that still on the table/necessary or should we consider to do that later, as it should now be fairly transparent to whoever's using it?

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/TimestampItem.php
   @@ -0,0 +1,44 @@
   +
   

   Strange newline?

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/TimestampItem.php
   @@ -0,0 +1,44 @@
   +        ->setLabel(t('Timestamp value'));
   
   +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Entity/CustomBlock.php
   @@ -198,11 +188,9 @@ public static function baseFieldDefinitions($entity_type) {
   +      ->setDescription(t('The time that the custom block was last edited.'));
   
   +++ b/core/modules/node/lib/Drupal/node/Entity/Node.php
   @@ -396,16 +375,13 @@ public static function baseFieldDefinitions($entity_type) {
   +      ->setDescription(t('The time that the node was last edited.'));
   
   +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Entity/Term.php
   @@ -239,10 +229,9 @@ public static function baseFieldDefinitions($entity_type) {
   +      ->setDescription(t('The time that the term was last edited.'));
   

   Is it okay to use t()?

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedItem.php
   @@ -0,0 +1,33 @@
   +class ChangedItem extends CreatedItem {
   

   Why does this extend CreatedItem and not TimestampItem? Because we assume that you cannot want a ChangedItem without also having a CreatedItem? Is that even true? Why does a ChangedItem need to use CreatedItem::applyDefaultValue(), which it does due to inheritance?

4. +++ b/core/modules/edit/lib/Drupal/edit/Form/EditFieldForm.php
   @@ -224,17 +225,16 @@ protected function simplify(array &$form, array &$form_state) {
   +   * @param \Drupal\Core\Entity\ContentEntityInterface $entity
   ...
   -   * @param \Drupal\Core\Entity\EntityInterface $entity
   

   Nice :) I think I probably need to apply this change everywhere in edit.module?

17: changed-field-type-2145103-17.patch queued for re-testing.

re-rolling

Regarding #18

1. Removed new line in TimestampItem::getPropertyDefinitions()
2. Using t() seems to be fine. It's used in all other implementations too.

3. Because we assume that you cannot want a ChangedItem without also having a CreatedItem?

   As far as I understand that doesn't imply that there's an created field, however the applyDefaultValue() is inherited. And I'm not really sure if this is needed / ok like that. Wouldn't that initialization prevent us from recognize if there was a change already or not?

The aggregator tests failed because the timestamp and not the created item was used. Adjusted now.
Re-roll was necessary due the "newly" introduced schema method in FieldItemInterface.

Updated patch to cover file entity as well.
Additional cleanup change for file entity to remove legacy handling of language field.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/TimestampItem.php
@@ -27,13 +28,12 @@ class TimestampItem extends FieldItemBase {
    *
    * @var array
    */
-  static $propertyDefinitions;
+  public static $propertyDefinitions;
 

For the record, it's like that in pretty much all item classes right now.

Not relevant as it will go away soon anyway.

As far as I understand that doesn't imply that there's an created field, however the applyDefaultValue() is inherited. And I'm not really sure if this is needed / ok like that. Wouldn't that initialization prevent us from recognize if there was a change already or not?

I don't really understand this. It would only be relevant for an entity that is still new, and then changed is the same as created, aka now. Without applyDefaultValue(), it would be NULL. I think I explicitly extended it from CreatedItem because there are some tests that test that behavior.

Patch passed, just the incorrectly named interdiff caused it to be set to needs work.

*grml* sorry for the noise.
I'm confused now - one of the last test said the patch failed too.
Re-adding to have a proper test - no difference to previous patch.

33: changed-field-type-2145103-33.patch queued for re-testing.

33: changed-field-type-2145103-33.patch queued for re-testing.

This patch looks great! Nice cleanups and fixed @todos :)

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedItem.php
   @@ -0,0 +1,33 @@
   + * Defines the 'changed' entity field type.
   ...
   + *   description = @Translation("An entity field containing a UNIX timestamp of when the entity has been last updated."),
   
   +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/CreatedItem.php
   @@ -0,0 +1,32 @@
   + *   description = @Translation("An entity field containing a UNIX timestamp of when the entity has been created."),
   ...
   + * Defines the 'created' entity field type.
   

   Is it just me that the description explains things way better than the general class documentation?

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/CreatedItem.php
   @@ -0,0 +1,32 @@
   diff --git a/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   
   diff --git a/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   index 3a776e2..4b07469 100644
   
   index 3a776e2..4b07469 100644
   --- a/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   
   --- a/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   
   +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -25,7 +25,7 @@
   
   @@ -25,7 +25,7 @@
     *   label = @Translation("Entity reference"),
     *   description = @Translation("An entity field containing an entity reference."),
     *   configurable = FALSE,
   - *   constraints = {"ValidReference" = TRUE}
   + *   constraints = {"ValidReference" = {}}
     * )
     */
   

   Just figured it out: annotations recently got empty array support.

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedItem.php
   @@ -0,0 +1,33 @@
   +class ChangedItem extends CreatedItem {
   

   Semantically this feels a bit odd but let's be pragmatic here.

4. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/TimestampItem.php
   @@ -0,0 +1,57 @@
   +
   +/**
   + * Defines the 'timestamp' entity field type.
   + *
   
   +++ b/core/modules/edit/lib/Drupal/edit/Form/EditFieldForm.php
   @@ -224,17 +225,16 @@ protected function simplify(array &$form, array &$form_state) {
   -  protected function getChangedFieldName(EntityInterface $entity) {
   -    foreach ($entity as $field_name => $field) {
   -      $constraints = $field->getItemDefinition()->getConstraints();
   -      if (isset($constraints['ComplexData']['value']['EntityChanged'])) {
   -        return $field_name;
   +  protected function getChangedFieldName(ContentEntityInterface $entity) {
   +    foreach ($entity->getPropertyDefinitions() as $field) {
   +      if ($field->getType() == 'changed') {
   +        return $field->getName();
          }
        }
   

   That is way better to read now, thank you!

changed-field-type-2145103-33.patch no longer applies.

Rerolled.

41: changed-field-type-2145103-41.patch queued for re-testing.

Another re-roll, only conflicted in use statements.

45: changed-field-type-2145103-45.patch queued for re-testing.

45: changed-field-type-2145103-45.patch queued for re-testing.

45: changed-field-type-2145103-45.patch queued for re-testing.

Re-roll, who wants to set this to RTBC? :)

Re-roll.

Updated the code to fix those test fails.

Back to RTBC.

changed-field-type-2145103-57.patch no longer applies.

error: patch failed: core/modules/aggregator/lib/Drupal/aggregator/Entity/Item.php:49
error: core/modules/aggregator/lib/Drupal/aggregator/Entity/Item.php: patch does not apply

Re-roll. Conflicted with the changed baseFieldDefinitions() method, nothing in the patch changed.

Looking good

Overall this looks like really nice clean-up, and yay consistency for frequently-used fields.

However...

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedItem.php
   @@ -0,0 +1,33 @@
   + *   constraints = {
   + *     "ComplexData" = {"value" = {"EntityChanged" = {}}}
   + *   }
   

   This is a "pre-existing condition," but... ouch. :( I literally have no idea how to translate that into English. Looks like jibran had trouble up above, too...

   I'm going to make a major (for now) beta target to clean up that DX, because...

2. +++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/Email.php
   @@ -17,7 +17,7 @@
   - *   constraints = {"Email" = TRUE}
   + *   constraints = {"Email" = {}}
   

   ...this is just silly. I see that it saves us a 3-line hack in TypedDataManager, but it really makes the DX totally non-intuitive. (And easy to mis-type, too... accidentally leaving off a }, typing a ] instead, etc.)

Both of those are issues exposed with the patch, not caused by it though, so no reason to hold this up.

In terms of change notice, Berdir said it didn't really make sense to make one specifically about this, since the code it's replacing was embedded in node_save(). He's going to update https://drupal.org/node/1806650 and https://drupal.org/node/2031221 instead to note this for people upgrading from 7 to 8, since those are the main places they'll look.

Therefore, committed and pushed to 8.x. Thanks!