Ensure computed field definitions always have custom storage

I had this issue with my computed fields and it works like a charm for me. Thanks so much, plach.

Done #4, return true for isComputed() only if all the properties are computed.

Oops, I did the opposite. Now for real, return true for isComputed() only if all the properties are computed, false otherwise.

Testbot, please?

Issue priority: Major because this affects any developer/site builder using a computed field. Not critical because an unused schema does not break the site.

If you return an empty array in your schema, the site will break without the patch. Leaving at major because as a workaround you can define a column in the schema even if it is unused and the site won't break.

I guess we cannot use unit tests as the typed data manager is not injected.

  /**
   * Helper to retrieve the field item class.
   */
  protected function getFieldItemClass() {
    $type_definition = \Drupal::typedDataManager()
      ->getDefinition('field_item:' . $this->getType());
    return $type_definition['class'];
  }

A test module which provides a computed field, and a test that 1) test that the computed field value is correct, 2) test that we can add a computed field instance to the user entity.

Removing Needs tests tag

I'm on the global sprint weekend and I'm going to work on that issue. I'm with penyaskito at the same room.

Attending comment 15, I have done all except point 3.

@luismagr When @plach said DataReferenceDefinition, probably he meant DataDefinition.

Ok, I have done again the interdiff and the patch.

Thanks @luismagr for working on this!

@plach:

#15.3: for now it is necessary. If you think so, we may change that in the context of this issue or in a new one, but if you don't have the columns array the site breaks.

#15.4: I would expect then that overriding getValue() in the Item class will suffice, but test results don't agree. Should we add the class back or are we doing anything wrong? Maybe I misunderstood.

Changing getValue() to return array('value' => '42') gives the same failure.

Waiting for @yched feedback, but IMHO having a different class as #12 has is "cleaner". For now, AFAIK the only computed field in core is ShortcutItem, and it is quite special (because it extends from StringItem). Having a clean implementation at one test can be helpful for pointing people looking for an example.

So, yeah, computed fields...

We've had a running discussion with @fago in the past couple months about what those mean exactly. "Computed property" is fairly clear, but "Computed field" is much more blurry. Current code isn't really clear about what happens or should happen when a *field definition* uses "setComputed(TRUE)", and no use case was really formalized so far. The fact that the only core example (ShortcutItem) is kinda weird doesn't really help.

We opened #2392845: Add a trait to standardize handling of computed item lists about that, but didn't update it with our latest discussions so far :-/. I just did.

First conclusion over there is : "a computed field" is not the same than "a field with computed properties", which directly invalidates this patch here :-)

I'm wondering if having the storage ignore "isComputed" and rely solely on "hasCustomStorage" is the right approach.

Maybe it should account for isComputed in itself ?
- If a property is computed, do not try to save it (and maybe also do not try to create a column - not sure, the field type schema() is not supposed to expose a column for a computed property)
- If a field is computed, then ignore it completely (no schema, no column, save nothing)

No definite opinion here, just wondering. As explained above, computed fields are still largely in design :-)

Maybe trying to imagine what a Mongo storage would need to do about computed fields and properties would help figure out more generally if and how "entity storages" should reason about computed fields and properties ?

Yes, possibly. But that's merely a question, just wondering.

A Mongo storage would definitely have to reason on computed *properties* (since it has no schema to exclude the property from being stored). Would it also have to reason on computed *fields*, or would it just treat them the same as "fields with custom storage" ?

#2392845: Add a trait to standardize handling of computed item lists is in now.

I think you meant #2392845: Add a trait to standardize handling of computed item lists :)

Queued test, back to NR!

Ugh, that is the issue @jibran linked to, but in #35 it's still showing the old title, which is how I got confused :P Sorry!

I applied the patch and tried to create a computed field as the one from the test. Then I tried to add a new field from UI but that is throwing an exception. This is because Drupal is still creating the storage tables. The field storage tables have only the standard columns (bundle, deleted, entity_id, revision_id, langcode, delta).

On the other hand, I see that now we have a 'custom_storage' property in FieldStorageConfig but I wonder how we set this on field type, so that when creating a new FieldStorageConfig this is read from the field type and saved in the config entity.

The exception:

