Add a trait to standardize handling of computed item lists

Demo field.

This patch instead of throwing the dice a random number of times, uses the value defined by a different field.

Hope I understood your idea, yched.

Attached patch includes a computed field, which depends on a different field. This is a working solution, I highlight the things that a computed field writer will struggle with:

1. +++ b/core/modules/field/tests/modules/field_computed_test/src/Plugin/Field/FieldType/DiceFieldItem.php
   @@ -0,0 +1,37 @@
   +  public static function schema(FieldStorageDefinitionInterface $field_definition) {
   +    return array('columns' => []);
   +  }
   

   I would expect this is not needed.

2. +++ b/core/modules/field/tests/modules/field_computed_test/src/Plugin/Field/FieldType/DiceFieldItemList.php
   @@ -0,0 +1,24 @@
   +  public function getValue($include_computed = FALSE) {
   +    $items_count = $this->getParent()->getValue()->field_dice_count->value;
   

   Shouldn't this be easier/less verbose? What's that flag about?

Let me know how we can move further and I'll do my best.

Removed the field item and formatter. Used hook_entity_base_field_info for adding both fields.
The field is not shown, still have to figure out why.

It seems regular fields get their values set by the TypedDataManager when they are first accessed in getPropertyInstance(). So I thought that place would be optimal to create the items.
Created a new list class FieldItemListComputed that has two methods:

• abstract protected computeItemValues(): To generate/compute a values array, like the one used to initialize normal fields
• public computeItems(): Uses the values from computeItemValues() to populate it's field item list.

In getPropertyInstance() the field is checked whether it uses FieldItemListComputed and is instructed to populate it's values with computeItems().
I moved the computation logic in DiceFieldItemList from getValue to computeItemValues(). I also removed the overriden getValue() and count() because computeItems() writes the values into the $list property so their implementations in FieldItemList should give the desired result.
Also FieldItemListComputed has a bunch of ovverriden methods which should enforce that the computed list can't be changed from the outside of the class.

Do these changes make sense?

Thanks for working on this, @Alumei!
I like it, would love @yched view on this.

Sadly the patch above is not the full solution as it works for base-fields only. I did some additional work to make it work for configurable fields but haven't come around to making a patch out of it ...
Working on it now.

I found a solution that works for fields that use a custom field type as those can specify their list class in their annotation. Sadly this enables computes configurable fields for custom field types only, but its a start ;-)

Apparently my solution wasn't finished yet -_-
While trying to incorporate a computed configurable field into the test module I encountered several problems e.g.:
- views integration does not account for computed fields
- when uninstalling and validating the uninstallation of a module computed fields are checked for content on which the storage errors out because there is no data in the db for computed fields

Sadly is seems like I messed up my code base and I don't know when I will have time to work on it again.

A fresh start really helped ;-)

I created a small increment on top of my previous patch containing:

• Renamed DiceFieldItemList to DiceItemList.
• Added a field type DiceItem that specifies a list_class = "\Drupal\field_computed_test\Plugin\Field\FieldType\DiceItemList".
• Added a required widget and formatter for DiceItem.
• ListDataDefinition now decides if it's computed based on the list class used and falls back to the parent behavior if undecided.
• Added 2 additional base-fields: One uses the new field-type and the other tests that the change in ListDataDefinition works even if the base-field was not explicitly set to be computed.

This should make the "computed field" use-case for base-fields pretty much usable.
As I wrote before: In theory these changes could work for configurable fields as well (At least for custom field types). But there probably will be problems with the UI and with FieldConfig & FieldStorageConfig though i haven't tested it yet.

Just did a fast test for configurable fields:

1. Errors out because in the field type there is no columns key defined.
2. After that i get the following error: There was a problem creating field The dice field: The "integer" plugin does not exist.

I had that problem in a previous version but don't know why that happens.

Tried to fix most of the test fails and it also contains the fixed schema for the custom field type.
If I did everything right it should come down to two fails, but I have not particular experience in writing tests so ...

For testbot ...

I think this fixes the last two fails.

I assumed for both patches that they only failed because isComputed() calls getClass() which uses the typed_data_manager service which was in all instances not part of the testing container.

It's getting late -_-
Gn8 @ testbot

Np ;-)
Got inspired by #1548204-88: Remove user signature and move it to contrib and thought of a computed field approach. Then I remembered stumbling upon this issue and checked back on it.

Nevertheless I finally found the reason why this still fails. Apparently I overlooked that the EntityAdapterUnitTest already defined a typed-data-manager and i accidentally replaced it with my lines further down ...

Let's see if it works now.

Finally got computed fields to work .. even for configurable fields.
Wanted to go into detail but it really is to late now :-/
@yched I hope the patch is enough for now if you get the time at the dev days, but will try to follow up tomorrow.

These are the changes from the last patch:

