Spin-off from #2164601: Stop auto-creating FieldItems on mere reading of $entity->field[N]

Problem

The notion of "computed property" is pretty clear - typically, $field->field_ref->entity.
This is defined at the level of a property definition, for example in FieldItemInterface::propertyDefinitions() :

$properties['entity'] = DataReferenceDefinition::create('entity')
      ->setComputed(TRUE);
This means the property value will never be stored, and is lazy-computed on request by some custom code in the corresponding Data class.

The notion of "computed field" is still quite shaky.
It can be defined at the level of a field definition, for example in FieldableEntityInterface::baseFieldDefinitions() :

$field['foo'] = BaseFieldDefinition::create('string')
      ->setComputed(TRUE);

But the actual effect of that is quite unclear at the moment. The only sure thing is that storage will never try to store the field values.

Proposal

This is the current state of our discussions with @fago in Ghent. For clarity, this tries to expose the concepts of "computed properties" (pretty much OK in HEAD currently) and "computed field" (still largely TBD) side by side.

Computed property

Sample case: EntityReferenceItem->entity
Behavior: don't save or load the value of that property when saving/loading the items, the value is populated on read at runtime.

The existence of a computed property is not field-by-field, it is determined by the field type, in the propertyDefinition() method of its Item class. It necessarily relies on the property definition providing a custom property class that implements the "compute the value" logic.

Also, the schema() method will likely omit to declare a schema column for that property, so nothing can be stored for that property anyway (well, at least in SQL - schemaless storage probably require some code to remove the value before storing the entity).

Computed field

Sample case: a "back reference" field (a field that lists all the entities that have an entity_ref field pointing to that entity)
Behavior : don't save or load *anything*, we don't know whether there are items and how many, that will be determined on read at runtime.

--> In other words, the "computed" thing here is the FieldItemList.

This is field-by-field, specified by the individual field definitions, and is not tied to the field type. You can define a computed field of any existing field type. That necessarily relies on the field definition providing a custom FieldItemList class, that implements the "compute the existing items" logic.
This raises the question : how can a configurable field be made "computed", since the only way to specify that custom List class currently is in runtime code ? A back_ref field would typically be manually added by site admins, and would thus need to be configurable...

Also, the field type can happen to have computed properties, that's orthogonal. For example, a back_reference field could be a field of type entity_ref, with a computed ->entity property in each item that loads the referencing entity (that's for illustration's sake, in reality it would probably have more specific needs and be a separate field type that extends entity_ref).

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
- computed property means : in each item that exist (however they happen to exist, stored or computed), don't store that property value, I'll tell you the value at runtime.

Remaining tasks

Create test-passing patches implementing progressively more complete computed fields:

  • Base field, manually computed.
  • Base field, manually computed, configurable.
  • Field type, annotated as computed, configurable.