The website encountered an unexpected error. Please try again later.

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'LIMIT 1 OFFSET 0' at line 1: SELECT 1 AS expression
FROM 
{user__field_test} t
WHERE 
LIMIT 1 OFFSET 0; Array
(
)
 in Drupal\Core\Entity\Sql\SqlContentEntityStorage->countFieldData() (line 1714 of core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php).

Drupal\Core\Database\Statement->execute(Array, Array) (Line: 625)
Drupal\Core\Database\Connection->query('SELECT 1 AS expression
FROM 
{user__field_test} t
WHERE 
LIMIT 1 OFFSET 0', Array, Array) (Line: 87)
Drupal\Core\Database\Driver\mysql\Connection->query('SELECT 1 AS expression
FROM 
{user__field_test} t
WHERE 
LIMIT 1 OFFSET 0', Array, Array) (Line: 510)
Drupal\Core\Database\Query\Select->execute() (Line: 1714)
Drupal\Core\Entity\Sql\SqlContentEntityStorage->countFieldData(Object, 1) (Line: 728)
Drupal\field\Entity\FieldStorageConfig->hasData() (Line: 69)
Drupal\field_ui\Form\FieldStorageConfigEditForm->form(Array, Object) (Line: 117)
Drupal\Core\Entity\EntityForm->buildForm(Array, Object) (Line: 54)
Drupal\field_ui\Form\FieldStorageConfigEditForm->buildForm(Array, Object, 'user.user.field_asdasdasd')
call_user_func_array(Array, Array) (Line: 514)
Drupal\Core\Form\FormBuilder->retrieveForm('field_storage_config_edit_form', Object) (Line: 271)
Drupal\Core\Form\FormBuilder->buildForm('field_storage_config_edit_form', Object) (Line: 74)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 620)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
call_user_func_array(Object, Array) (Line: 153)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 68)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 99)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 78)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 50)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 657)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

I opened an issue to discuss if the "computed field" concept makes sense for configurable fields: #2932273: Figure out how to implement configurable computed fields

I bypassed the error in #44(without having applied the patch) using a quick hack on countFieldData()

    $storage_definitions = $this->entityManager->getFieldStorageDefinitions($this->entityTypeId);
    if (!$storage_definition->getColumns()) {
      return $as_bool ? FALSE : 0;
    }

This is bad DX since developers not familiar with all the details of the entity field storage can easily miss this point.

Looking at this issue from the field system for a few hours and I'm still not the wiser on the right way to workaround it....

It was this comment on Drupal\Core\Field\FieldItemInterface::schema() that suggested to me that nothing else would be required.

  /**
  * Returns the schema for the field.
  * 
  * Computed fields having no schema should return an empty array.
  *
  * @return array
  *   An empty array if there is no schema, or an associative array with the
  *   following key/value pairs:
  */
 public static function schema(FieldStorageDefinitionInterface $field_definition);

With the patch from #27, maybe extend this with

  * Computed fields having no schema should return an empty array. You should
  * also ensure that any property definitions for computed fields are set to computed.
  * @code
  *   public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
  *    $properties['value'] = DataDefinition::create('string')
  *      ->setComputed(TRUE)
  *      ->setLabel(t('Value'));
  *    return $properties;
  *  }
  * @endcode

And trivial mute point, shouldn't TestComputedFieldItem::schema() happily work as per the doc comment? i.e. no columns array.

+  public static function schema(FieldStorageDefinitionInterface $field_definition) {
+    return [];
+  }

My first reaction for this issue was, yes, totally makes sense. Today i stumbled across an example in the wild that uses a computed field that must live in the DB to suit views: https://github.com/Gizra/og/blob/8.x-1.x/src/Plugin/Field/FieldType/OgGr...

As OTOH all computed fields living out there i have seen set their custom_storage, i'd say this is a wontfix or at most documentation issue.

I think we should close this in favor of #2986836: Support computed bundle fields by adding the notion of computed field storage definitions. I think the notion of having custom storage is a different one than the notion of computed. And we should avoid mixing two concepts which are already complicated on their own.

#51 sounds sensible to me.

> And we should avoid mixing two concepts which are already complicated on their own.

A wholehearted sigh on this too.

That said, +1 for #51.

OK, 2 +1's is enough for me. If there is strong pushback we can always re-open.