• FieldItemListComputedInterface now extends from FieldItemListInterface to emphasise that the computed item is a specal case of an item list
• Changed the ReflectionClass based subclass checks to use is_subclass_off() instead
• Updated the subclass checks to check against the interface instead of the abstract class
• Added a protected property to FieldConfig and implemented the isComputed() method to check against the interface and cache its result in the new $computed property
• Updated isReadOnly() inside of FieldConfig to return the result of isComputed() analogue to the implementation in BaseFieldDefinition
• Added the $computed propery to FieldStorageConfig and also added a protected isComputed() method that works the same as in FieldConfig (Would be nice if it could be public and on the interface t=so that it could be reused on FieldConfig but wasn't sure whether that interface change would be appropriate)
• Updated isQueryable() and hasCustomStorage() inside of FieldStorageConfig to use the result of isComputed() analogue to the implementation in BaseFieldDefinition
• Changed hasData() on FieldStorageConfig to only check the database for data if the field is not computed and return false for that case.
• Removed the field_ prefix from the custom base fields to bettter distinguish them from configurable fields
• Updated the custom base field definitions to use the correct widgets and formatters (This solved the my problem from #17.2)
• Updated FieldStorageAddForm to skip the field storage config and field config forms for computed fields (This made more sense for me when I implementet it. The intention was that computed fields may not need to configure anything and in that case the only important data is the field name & label, which is already specified at that point. Nevertheless both forms are still accessible from the field overview)

Obvious problems I know exists atm are:

• Despite the fact that there is an error when removing a computed field via UI that field seems to be removed anyway
• Somehow the test module is displayed on the Uninstall screen

Based on the last patch on needs to implement the following to realize a computed fields:

1. Create a new Item-list class that implements FieldItemListComputedInterface.
   • Either implement custom initialization logic and put it in computeItems().
   • Or extend FieldItemListComputed which forces you to implement computeItemValues() instead. In computeItemValues() you specify an array keyed by delta. where each value is an array again containing the values for an field-item and that is keyed by property name. This makes it possible to easily create a custom field-type for computed fields in the same way you would for a normal fields or provide the correct data structure to be able to reuse an existing field type.
2. If you want to support the configurable fields use-case with your computed field as well as the base fields one you need to create a custom filed type as well. There you must set list_class = "" in the filed type annotation to point at the item-class from 1. (This is necessary because list_class can not be specified in the UI only in the annotation and in code)
3. Also to support the configurable fields use-case it is currently required that you create a dummy widget for your field type as the field UI requires it
4. At last create a custom formatter if you if you created a new field type

Should the widget requirement be removed from field UI for computed fields? I think that requirement is bad DX for computed fields as they usually should not require the input of data in an edit or insert form (In those cases a computed field would probably be the wrong choice and computed properties may be more appropriate). I would propose not to remove widgets for computed fields but make them optional instead.

Attempted reroll.

reroll

I have an urgent need for a computed field for a contrib module see #2738807: Stabilize the stock level computed field implementation
Anyone know of an alternative approach?

Quite a few contrib modules could make use of computed entityreference fields. I tried making one for Group module, and it didn't seem to be possible: #2718195: Add a computed field for entity's group(s).

rerolled

Thanks Alumei for all the work done here already!

ad encountered bugs:

- views integration does not account for computed fields
- when uninstalling and validating the uninstallation of a module computed fields are checked for content on which the storage errors out because there is no data in the db for computed fields

I think the best way to proceed here would be to get an example implementation + base class committed first and then open follow-up bug reports for any remaining issues not causing test fails, and addressing them individually while improving test coverage.

I took a look at the patch:

1. +++ b/core/lib/Drupal/Core/Field/FieldItemListComputed.php
   @@ -0,0 +1,59 @@
   +abstract class FieldItemListComputed extends FieldItemList implements FieldItemListComputedInterface {
   

   I don't think we should introduce a new interface. So far we have not interface that computed fields or properties must implement and I do not really see a need for having it as computeItems() shuold be only called internally, not?

2. +++ b/core/lib/Drupal/Core/TypedData/ListDataDefinition.php
   @@ -108,4 +109,42 @@ public function setItemDefinition(DataDefinitionInterface $definition) {
   +      return $reflectionClass->isSubclassOf('\Drupal\Core\Field\FieldItemListComputed');
   

   Interesting idea, but why is that necessary? When someone sets a custom class for a field to become computed, one can set the flag as well?
   Is it for allowing a field type to be computed by default? That's a different feature/topic which I'd prefer to deal separately with.

3. +++ b/core/modules/field/tests/modules/field_computed_test/src/Plugin/Field/FieldFormatter/DiceFormatter.php
   @@ -0,0 +1,32 @@
   +class DiceFormatter extends FormatterBase {
   

   seems useless, should stick with default integer formatters.

4. +++ b/core/modules/field/tests/modules/field_computed_test/src/Plugin/Field/FieldWidget/DiceWidget.php
   @@ -0,0 +1,30 @@
   +      '#markup' => 'This is a computed field that contains a number of random values where the amount of items depends on value of the field_dice_count field on the same entity.',
   

   hm, a widget for a computed field generally seems wrong -> we should throw exceptions when that is configured. OR, are there valid use cases for having that, e.g. embedding a form for editing computed data?

Summarizing I think we should go step by step and reduce the scope of the first patch to a single base field, that's manually defined computed. Then, we can add another configurable, computed one. And finally, as last step the field type being computed.

Updated issue summary and re-rolled for 8.2.x. I'll be looking at this today, seeing if I can create a patch for a manually computed base field first.

Let's see what testbot has to say.

Unassigning myself as I don't have time to work on this now and didn't make any progress since last week.

The semantics of "computed fields" being unclear also is getting in the way at #2826101-8: Add a ReadOnly constraint validation and use it automatically for read-only entity fields.

New reroll against 8.4.x. Also fixed comments from #52 and minor fixes I noticed.

• Removed FieldItemListComputedInterface
• Removed the isComputedByClassInheritance() related logic from ListDataDefinition (this also seemed to fix the tests!)
• Removed DiceFormatter and DiceWidget

Whoops!

While using this to create a computed entity reference field BaseFieldDefinition::create('entity_reference') in hook_entity_base_field_info(), I found that the class implementing the computed field needs to extend EntityReferenceFieldItemList. This off course means I can't use FieldItemListComputed. Other field types might have the same problem.

This leads me to believe we should probably only define a FieldItemListComputedInterface which computed field should implement.

Something like this should do it. Renamed computeItems() to computeValue() to be more in line with the regular setValue() method for non computed fields.

1. +++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
   @@ -200,6 +201,11 @@ public function getPropertyInstance(TypedDataInterface $object, $property_name,
   +      $property->computeValue();
   

   If I am not mistaken then this will compute the values when the field item list is being initialized, but the values have to be computed only when they are needed/accessed, right?

2. +++ b/core/modules/field/tests/modules/field_computed_test/src/Plugin/Field/FieldType/DiceItemList.php
   @@ -0,0 +1,43 @@
   +    if (is_array($values) && !empty($values)) {
   +      foreach ($values as $delta => $value) {
   +        $this->list[] = $this->createItem($delta, $value);
   +      }
   +    }
   

   Let's convert this simply to:
   $this->setValue($values);

3. +++ b/core/modules/field/tests/modules/field_computed_test/src/Plugin/Field/FieldType/DiceItemList.php
   @@ -0,0 +1,43 @@
   +    foreach (range(0, $items_count - 1) as $delta) {
   

   A traditional for loop is faster than foreach + range.

Re #65:
About

This leads me to believe we should probably only define a FieldItemListComputedInterface which computed field should implement.

@fago, has said in #52

I don't think we should introduce a new interface. So far we have not interface that computed fields or properties must implement and I do not really see a need for having it as computeItems() shuold be only called internally, not?

I think we should put the computeValue method into the FieldItemListInterface instead and in the implementation just check if the field definition is computed and only in this case compute the values, otherwise do nothing.

Thanks so much for pushing this forward again!

#67.1 I think so. Not sure if this is a bad thing. Do you have suggestions on how to improve this?
#67.2 Done
#67.3 Done

I think we should put the computeValue method into the FieldItemListInterface instead and in the implementation just check if the field definition is computed and only in this case compute the values, otherwise do nothing.

I think adding computeValue() to FieldItemListInterface breaks BC. Having a interface only for computed fields seems to be a clean solution to deal with this. I like the idea of checking the isComputed() method from the definition in TypedDataManager, but we need a way to enforce the implementation of the computeValue() method. If there is a way we can avoid the new interface without breaking BC, I'm open to suggestions.

We could maybe use the getValue() method and skip computeValue()? When reading the documentation it seemed this is the only method computed fields need to implement anyway? Not sure if this would break anything though.

#67.1 I think so. Not sure if this is a bad thing. Do you have suggestions on how to improve this?

It might be a bad thing if the computed field has to perform complex computations. The example form the issue summary - a "back reference" field (a field that lists all the entities that have an entity_ref field pointing to that entity) - will execute an entity query each time an entity with such a field is loaded, but probably the field will be accessed only in some cases e.g. on entity form edit, but not on entity view.

If we want to solve this for all the cases e.g. cover the use case where the value is retrieved directly from the property instead from the field item list or from the field item - $field_item->get($property_name)->getValue() - then the only place where we could implement this logic is in TypedData::getValue(), however we don't have any field related logic in TypedData. Probably we could add something like the following code there :

    if (!isset($this->value) && ($parent = $this->getParent()) && $parent instanceof FieldItemInterface && $parent->getFieldDefinition()->isComputed()) {
      $parent->getParent()->computeValue();
    }

but I guess a lot of people might be against this...

I think adding computeValue() to FieldItemListInterface breaks BC. Having a interface only for computed fields seems to be a clean solution to deal with this. I like the idea of checking the isComputed() method from the definition in TypedDataManager, but we need a way to enforce the implementation of the computeValue() method. If there is a way we can avoid the new interface without breaking BC, I'm open to suggestions.

There are good news about this, because it is not a BC break :). From #2862574-11: Add ability to track an entity object's dirty fields (and see if it has changed):

We just had a discussion about the new interface on IRC and personally at the DrupalDevDays Seville with tstockler and berdir and they both agree that we add new methods to content entities by adding them to ContentEntityInterface and in D9 they should be moved to the according interface - in this case to FieldableEntityInterface. We've done this for an example with the getLoadedRevisionId method.

This means we do not need the new interface and instead we have to add the new method to ContentEntityInterface.

We add new methods to interfaces without having a BC break if there is a base class implementing the interface and here we have this relationship. This is described as well in https://www.drupal.org/core/d8-bc-policy under the section Interfaces within non-experimental, non-test modules not tagged with either @api or @internal.

We could maybe use the getValue() method and skip computeValue()? When reading the documentation it seemed this is the only method computed fields need to implement anyway? Not sure if this would break anything though.

Well this sounds good as well :). I don't think either that we really need a new method, expect we decide to use the solution I've proposed at the beginning of this comment about #67.1.

How does this solve the issue of FieldConfig's not being able to be computed? I.e. https://api.drupal.org/api/drupal/core%21modules%21field%21src%21Entity%... is hard coded to return FALSE.

Let me explain my use case:

Currently I am building out a framework where node types and fields are dynamically generated. Each node type has a set of fields that are stored in the database, custom_storage set to FALSE. Then there are a handful of field configs where there custom_storage is set to TRUE. It's a hybrid data model.

The custom storage field values are looked up from an external datasource(Cassandra) based on the field values that are stored in the database. Then their values are filled in via hook_entity_storage_load.

In addition, the fields that have custom storage set to TRUE are still queryable because we have implemented a new entityquery backend that detects when a query is ran against these bundles and queries a backend service that can handle the query (Elasticsearch).

At this point I have gotten all this working nicely. The issue I am facing now is that I want to create a search index from my entities via search_api. Search api relies on fieldconfig object to determine if an field is indexable via isComputed method, but it's hard coded to be FALSE.

In my opinion, fieldconfig should allow the following:

1. Allow the configuration to set isComputed
2. Allow the configuration to set isQueryable

If this is the wrong ticket or if I am taking the wrong direction to my problem please let me know, but I feel like it ties into this.

From #14 and #52:

- views integration does not account for computed fields

[....]

I think the best way to proceed here would be to get an example implementation + base class committed first and then open follow-up bug reports for any remaining issues not causing test fails, and addressing them individually while improving test coverage.

It looks like some related activity on the views-integration is happening at #2852067: Add support for rendering computed fields to the "field" views field handler.

From #52

hm, a widget for a computed field generally seems wrong -> we should throw exceptions when that is configured. OR, are there valid use cases for having that, e.g. embedding a form for editing computed data?

I'm not sure if this is still an open question or not, but I do believe that having the ability to leverage the field API front-end, and related widgets, for computed data is quite compelling. I'm thinking specifically of custom DB storage needs that do not directly leverage Drupal field-based storage models. We have a case where several custom entity types have many-to-many relationships that are handled via custom mapping tables. Being able to expose these relationships (editorially) as a computed entity reference field, using the native entity reference widgets, is extremely useful. It's very easy to then use pre/postSave() to persist the data from the computed field widget in a custom way. I'm sure there are numerous other use cases where field API widgets can be an asset to custom computed storage needs.

I've been working on some patches, I will clarify things later this week. My biggest considerations are:

• Why should it be necessary to create a new Field type for computed fields, as is suggested by the DiceItem test class? We should be able use use existing field types, and make them computed with the setComputed() and setClass() methods on the BaseFieldDefinition. Using existing field types we can use their validations too. I've added some examples in the tests
• When should the computed field be computed? And should the computed field itself not be in charge of storing the computed result. I will explain this later on

I see i missed #69, I will revise that one later.

More later...

Could you also add interdiffs for easier review?

Apologies I rushed that a bit too much and uploaded your patch in stead of interdiff. On the other hand I realise that I'm probably missing things that were mentioned already in this issue queue.

About #69

1. +++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
   @@ -200,6 +201,11 @@ public function getPropertyInstance(TypedDataInterface $object, $property_name,
   +    elseif ($property instanceof FieldItemListComputedInterface) {
   +      // populate the computed list with values as there are no initial values
   +      // to set.
   +      $property->computeValue();
   +    }
   

   I think the computed result here is later overwritten by ContentEntityStorageBase::initFieldValues(), changing the computed value that was set earlier. That's why the tests that I added to your patch fail.

   On the other hand I think that these values should be validated.

2. +++ b/core/modules/field/tests/modules/field_computed_test/src/Plugin/Field/FieldType/DiceItemList.php
   @@ -0,0 +1,38 @@
   +        'value' => rand(1, 6),
   

   I'm not a big fan of random values in tests, as it introduces random errors and it makes it more difficult to test. Therefore I replaced this test with another, but I might have thrown the baby out with the bathwater. Will revise that later to see I can incorporate things.

3. +++ b/core/modules/field/tests/modules/field_computed_test/src/Plugin/Field/FieldType/DiceItemList.php
   @@ -0,0 +1,38 @@
   +  public function computeValue() {
   +    $values = $this->computeItemValues();
   +    $this->setValue($values);
   +  }
   

   Wouldn't it be better (thinking out loud) to return the values as an array and let a protected method set them. A 'computeValue' method that 'sets' something sounds confusing. Plus it requires the developer do make an extra step that we could do in a protected method. Just returning the array might be more intuitive.

   I would also suggest to rename the method to 'computeValues'.

About #79

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -486,7 +486,7 @@ public function hasField($field_name) {
   -    if (!isset($this->fields[$field_name][$this->activeLangcode])) {
   +    if (!isset($this->fields[$field_name][$this->activeLangcode]) || $this->fields[$field_name][$this->activeLangcode]->isComputed()) {
   

   This means the computed field is computed again and again everytime it is requested. The reason why I added this is because the entity might change during a single page request, and the computed value with it.

   e.g. when creating an entity with a computed value based on its ID, the entity ID won't be available until the entity is saved, yet the computation - in the current setup - will have run before that. That's why it might be a good idea to force the computation to run again and again, and let the implementation define the way the computed value is stored.

   I'm not sure this is still valid when implementing hchonov's suggestion in #70 of only computing the value when called from TypedData::getValue()

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -108,7 +108,7 @@ protected function initFieldValues(ContentEntityInterface $entity, array $values
   -        elseif (!array_key_exists($name, $values)) {
   +        elseif (!array_key_exists($name, $values) && !$field->isComputed()) {
              $entity->get($name)->applyDefaultValue();
            }
   

   Don't apply default values to computed fields.

3. +++ b/core/lib/Drupal/Core/Field/FieldItemList.php
   @@ -160,6 +162,31 @@ public function __unset($property_name) {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function __construct(DataDefinitionInterface $definition, $name = NULL, TypedDataInterface $parent = NULL) {
   +    parent::__construct($definition, $name, $parent);
   +  }
   

   This can go obviously.

4. +++ b/core/lib/Drupal/Core/Field/FieldItemList.php
   @@ -160,6 +162,31 @@ public function __unset($property_name) {
   +  public function getComputedValues(){
   +    $this->list = $this->computeValues();
   +    $errors = $this->validate()->getIterator();
   +    if($errors->current()){
   +      throw new FieldException($errors->current()->getMessage());
   +    }
   +  }
   

   Here I propose to let another method set the values returned by the computeValues method, for better developer experience. The developer just has to return an array with values.

   I also added field validation for computed fields, which should throw the right exception when the values returned don't match the data types.

The tests I've added are meant to check if we can get computed fields by just having one field item class, and if the values are correctly parsed and throw the right exceptions.

I realise that I haven't incorporated some of the things mentioned in #70 yet.

#77.1 Haven't run into that yet, but it seems to make sense.
#77.2 Let's see what the testbot says.
#77.3 Sounds reasonable.

#78.1 I agree, we should document on the interface computed fields are responsible for their own caching.
#78.2 Sounds reasonable
#78.3:)
#78.4 I like it.

Fixed some small nits. Let's run the tests and see what will happen...

The test seem to fail because of the change in #9. Reverted the change in Drupal\Core\TypedData\Plugin\DataType\ItemList to fix it.

Hmm, I think we found a bug in the ContentEntityBaseUnitTest? The test provided a string where $this->fields should be "The array of fields, each being an instance of FieldItemListInterface." (at least according to the docs). So I added a mocked FieldItemList and that fixed it. We definitely need some feedback from the entity system maintainers though since I'm not sure if this was actually a bug.

And a whole bunch of coding standard fixes while I was at it...

1. #70:

   If we want to solve this for all the cases e.g. cover the use case where the value is retrieved directly from the property instead from the field item list or from the field item - $field_item->get($property_name)->getValue() - then the only place where we could implement this logic is in TypedData::getValue(), however we don't have any field related logic in TypedData. Probably we could add something like the following code there :

     if (!isset($this->value) && ($parent = $this->getParent()) && $parent instanceof FieldItemInterface && $parent->getFieldDefinition()->isComputed()) {
         $parent->getParent()->computeValue();
       }

   I have tried this but have given up. I think that what would be required is not $this->getParent()->getFieldDefinition()->isComputed but $this->getParent()->getParent()->getFieldDefintion->isComputed() as the flag is set on field item list level, and not on the item level itself. I've tried a lot of ways to let TypedData::getValue() fetch the computed values of both a field property (typed Data) as well as a field item list, but It doesn't seem to be easy and is likely to require a lot of small interventions. (But I might lack the in-depth knowledge to achieve this).

2. #69: We could maybe use the getValue() method and skip computeValue()? When reading the documentation it seemed this is the only method computed fields need to implement anyway? Not sure if this would break anything though.

   This would also be more coherent with the way computed values are fetched on Typed data level (see TextProcessed), and maybe serve integrations with Views etc too. This is implemented in this patch. Let's see what the testbot says.

3. #67.2 Let's convert this simply to:
   $this->setValue($values);

   Agreed and implemented.

4. #67.2 I think we should put the computeValue method into the FieldItemListInterface instead and in the implementation just check if the field definition is computed and only in this case compute the values, otherwise do nothing.

   OK. If we don't have a 'computed' field item list interface anymore, we need to check for the isComputed flag. I've added a method to the TypedData class.

5. #83 I haven't had the chance to look at this (but interdiff has taken it into account)

So it turns out that ModerationStateFieldItemList implements the computed field item list all along, correctly as it seems ,without having to patch anything. For it to work it seems to have to overwrite the following methods: get(), getIterator(), setValue(), onChange().

Maybe the solution is as simple as to create a abstract base class ComputedFieldItemListBase that helps setting up these methods, so the implementation is less cumbersome for developers.

I feel like we're getting a bit distracted by all the comments, and we need to go back to the issue summary:

Writing tests covering the following scenario

• Base field, manually computed.
• Base field, manually computed, configurable.
• Field type, annotated as computed, configurable.

Added to that: updating https://www.drupal.org/docs/8/api/entity-api/dynamicvirtual-field-values... (that got me distracted too).

So it turns out that ModerationStateFieldItemList implements the computed field item list all along .... Maybe the solution is as simple as to create a abstract base class ComputedFieldItemListBase that helps setting up these methods, so the implementation is less cumbersome for developers.

+1 Nice find, I guess there should also be one for entity references with a referencedEntities method.

I proposed #88 in #2857301: Make the process of creating computed fields less bug prone.

Thanks @Sam152 for the hint.

I've practically copy pasted your solution (added the validation I mentioned in #74) and ran the tests and they all pass. Hooray!

Not sure if I'm the person to do so, but I've added you to the to-be-credited list.

So we're getting closer: What's left now is

- Adding documentation to base class and tests.
- Writing some tests for configurable base fields and field types.

I don't think I can add the credit. Not sure what the policy is, a maintainer will have to look at it.

A lot of the coding standard fixes done in #84 are reverted again. You might want to take a look at that., Besides that, looks good!

One thing though, to generate $this->list, you need to add TypedData and do something like a createItem to create the TypedData from your values. We might want to add a wrapper for that as well. Developers just return an array of values in, let's say, a computedValues() method. The computeFieldItemListValues() takes the data and creates TypedData from that with something like:

      foreach ($this->computedValues() as $index => $value) {
        $this->list[$index] = $this->createItem($index, $value);
      }

This might make it a little easier for developers?

#93: coding standards: I'll have a look at it again when we have a patch ready for reviewing. thanks!
#93: wrapper: Agreed, as a matter of fact I'm working on that right now.

I'm going through a bunch of scenarios of fully computed and partially computed new field types that are configurable, and computed field item lists that are computed and configurable. Trying to write up tests for all scenarios that I can think of.

1. About fully computed field types

I've added an example of a fully computed field type 'dice' based on the example by penyaskito. You can use the field configuration as a setting for the computed values.
Having a fully computed field type does however does create a series of problems, that we should handle in a follow up issue imho:

1. A fully computed field type will still create the table for the fields without additional columns for the value, as it requires ::schema() to return an empty array of columns.
   We should somehow detect if all field properties are computed and then not create the table at all.
2. There is at least one Field UI page that (logically) throws an exception when accessing a fully computed field type: admin/structure/types/manage/{entity-type}/fields/{field}/storage

2. About computed field item lists and entity edit forms:

It is possible to expose computed field item lists in an Entity form. The forms will be populated with the computed values. The user can fill in new values, but they will obviously not be saved.

Should we add the 'disabled' attribute or show a message to the form element form element. Or is this something we leave to other developers. In any case I think this should go a follow-up issue.

3. About the changes in the unit tests

I haven't further revised the changes in the unit tests proposed earlier, but left them there in the tests.

4. About the coding standards of #84

1. I think it's up to the developer to use lowerCamelCase or snake_case for variables, as long as there is consistency. See https://www.drupal.org/docs/develop/standards/coding-standards#naming
2. Can you tell me why we cannot use the @expectedException annotation?

The rest I've implemented the rest of the suggestions and ran everything through codesniffer.

I think it's up to the developer to use lowerCamelCase or snake_case for variables, as long as there is consistency.

In core there are exceptions, but I think snake case is used pretty consistently. So that would be a reason to change it. The coding standards were changed mostly to allow more freedom for contrib. Switching to lowerCamelCase in core is a separate discussion. I don't think this patch should deviate from the standard in core, and changing this should be a separate issue imho.

Can you tell me why we cannot use the @expectedException annotation?

Again, consistency. Symfony uses the annotation a lot. Core at the moment uses $this->setExpectedException(). Changing this to annotations should also be done consistently and is a separate issue.

@seanB OK, thanks for the feedback. I will incorporate this as soon as we get feedback from the core maintainers.

A new patch with the following improvements.

1. Better coding standards as suggested in #97
2. A revised and simplified base class.
3. Added the isEmpty() method that now always returns FALSE. This is necessary to force the computation to be executed. If not it might not always work, for example in this issue: https://www.drupal.org/node/2912116.

In addition to the methods above, we need to add the call to initValues to the getValues method.

Bumped into this in #2915398: The moderation_state field is not computed during the creation of a new entity translation..

Some thoughts on the progress of #102. Apologies if I've missed some context, the issue is quite long.

1. +++ b/core/lib/Drupal/Core/Field/ComputedFieldItemListBase.php
   @@ -0,0 +1,74 @@
   +  public function isEmpty() {
   +    return FALSE;
   +  }
   

   Why can we always make this assumption?

2. +++ b/core/lib/Drupal/Core/Field/ComputedFieldItemListBase.php
   @@ -0,0 +1,74 @@
   +          $errors = $this->validate()->getIterator();
   +          if ($errors->current()) {
   +            throw new FieldException($errors->current()->getMessage());
   +          }
   

   Is this how we handle validation for non computed fields? I don't think setting field values to something invalid throws an exception, so I don't think computing invalid values should either?

3. +++ b/core/modules/field/tests/modules/field_computed_test/src/Plugin/Field/FieldType/Multiplier.php
   @@ -0,0 +1,59 @@
   + * @FieldType(
   + *   id = "multiplier",
   + *   label = @Translation("Multiplier"),
   + *   description = @Translation("Computed multiplied integer based on a stored integer and a factor stored in the field configuration."),
   + *   category = @Translation("Number"),
   + *   default_widget = "number", default_formatter = "number_integer"
   + * )
   

   Is this now scope creep based on the new direction of the issue? We're only introducing a base class for computed fields, not properties right?

4. +++ b/core/modules/field/tests/src/Kernel/FieldComputedFieldItemListTest.php
   @@ -0,0 +1,184 @@
   +  /**
   +   * Test a valid computed 'timestamp' field.
   +   */
   ...
   +  /**
   +   * Test a valid computed entity reference field.
   +   */
   ...
   +  /**
   +   * Test that a non existing entity reference returns NULL.
   +   */
   

   My instinct is we can rely on field types as an abstraction and trust createItem will behave for different field types as long as we're calling it without our computed values. Seems a little over-tested here, but I might be missing something.

This issue is quite long indeed, but sadly no one actually took into consideration the path forward suggested by the entity field API maintainers years ago :(

Quoting @yched in #10:

The intent of this issue is to find the various entry points that we need to plug (offsetGet, count, isEmpty...), and see if we can shape that into a base class to help other implementations of "a computed FieldItemList that determines its actual values at runtime"

A part of that suggestion won't work, this can not be a base class (like the one implemented in the latest patches) because some field types provide their own field item list class (for example \Drupal\Core\Field\EntityReferenceFieldItemList), so a computed entity reference field can not extend both the existing field item list class and the computed one at the same time. This leaves us with a single choice: a computed item list trait :)

As for the entry points, there are a lot more than those discovered in this issue so far. I looked at the class hierarchy starting from \Drupal\Core\Field\FieldItemList and going up to its parents (\Drupal\Core\TypedData\Plugin\DataType\ItemList, \Drupal\Core\TypedData\TypedData, etc.) and this seems to be a complete list of entry points for a computed item list:

\Drupal\Core\TypedData\TypedDataInterface::getValue()
\Drupal\Core\TypedData\TypedDataInterface::setValue()
\Drupal\Core\TypedData\TypedDataInterface::getString()
\Drupal\Core\TypedData\ComplexDataInterface::get()
\Drupal\Core\TypedData\ComplexDataInterface::set()
\Drupal\Core\TypedData\ListInterface::appendItem()
\Drupal\Core\TypedData\ListInterface::removeItem()
\Drupal\Core\TypedData\ListInterface::isEmpty()
\Drupal\Core\TypedData\ListInterface::filter()
\Drupal\Core\TypedData\Plugin\DataType\ItemList::offsetExists()
\Drupal\Core\TypedData\Plugin\DataType\ItemList::getIterator()
\Drupal\Core\TypedData\Plugin\DataType\ItemList::count()

The criteria for adding a method to that list was: "does this method do something with $this->list?". If yes, we need to compute the values first.

Now that we have a complete list of entry points, we can go further and look at @fago's comment from #52:

Summarizing I think we should go step by step and reduce the scope of the first patch to a single base field, that's manually defined computed. Then, we can add another configurable, computed one. And finally, as last step the field type being computed.

He suggests that this issue should only be about implementing a computed base field, and I fully agree with that. The problem space is already very complicated without thinking about computed configurable fields and computed field types at the same time.

Thankfully, we now have a test computed base field in core since #2852067: Add support for rendering computed fields to the "field" views field handler, so we can simply use that for writing full test coverage for the trait that I'm proposing.

Finally, here's a patch that implements all of the above :)

I'm also reclassifying this as a major bug because the two non-testing computed base fields that we have in core right now, \Drupal\path\Plugin\Field\FieldType\PathFieldItemList and \Drupal\content_moderation\Plugin\Field\ModerationStateFieldItemList are receiving a galore of bug reports, most of them being major:

#2905524: The count() method used on PathFieldItemList behaves differently if isEmpty() has been called.
#2906600: FieldItemList::equals() doesn't work correctly for computed fields with custom storage
#2908674: When using get() method directly on PathItem the alias is not loaded
#2915398: The moderation_state field is not computed during the creation of a new entity translation.

Wow, great analysis. Looking at all the touch points with $this->list certainly beats bumping into bugs in each method one by one :-/

Side note, I wonder if there is any chance overriding __get and computing on reads of $this->list. Maybe that would make methods on sub-classes of FieldItemList work that we don't know about.

In any case, this looks way better than what we've got so far, so very excited about this.

1. +++ b/core/lib/Drupal/Core/TypedData/ComputedItemListTrait.php
   @@ -0,0 +1,143 @@
   +/**
   + * Trait ComputedItemListTrait
   + */
   

   Need some better docs here.

2. +++ b/core/lib/Drupal/Core/TypedData/ComputedItemListTrait.php
   @@ -0,0 +1,143 @@
   +   * Whether the computed values have been already calculated or not.
   

   We should standardise on computed/calculated, do they mean the same thing? "Whether the values have already been computed or not." maybe?

3. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityFieldTest.php
   @@ -739,13 +739,100 @@ public function testComputedProperties() {
   +   * @group failing
   

   Is this left over?

Looking a bit at the previous patches, there was one thing that should be done here: don't create an empty item automatically for computed fields anymore. This also revealed a bug in the test coverage.

Edit: cross-post with #106, I'll address that in the next comment.

@Sam152, whoa, that was fast :)

Side note, I wonder if there is any chance overriding __get and computing on reads of $this->list. Maybe that would methods on sub-classes of FieldItemList that we don't know about.

__get() calls through first(), which in turns calls get(), so we should be fine. All subclasses of FieldItemList that are computed will get everything for free when they start using the trait.

1. Already fixed in #107.
2. Right, let's standardize on 'computed'.
3. Yes, I always forget to remove that..

This is looking really good. I think the issue summary needs to be rejigged if we're just addressing computing FieldItems in this issue and not properties. Huge +1 generally, this would have prevent a lot of bugs in content_moderation.

@Sam152, the issue summary only mentions computed properties as something that already "makes sense" in HEAD, as opposed to field item lists which have an undefined behavior when they are computed.

I also posted two patches for updating \Drupal\path\Plugin\Field\FieldType\PathFieldItemList and ModerationStateFieldItemList to use the trait added here, which will bring two real-world use cases for it and will allow us to see if there are any hidden bugs before promoting it as the "official" way of implementing computed fields.

#2915398-11: The moderation_state field is not computed during the creation of a new entity translation.
#2916300: Use ComputedFieldItemListTrait for the path field type

The test failures from #108 show that we can not remove the code that creates an empty item automatically for computed fields because that will be an API break for fields that are not using the new trait.

@amateescu The analysis in #105 is absolutely spot on and really helpful. I was looking at the patch yesterday and came to the same conclusion that we cannot simply remove the creation of the empty item on computed fields. For the rest it looks very good to me.

I like the fact that the trait is on typed data level. However this issue is about facilitating computed field items. Can we assume this works on typed data level? Should we either move the trait to the field module or add tests on typed data level?

❤️

@nuez, I think the integration tests from the patch are far more useful than some unit test of the trait at the typed data level. For example, it's a lot easier to test that setting a value and then calling a getter does not trigger a re-computation of the values.

Now, both of the followups have green patches: #2916300-8: Use ComputedFieldItemListTrait for the path field type and #2915398-16: The moderation_state field is not computed during the creation of a new entity translation., and, along with the computed test field that we extend here, we have enough proofs that we finally nailed the behavior of computed fields :)

Wrote a change record to describe in detail what module developers need to do in order to implement a computed field properly: https://www.drupal.org/node/2916667

Will wait for any other reviewers who might want to have a look at this, but +1 RTBC.

+1 RTBC from me too. Good work!

Awesome work, let's go!

Patch looks good. We need a change notice for this. Just one question.

+++ b/core/lib/Drupal/Core/TypedData/ComputedItemListTrait.php
@@ -0,0 +1,148 @@
+      $this->computeValue();

Should we add an abstract method and introduce a new abstract class for computed fields?

Change record is here: https://www.drupal.org/node/2916667

The abstract method is on the trait. #105 describes:

this can not be a base class (like the one implemented in the latest patches) because some field types provide their own field item list class

+++ b/core/lib/Drupal/Core/TypedData/ComputedItemListTrait.php
@@ -0,0 +1,148 @@
+  public function applyDefaultValue($notify = TRUE) {
+    // Default values do not make sense for computed fields. However, this
+    // method can be overridden if needed.
+    return;

should we be calling the parent here?

Thanks, sorry for the noise for change record. Can we create an interface for all trait functions and also add computeValue function to the interface? Something like this:

class ComputedTestFieldItemList extends FieldItemList impelements ComputedFieldItemListInterface {
  use ComputedItemListTrait;
   /**
    * Compute the list property from state.
    */
   protected function computeValue() {
     foreach (\Drupal::state()->get('entity_test_computed_field_item_list_value', []) as $delta => $item) {
       $this->list[$delta] = $this->createItem($delta, $item);
     }
   }
}

From the issue summary:

In short, the two "computed" operate at different levels:
- computed field means : don't store anything, I'll tell you which Items exist at runtime

Does that not void the current meaning of FieldStorageDefinitionInterface::hasCustomStorage()? That's a problem to fix in #2401117: Ensure computed field definitions always have custom storage, but I thought I'd raise it here before code goes in that makes that issue a nightmare to solve :)

Awesome patch by the way!

@jibran computeValue is protected, no need for an interface.

The rest of the methods already exist on FieldItemListInterface right?

Right! so

interface ComputedFieldItemListInterface {
    public function computeValue();
}

will be just fine. Just for the record symfony has same pattern with ContainerAwareInterface and ContainerAwareTrait.

computeValue is protected - I don't think you need an interface for protected methods?

Thanks @yched for chiming in! As always, you asked all the right question :)

Re #126.1:

Sadly, instanceof does not work on traits, only on classes and interfaces. And since we're not introducing an interface here, we can not use that suggestion. However, there is something we can do in the trait itself: don't call the parent get() anymore and just inline its code, which means that computed fields that implement the trait will not have an empty item added automatically, but still leave this behavior for "old" computed fields.

#126.2:

Loved this question! Your reason for being worried is still true in HEAD, not because of an isEmpty() call (although that was very close), but because of applyDefaultValue() which is called when creating an entity by \Drupal\Core\Entity\ContentEntityStorageBase::initFieldValues(), which has this code that you will probably remember very well:

      // Create one field item and give it a chance to apply its defaults.
      // Remove it if this ended up doing nothing.
      // @todo Having to create an item in case it wants to set a value is
      // absurd. Remove that in https://www.drupal.org/node/2356623.
      $item = $this->first() ?: $this->appendItem();
      $item->applyDefaultValue(FALSE);
      $this->filterEmptyItems();

This is actually the reason for the trait implementation of applyDefaultValue() which simply returns without calling the parent. This is also the answer to #124 :)

However, after writing tests for this remark, I realized that what I wrote above is not enough because values are also being computed just before saving the entity by \Drupal\Core\Field\FieldItemList::preSave() which calls filterEmptyItems() which will trigger the computation through the filter() method.

So we have to implement filterEmptyItems() in our trait, which is a bit sad because this is a method provided by FieldItemListInterface, which in turn means our trait can not stay at the typed data level anymore. Oh well.. it was worth a try :)

@jibran, the computation of the field's values is something internal to the field implementation, so it should not be publicly available to the outside code. This means the computeValue() needs to stay protected, which means no interface. This is also in line with @fago's thoughts from #52.1.

The test-only patch does not contain the trait's filterEmptyItems() method, which will show how the added test coverage fails for the expectation that the values are never computed if no code interacts with the computed field.

@Wim Leers seems to be having some trouble with FieldItemList::equals() for computed fields in #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior, so let's make sure that we have some explicit test coverage for that here.

Thanks for the explanation @larowlan and @amateescu!

#126 + #131: This is such an excellent, thorough discussion! 👏

#133: ❤️

Re #136:

filterEmptyItems() : wondering if this should rather be plugged at the level of the more generic \Drupal\Core\TypedData\Plugin\DataType\ItemList::filter() helper : it probably needs that same fix, and that could allow the trait to remain at the TypedData\ItemList level ?

Yes! That makes perfect sense. Somehow I missed that the more generic filter() doesn't need to compute the values for the following reasons:

- when a computed field is instantiated (i.e. when an entity is created), the field is empty so there's nothing to filter
- when some code interacts with a computed field through a getter or a setter, the values will be computed so filter() will do its job

Side note though : if the trait is at the TypedData\ItemList, then I guess strictly speaking its code comments can't mention "fields" and "entities" ? possible meh :-)

Meh indeed, but fixed anyway :)

Nit : computeValue() / doComputeValue()

Yup, I had that distinction in mind when I wrote the methods, but I preferred to have computedValues() as the "API" that computed fields need to override to calculate their values. Anyway, I like the suggestion to use ensureComputedValue() for the safeguard method so consider it done.

nit : something wrong around 'when there the field" ;-)

I blame that on working late into the night.

Also fixed the 'lifecycle' ubernit ;)

(yeah I know, I haven't changed much it seems :-p)

And I hope you never do :D

This blocks #2916300: Use ComputedFieldItemListTrait for the path field type, which blocks #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior.

AFAICT #137 addressed @yched's feedback in #136.

I tried to find even a single supernitpick, but failed. I almost wrote s/lifecycle/life cycle/, but turns out even Microsoft says "lifecycle", so screw macOS' built-in dictionary for saying otherwise.

+++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityFieldTest.php
@@ -739,13 +739,135 @@ public function testComputedProperties() {
+    // Check that the values are not computed unnecessarily during the lifecycle
+    // of an entity when the field is not interacted with directly.
...
+    $this->assertSame(0, \Drupal::state()->get('computed_test_field_execution', 0));
...
+    $this->assertSame(0, \Drupal::state()->get('computed_test_field_execution', 0));
...
+    $this->assertSame(0, \Drupal::state()->get('computed_test_field_execution', 0));
...
+    // Check that the values are only computed once.
+    $this->assertSame(1, \Drupal::state()->get('computed_test_field_execution', 0));
...
+    // Check that the values have not been computed when they were explicitly
+    // set.
+    $this->assertSame(0, \Drupal::state()->get('computed_test_field_execution', 0));

Just want to say, major kudos for this super clear, super valuable test coverage!

Loving this patch.

Maybe a small thing to note: Can we put a todo in the codebase along with this patch that reminds us of the BC we couldn't get around (see #114)? Might be cool to fix this for D9.

@kristiaanvandeneynde, that's a good point, added the usual deprecation message and a @trigger_error as well.

Also, sorry that I forgot to answer your earlier comment in #127. I did look at that issue and it is not impacted by this patch :)

In other words, I don't get what triggers the computation you noticed during FieldItemList::preSave() ?

This is a brown paper bag over my head situation :P It was the patch itself which was triggering the computation in filter(), as can be seen in the patch from #133 :) So, of course, you're right that we should simply remove the filter() override.

As for removing the "$index == 0 && !isset($this->list[0])" part of the condition, I wouldn't do that at all (not even in a followup) because who knows what custom code we might break.

RTBC+1 as well. Thanks for taking the time to adjust the patch to meet all of the recent comments' remarks!

Re-reviewed #143, +1 RTBC.

This unblocks several bug fixes and improvements, would be great to get this in.

Quoting @amateescu from #2543726-206: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration

The actual bug is that, just like computed fields, custom storage fields need to behave like regular fields and return the correct value(s) when requested via API calls

I think we need an issue title update because this trait can be used for custom storage fields as shown in #2921048: Introduce temporary work-around to make $term->parent work in REST API + JSON API in Drupal 8.5 — 8.6 ships with root cause fix.

+++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/ItemList.php
@@ -98,7 +98,10 @@ public function get($index) {
     }
     // Automatically create the first item for computed fields.
+    // @deprecated in Drupal 8.4.x, will be removed before Drupal 9.0.0.
+    // Use \Drupal\Core\TypedData\ComputedItemListTrait instead.
     if ($index == 0 && !isset($this->list[0]) && $this->definition->isComputed()) {
+      @trigger_error('Automatically creating the first item for computed fields is deprecated in Drupal 8.4.x and will be removed before Drupal 9.0.0. Use \Drupal\Core\TypedData\ComputedItemListTrait instead.', E_USER_DEPRECATED);
       $this->list[0] = $this->createItem(0);

Shouldn't this be in a separate patch? I'd love to get rid of the behaviour but doesn't look necessary. Also the deprecation message is going to need to be 8.5.0 now.

@catch, would you like to have separate patches for 8.4.x and 8.5.x? I agree that the deprecation could be 8.5.x only, but the trait provided by this patch is needed in 8.4.x because there are many bug reports that depend on it..

Sorry I mis-typed, I meant putting the deprecation in a separate issue - it looks like scope creep here a bit.

However if this is targeted fro 8.4.x too should we also make the Trait @internal in 8.4.x? - then no-one can accuse us of adding a new API in a patch release.

+++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/ItemList.php
@@ -98,7 +98,10 @@ public function get($index) {
     }
     // Automatically create the first item for computed fields.
+    // @deprecated in Drupal 8.4.x, will be removed before Drupal 9.0.0.
+    // Use \Drupal\Core\TypedData\ComputedItemListTrait instead.
     if ($index == 0 && !isset($this->list[0]) && $this->definition->isComputed()) {
+      @trigger_error('Automatically creating the first item for computed fields is deprecated in Drupal 8.4.x and will be removed before Drupal 9.0.0. Use \Drupal\Core\TypedData\ComputedItemListTrait instead.', E_USER_DEPRECATED);
       $this->list[0] = $this->createItem(0);
     }

Patch overall looks great, I really like the naming.

Only thing I'm not 100% sure about is deprecating this now, here, with a trigger_error(). This does seem to be triggered fairly often, removing the @ and going to node/add/artcile gives me 6 deprecated messages all from the applyDefaultValue() and 11 when saving. *

The thing is that @trigger_error() is fairly expensive, because what it does is jump to the error handler, which has to load errors.inc where error_reporting() returns 0 (this is what @ actually does) and then we go back.

Haven't done any actual benchmarks, I guess in the end it's not that much compared to all the crazy things we do in typed data when creating a new entity, but still wondering if we want to postpone the actual call (we could have it commented out or something) until we have a solution for our own calls and know that we actually *can* deprecate and eventually remove it.

* On my test site, thd calls on save have a number of different sources, 2x creating a new entity (the entity form re-creation and metatag), comment form, token_node_insert().

Did some more debugging, and I think it's fine, didn't fully understand the implications and that it is really only called for computed fields. Can't see the error handling show up in profiling and the 6 calls are from 3 fields, path (which will be committed soon after this), menu_link from token.module (which is a workaround until we have something like that in core to have menu link tokens during node save for pathauto, and we could also use this trait in 8.5, if we don't simply get into core directly) and the metatag field, which also does re-triggers the whole entity create.

As we'll simply remove the whole block there, even without doing anything about #2356623: Remove usages of FieldItem::applyDefaultValue() specifically, all performance implications will be resolved.

So, definitely +1, lets get it in :)

@catch, IMO the deprecation is actually in the scope of this issue, so how about this:

- commit a patch to 8.4.x with the new trait marked as @internal and without the deprecation
- commit the patch as-is to 8.5.x with the deprecation message updated to mention 8.5.x

Yes, +1 for #153, makes sense. Thank you all for your great work on this! :)

+1 too and nice way of dealing with the 8.4.x vs 8.5.x differences.

Re-testing, expecting this to fail now because of #2870194: Ensure that process-isolated tests can use Symfony's PHPunit bridge to catch usages of deprecated code if I understand that correctly?

Yes that's another reason I wanted the deprecation in a separate issue but we can update the phpunit deprecation exclusion list on the assumption it fails if it really needs to be done here.

TBH I don't see the point of marking the trait as internal; the point of not backporting additions is to avoid method collisions and the like. @internal is just a documentation convention; it is not API fairy dust that somehow changes the scope of the addition. ;) And if we mark things that are clearly intended as public API as @internal, all that does is confuse the issue of what our API actually is.

So if this is needed for multiple bugfixes, let's backport it, scope the issues as @catch suggested, inform developers with the CR, and evaluate the individual disruptions in the followup issues?

Thanks!

So the reason for me for marking it @internal is because we're not using it in core yet. If we find something unexpected, that gives us room to update things before 8.5.x, if we're 100% happy after a couple of conversions, we can remove @internal in a later patch release of 8.4.x.

I think the API addition thing goes beyond just collisions... as far as I understand (may be assuming wrongly here), semver patch releases are supposed to be forward compatible, so a module compatible with 8.4.3 should be compatible (bugs aside) with all 8.4.* versions. If we add this to 8.4.3 without a special designation, then contrib authors may use it and mistakenly declare >=8.4 compatibility when in fact they need >=8.4.3.

Though I agree that having @internal in there without explanation is confusing. It seems to say "this is not part of the API", but what we actually mean is "this will become part of the API, but it doesn't reliably exist in 8.4.*, so if you use it in contrib you need to require a particular patch release." Maybe just specifically state that it is added in 8.4.3 (possibly along with the @internal tag, to draw attention to it).

[crosspost with @catch]

#160 makes sense also.

Maybe we can just commit the 8.5.x version for now, and consider a backport once we want to backport something that would depend on it? There's only a few more weeks in which it'd matter since 8.4.4 will be the last normal patch release of 8.4.x. (The February window is criticals only as per https://www.drupal.org/core/release-cycle-overview#current-development-c....)