CommentFileSizeAuthor
#166 interdiff-8.5.x.txt1.5 KBamateescu
#166 2392845-166-8.5.x.patch14.6 KBamateescu
#164 interdiff-8.4.x.txt499 bytesamateescu
#164 2392845-164-8.4.x.patch12.34 KBamateescu
#164 interdiff-8.5.x.txt1.31 KBamateescu
#164 2392845-164-8.5.x.patch14.59 KBamateescu
#153 interdiff-8.4.x.txt1.5 KBamateescu
#153 2392845-153-8.4.x.patch12.22 KBamateescu
#153 interdiff-8.5.x.txt1.38 KBamateescu
#153 2392845-153-8.5.x.patch13.28 KBamateescu
#143 interdiff-143.txt860 bytesamateescu
#143 2392845-143.patch13.28 KBamateescu
#141 interdiff-141.txt1.08 KBamateescu
#141 2392845-141.patch13.65 KBamateescu
#137 interdiff.txt7.82 KBamateescu
#137 2392845-137.patch12.57 KBamateescu
#133 interdiff.txt1.15 KBamateescu
#133 2392845-133.patch12.79 KBamateescu
#131 interdiff.txt6.38 KBamateescu
#131 2392845-131.patch12.19 KBamateescu
#131 2392845-131-test-only.patch11.82 KBamateescu
#114 interdiff.txt769 bytesamateescu
#114 2392845-114.patch9.74 KBamateescu
#108 interdiff.txt1.48 KBamateescu
#108 2392845-108.patch10.49 KBamateescu
#107 interdiff.txt2.9 KBamateescu
#107 2392845-106.patch10.55 KBamateescu
#105 2392845-105.patch9.65 KBamateescu
#102 interdiff-2392845-96-102.txt11.78 KBnuez
#102 clarify_the_notion_of-2392845-102.patch26.72 KBnuez
#98 interdiff-2392845-95-96.txt609 bytesnuez
#98 2392845-computed-fields-96.patch27.69 KBnuez
#95 interdiff-2392845-91-95.txt28.88 KBnuez
#95 2392845-computed-fields-95.patch28.44 KBnuez
#91 interdiff-2392845-86-91.txt9.37 KBnuez
#91 2392845-computed-fields-91.patch17.66 KBnuez
#86 interdiff-2392845-84-86.txt15.46 KBnuez
#86 2392845-computed-fields-86.patch23.7 KBnuez
#84 interdiff-83-84.txt6.54 KBseanB
#84 2392845-computed-fields-84.patch19.75 KBseanB
#83 interdiff-81-83.txt1.6 KBseanB
#83 2392845-computed-fields-83.patch19.6 KBseanB
#81 interdiff-79-81.txt777 bytesseanB
#81 2392845-computed-fields-81.patch18.66 KBseanB
#79 interdiff-77-79.txt3.29 KBseanB
#79 2392845-computed-fields-79.patch19.42 KBseanB
#77 interdiff-2392845-69-77.txt16.73 KBnuez
#77 2392845-computed-fields-77.patch18.88 KBnuez
#74 2392845-computed-fields-74.patch18.53 KBnuez
#74 2392845-computed-fields-66-with-tests.patch15.57 KBnuez
#74 2392845-computed-fields-66.patch11.45 KBnuez
#69 interdiff-66-69.txt1.23 KBseanB
#69 2392845-computed-fields-69.patch11.29 KBseanB
#66 interdiff-62-66.txt5.28 KBseanB
#66 2392845-computed-fields-66.patch11.45 KBseanB
#63 interdiff-55-62.txt8.91 KBseanB
#63 2392845-computed-fields-62.patch12.24 KBseanB
#55 2392845-computed-fields-55.patch16.64 KBsteveoliver
#46 2392845-computed-fields-46.patch16.49 KBvprocessor
#42 2392845-41.patch21.08 KBlokapujya
#38 2392845-computed-fields-38.patch21.05 KBtim.plunkett
#32 interdiff.txt12.16 KBAlumei
#32 2392845-computed-fields-32.no-test.patch21.19 KBAlumei
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2392845-computed-fields-32.no-test.patch. Unable to apply patch. See the log in the details link for more information. View
#31 interdiff.txt2.07 KBAlumei
#31 2392845-computed-fields-31.no-test.patch16.43 KBAlumei
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,357 pass(es). View
#25 interdiff.txt1.31 KBAlumei
#25 2392845-computed-fields-25.no-test.patch16.64 KBAlumei
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#22 interdiff.txt2.95 KBAlumei
#22 2392845-computed-fields-22.no-test.patch15.33 KBAlumei
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,353 pass(es), 2 fail(s), and 0 exception(s). View
#16 interdiff.txt9.13 KBAlumei
#16 2392845-computed-fields-16.no-test.patch13.05 KBAlumei
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,490 pass(es), 17 fail(s), and 0 exception(s). View
#11 interdiff.txt4.63 KBAlumei
#11 2392845-computed-fields-11.no-test.patch6.84 KBAlumei
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,533 pass(es). View
#9 2392845-computed-fields-9.no-test.patch3.73 KBpenyaskito
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,221 pass(es). View
#6 interdiff.txt917 bytespenyaskito
#6 field-computed-6.no-test.patch3.94 KBpenyaskito
#5 field-computed-5.no-test.patch3.9 KBpenyaskito
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

