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