yched’s picture

Bump for #2401117: Ensure computed field definitions always have custom storage.

I updated the IS with the current state of our discussions with @fago in Ghent.

yched’s picture

Issue summary: View changes
yched’s picture

@penyastiko asked how he could help.

I think one way to move forward would be to try writing a sample "computed FieldItemList" class for a simple case - keeping the "entity back reference" case in mind, but simpler (without actual db queries to fetch the results).

Could be something like: a FieldItemListIntegerRange class for a computed integer field, with values = range from 1 to "the value of some other integer field in the entity" (and empty / no value if the controlling field is empty or 0). For simplicity, the name of that controlling field can be hardcoded in the FieldItemListIntegerRange.

yched’s picture

In terms of code, the first change that comes to mind would be that ItemList::get() needs to stop auto-creating an item for computed fields :

  public function get($index) {
    if (!is_numeric($index)) {
      throw new \InvalidArgumentException('Unable to get a value with a non-numeric delta in a list.');
    }
    // Automatically create the first item for computed fields.
    if ($index == 0 && !isset($this->list[0]) && $this->definition->isComputed()) {
      $this->list[0] = $this->createItem(0);
    }
    return isset($this->list[$index]) ? $this->list[$index] : NULL;
  }

If you're a computed field, we don't know in advance if your ItemList has Items, and how many. This is entirely up to the custom FieldItemList class that does the computation.

This also spills to any method that tries to determine if the field is empty, or to count its items...

penyaskito’s picture

Demo field.

penyaskito’s picture

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

penyaskito’s picture

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.

yched’s picture

Hey, I didn't expect you to be so quick :-) @penyaskito++ !

Having a quick look at it (sunday, not supposed to be in front of my keyboard :-p)

- We should remove the "dice" field type (DiceFieldItem). The idea is that you can define a "computed field" of any existing field type (in our case here, integer). Its' the field *definition* that says:
I'm an integer field : new BFD('integer'),
I'm computed : ->setComputed(TRUE),
and my "computed" logic lies in this custom ItemList class : ->setClass($class_name) (not really obvious, but this one talks about the *list* class)

- Then we can remove the custom formatter - that's the whole idea : a computed field just has custom logic to determine the values at runtime, other than that it's a regular field that can use the regular formatters for its field type.

- Once we get that working for base fields, we'll have to figure out how this can work for configurable fields... The current field_config / field_storage_config entities have no entries (and no UI...) for "I'm computed" / "This is my custom list class".

- Also, there is no randomness here, so we should rename DiceFieldItemList to RangeFieldItemList :-)

penyaskito’s picture

Status: Active » Needs review
FileSize
3.73 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,221 pass(es). View

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.

yched’s picture

The field is not shown, still have to figure out why

This is the crux of the issue here, we need to create the Items at some point. 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"

Alumei’s picture

FileSize
6.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,533 pass(es). View
4.63 KB

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?

penyaskito’s picture

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

Alumei’s picture

Assigned: Unassigned » Alumei

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 ;-)

Alumei’s picture

Assigned: Alumei » Unassigned

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.

Alumei’s picture

FileSize
13.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,490 pass(es), 17 fail(s), and 0 exception(s). View
9.13 KB

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.

Alumei’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 16: 2392845-computed-fields-16.no-test.patch, failed testing.

The last submitted patch, 16: 2392845-computed-fields-16.no-test.patch, failed testing.

Alumei’s picture

FileSize
15.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,353 pass(es), 2 fail(s), and 0 exception(s). View
2.95 KB

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

Alumei’s picture

Status: Needs work » Needs review

For testbot ...

Status: Needs review » Needs work

The last submitted patch, 22: 2392845-computed-fields-22.no-test.patch, failed testing.

Alumei’s picture

FileSize
16.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
1.31 KB

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.

Alumei’s picture

Status: Needs work » Needs review

It's getting late -_-
Gn8 @ testbot

Status: Needs review » Needs work

The last submitted patch, 25: 2392845-computed-fields-25.no-test.patch, failed testing.

The last submitted patch, 25: 2392845-computed-fields-25.no-test.patch, failed testing.

yched’s picture

Thanks a lot @Alumei, and sorry for the lack of feedback, busy days with client work :-/
This is one of the things I plan to focus on at Dev Days next week.

Alumei’s picture

Status: Needs work » Needs review
FileSize
16.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,357 pass(es). View
2.07 KB

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.

Alumei’s picture

FileSize
21.19 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2392845-computed-fields-32.no-test.patch. Unable to apply patch. See the log in the details link for more information. View
12.16 KB

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.

Alumei’s picture

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)
Alumei’s picture

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
Alumei’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 32: 2392845-computed-fields-32.no-test.patch, failed testing.

tim.plunkett’s picture

Attempted reroll.

Status: Needs review » Needs work

The last submitted patch, 38: 2392845-computed-fields-38.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

The last submitted patch, 38: 2392845-computed-fields-38.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
21.08 KB

reroll

Status: Needs review » Needs work

The last submitted patch, 42: 2392845-41.patch, failed testing.

guy_schneerson’s picture

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?

joachim’s picture

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 or pseudofield showing an entity's group(s).

vprocessor’s picture

vprocessor’s picture

Status: Needs work » Needs review
vprocessor’s picture

Assigned: Unassigned » vprocessor
vprocessor’s picture

Assigned: vprocessor » Unassigned

Status: Needs review » Needs work

The last submitted patch, 46: 2392845-computed-fields-46.patch, failed testing.

vprocessor’s picture

Assigned: Unassigned » vprocessor
fago’s picture

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.

vprocessor’s picture

Assigned: vprocessor » Unassigned

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

steveoliver’s picture

Assigned: Unassigned » steveoliver
Issue summary: View changes
Status: Needs work » Active
Issue tags: -Ghent DA sprint
FileSize
16.64 KB

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.

steveoliver’s picture

Status: Active » Needs review

Let's see what testbot has to say.

Status: Needs review » Needs work

The last submitted patch, 55: 2392845-computed-fields-55.patch, failed testing.

steveoliver’s picture

Assigned: steveoliver » Unassigned

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

Wim Leers’s picture

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.

Wim Leers’s picture

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

seanB’s picture

Status: Needs work » Needs review

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
seanB’s picture

seanB’s picture

Version: 8.3.x-dev » 8.4.x-dev
seanB’s picture

Status: Needs review » Needs work

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.

seanB’s picture

Status: Needs work » Needs review
FileSize
11.45 KB
5.28 KB

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

hchonov’s picture

  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.

Wim Leers’s picture

Thanks so much for pushing this forward again!

seanB’s picture

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

hchonov’s picture

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

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.

pwaterz’s picture

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.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

rjacobs’s picture

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.

nuez’s picture

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

Status: Needs review » Needs work

The last submitted patch, 74: 2392845-computed-fields-74.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

seanB’s picture

Could you also add interdiffs for easier review?

nuez’s picture

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

nuez’s picture

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.

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

seanB’s picture

Status: Needs work » Needs review
FileSize
19.42 KB
3.29 KB

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

Status: Needs review » Needs work

The last submitted patch, 79: 2392845-computed-fields-79.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

seanB’s picture

Status: Needs work » Needs review
FileSize
18.66 KB
777 bytes

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

Status: Needs review » Needs work

The last submitted patch, 81: 2392845-computed-fields-81.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

seanB’s picture

Status: Needs work » Needs review
FileSize
19.6 KB
1.6 KB

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.

seanB’s picture

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

Status: Needs review » Needs work

The last submitted patch, 84: 2392845-computed-fields-84.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

nuez’s picture

Status: Needs work » Needs review
FileSize
23.7 KB
15.46 KB
  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)

Status: Needs review » Needs work

The last submitted patch, 86: 2392845-computed-fields-86.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

nuez’s picture

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

seanB’s picture

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.

Sam152’s picture

nuez’s picture

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.

nuez’s picture

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

seanB’s picture

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?

nuez’s picture

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

nuez’s picture

Status: Needs work » Needs review
FileSize
28.44 KB
28.88 KB

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.

Status: Needs review » Needs work

The last submitted patch, 95: 2392845-computed-fields-95.patch, failed testing. View results

seanB’s picture

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.

nuez’s picture

Status: Needs work » Needs review
FileSize
27.69 KB
609 bytes
nuez’s picture

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

nuez’s picture

nuez’s picture

nuez’s picture

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.
Sam152’s picture

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

Sam152’s picture

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.

amateescu’s picture

Version: 8.5.x-dev » 8.4.x-dev
Category: Task » Bug report
FileSize
9.65 KB

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.

Sam152’s picture

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?

amateescu’s picture

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.

amateescu’s picture

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

The last submitted patch, 105: 2392845-105.patch, failed testing. View results

The last submitted patch, 107: 2392845-106.patch, failed testing. View results

Sam152’s picture

Issue summary: View changes

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.

amateescu’s picture

@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

Status: Needs review » Needs work

The last submitted patch, 108: 2392845-108.patch, failed testing. View results

amateescu’s picture

Status: Needs work » Needs review
FileSize
9.74 KB
769 bytes

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.

nuez’s picture

@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?

Wim Leers’s picture

❤️

amateescu’s picture

@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 :)

amateescu’s picture

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

Sam152’s picture

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

nuez’s picture

+1 RTBC from me too. Good work!

seanB’s picture

Status: Needs review » Reviewed & tested by the community

Awesome work, let's go!

jibran’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs change record

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?

Sam152’s picture

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

larowlan’s picture

+++ 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?

jibran’s picture

Issue tags: -Needs change record

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);
     }
   }
}
yched’s picture

Super super thrilled that the issue has come to this point ! I was kind of hoping for something like this, but was not sure it would be doable. Thanks a ton for pushing this folks :-)

Two remarks :

  1. About the "// Automatically create the first item for computed fields." code in ItemList::get:
    Sad, those lines have always made me cringe ;-), but I get the BC argument in #114 that it needs to stay to preserve behavior for existing ItemList classes not using the new trait.
    Maybe then it would deserve adjusting the comment to clarify that this code now only because of "old computed fields not using the trait"
    ?
    Also, maybe the condition could move from "if (isComputed())" to "if (isComputed() && !instanceof ComputedItemListTrait)" ?
  2. I think I remember being worried earlier in the D8 cycle about some code where we were doing an isEmpty() check on all the fields of an entity pretty early on the lifecycle of a loaded entity (thus pretty much nullifying the lazy-computation benefits since isEmpty() has to trigger the computation).
    That was quite some time ago and this code might have gone since then, but wondering if we should check that "regular operations" (for example : just loading an entity, just saving it, just displaying it in a view mode / form mode that doesn't involve the computed field) do not trigger the computation
    Maybe the tests added in the patch do account for that, in which case, never mind me :-)
kristiaanvandeneynde’s picture

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!

larowlan’s picture

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

The rest of the methods already exist on FieldItemListInterface right?

jibran’s picture

Right! so

interface ComputedFieldItemListInterface {
    public function computeValue();
}

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

larowlan’s picture

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

amateescu’s picture

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.

The last submitted patch, 131: 2392845-131-test-only.patch, failed testing. View results

amateescu’s picture

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

jibran’s picture

Thanks for the explanation @larowlan and @amateescu!

Wim Leers’s picture

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

#133: ❤️

yched’s picture

Re @amateescu #131 :

Re: Re: #126.1 :
❤️, much cleaner than an instanceof check

Re: Re: #126.2 :

applyDefaultValue() : HAH!, you bet I remember that code :-D. Pretty simple though, default values indeed don't make sense for computed fields, so +1 on overriding applyDefaultValue() to a no-op in the trait.

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 ?

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

  1. +++ b/core/lib/Drupal/Core/Field/ComputedFieldItemListTrait.php
    @@ -0,0 +1,176 @@
    +  protected function doComputeValue() {
    

    Nit : computeValue() / doComputeValue()

    The logic in other places where we have a pattern of X() / doX() is usually the other way around : X() being the outside facing API, holding some common logic and deferring to the internal, overridable doX().

    Maybe s/doComputeValue()/ensureComputedValue() would be more accurate ? All the places that currently guard with doComputeValue() precisely do not necessarily trigger a recompute, that's the nice thing :-)

  2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityFieldTest.php
    @@ -739,13 +739,134 @@ public function testComputedProperties() {
    +    // Check that the values are not computed unnecessarily during the life
    +    // cycle of an entity when there the field is not interacted with directly.
    

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

    ubernit while I'm in there : patch contains both wordings "life cycle" / "lifecycle". Not sure which is best, but we seem to have slightly more of the latter in current core ?
    (yeah I know, I haven't changed much it seems :-p)

amateescu’s picture

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

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

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!

kristiaanvandeneynde’s picture

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.

amateescu’s picture

@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 :)

yched’s picture

  1. +++ b/core/lib/Drupal/Core/TypedData/ComputedItemListTrait.php
    @@ -0,0 +1,165 @@
    +  public function filter($callback) {
    +    // If the values were never computed, for example when the item list has not
    +    // been interacted with during its lifecycle, there is no need to compute
    +    // them now.
    +    if ($this->valueComputed === FALSE) {
    +      return $this;
    +    }
    +
    +    return parent::filter($callback);
    +  }
    

    Looking at this again, I'm not sure I understand why the override is needed, actually.

    @amateescu in #131 :

    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

    But ItemList::filter() simply does array_filter($this->list, $some_callback). So, as you wrote in #137, in itself filter() should never trigger a computation, and is simply a no-op on a not-yet-computed field, where $this->list is []. So it doesn't seem we need an override to opt-out early when !$this->valueComputed ?

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

  2. +++ 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);
         }
    

    +1 on the trigger_error deprecation warning, wondering if it we should remove the "$index == 0 && !isset($this->list[0])" part of the condition, and warn whenever a computed field doesn't use the trait. As is, it seems the warning will trigger a bit randomly ? (well, not randomly, but only in specific cases).

    (probably not a blocker for the patch though)

amateescu’s picture

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.

yched’s picture

Heh, ok, that makes more sense :-)

RTBC+1, awesome to see this fixed !

kristiaanvandeneynde’s picture

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

Sam152’s picture

Re-reviewed #143, +1 RTBC.

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

jibran’s picture

catch’s picture

Version: 8.4.x-dev » 8.5.x-dev
+++ 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.

amateescu’s picture

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

catch’s picture

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.

Berdir’s picture

+++ 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().

Berdir’s picture

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

amateescu’s picture

@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

Anybody’s picture

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

kristiaanvandeneynde’s picture

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

Berdir’s picture

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?

catch’s picture

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.

xjm’s picture

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!

catch’s picture

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.

cburschka’s picture

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]

xjm’s picture

#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....)

The last submitted patch, 153: 2392845-153-8.5.x.patch, failed testing. View results

amateescu’s picture

One last try. The 8.5.x patch needs to account for #2392845: Add a trait to standardize handling of computed item lists and I added some more information to the @internal doc of the trait for the 8.4.x patch.

The last submitted patch, 164: 2392845-164-8.5.x.patch, failed testing. View results

amateescu’s picture

catch’s picture

Title: Clarify the notion of "computed field" » Add a trait to standardize handling of computed item lists
Version: 8.5.x-dev » 8.4.x-dev

Committed/pushed to both branches, really good to see this in.

  • catch committed f1adffe on 8.5.x
    Issue #2392845 by amateescu, nuez, seanB, Alumei, penyaskito, vprocessor...
catch’s picture

Status: Reviewed & tested by the community » Fixed

  • catch committed 87a7882 on 8.4.x
    Issue #2392845 by amateescu, nuez, seanB, Alumei, penyaskito, vprocessor...
andypost’s picture

CR published, thanx a lot for getting in

Wim Leers’s picture

yched’s picture

Awesome!

Status: Fixed » Closed (fixed)

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

Wim Leers’s picture