Problem/Motivation

As discussed for the beta blockers #2116363: Unified repository of field definitions (cache + API) and #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions we need to bring the concept of storage fields (aka "FieldConfig" of configurable fields) to the Entity Field API. Thus, marking as beta-blocker-blocker.

Proposed resolution

The plan is to introduce FieldStorageDefinitionInterface and have FieldDefinitionInterface extend it. Then FieldConfig should implement StorageFieldDefinitionInterface, but stop implementing FieldDefinitionInterface as it's lie. For the few situations where widgets need to use FieldConfig as fully-fledged fields we can provide a decorator class which contains the dumb-default implementations for remaining methods (name to be found). (Could be postponed to another issue if turns out be hard.)

Finally, the FieldConfig class should receive (yet another) rename to match the new interface name, i.e. become StorageFieldConfig. yched suggested and we agreed that FieldInstance should be FieldConfig then - but as this has high documentation and community term changes we decided to postpone this.

For providing field storage definitions, we need a new hook + getters on the Entity Manager along with hook_entity_field_info(). As there is no use case for providing field storage base fields we do not add a respective methods on entity class for now.

(Replaced the initially suggested term FieldStorageDefinition with StorageFieldDefinition, as suggested in #12)

Remaining tasks

Follow-up: #2228187: Rename FieldConfig to and FieldInstance
#2235675: Decide on moving field cardinality to the FieldDefinitionInterface

User interface changes

-

API changes

FieldItemInterface::propertyDefinitions() receives a definition object implementing StorageFieldDefinitionInterface instead of a FieldDefinitionInterface now
FieldItemInterface::schema() receives a definition object implementing StorageFieldDefinitionInterface instead of a FieldDefinitionInterface now

CommentFileSizeAuthor
#80 d8_field_storage_definition.interdiff.txt15.55 KBfago
#80 d8_field_storage_definition.patch96.86 KBfago
#75 field-storage-definition-2226197-75.patch94.86 KBjessebeach
#75 interdiff.txt67.38 KBjessebeach
#75 StorageFieldDefinitionInterface-to-FieldStorageDefinitionInterface.txt67.04 KBjessebeach
#68 Drupal-Fields-old.jpg80.34 KBjessebeach
#67 Drupal-Fields.jpg94.73 KBjessebeach
#57 d8_storage_field_definition.interdiff.txt1.98 KBfago
#57 d8_storage_field_definition.patch92.69 KBfago
#30 d8_field_storage_definition.interdiff.txt11.4 KBfago
#30 d8_field_storage_definition.patch92.51 KBfago
#27 getType-to-getTypeId-refactor.txt25.14 KBjessebeach
#27 interdiff.txt31.47 KBjessebeach
#27 d8-field-storage-definition-2226197-27.patch113.27 KBjessebeach
#21 d8_field_storage_definition.interdiff.txt672 bytesfago
#21 d8_field_storage_definition.patch86.62 KBfago
#18 d8_field_storage_definition.patch86.75 KBfago
#18 d8_field_storage_definition.interdiff.txt672 bytesfago
#16 d8_field_storage_definition.interdiff.txt4.62 KBfago
#16 d8_field_storage_definition-rename.interdiff.txt70.29 KBfago
#16 d8_field_storage_definition.patch86.75 KBfago
#13 d8_field_storage_definition.patch82.28 KBfago
#13 d8_field_storage_definition.interdiff.txt7.91 KBfago
#9 d8_field_storage_definition.patch76.84 KBfago
#8 d8_field_storage_definition.interdiff.txt9.41 KBfago
#5 d8_field_storage_definition-2226197-5.patch68.62 KBjessebeach
#5 interdiff.txt6.91 KBjessebeach
#1 d8_field_storage_definition.patch68.42 KBfago
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

Status: Active » Needs review
FileSize
68.42 KB

ok, so here is a first patch. Let's see where it fails.

Where I was not so sure was whether label() and description() should be in the storage field definition, but as Views is going to need it might make sense to do whatever blackmagic it does in the field config class, so I just left it there for now.

Status: Needs review » Needs work

The last submitted patch, 1: d8_field_storage_definition.patch, failed testing.

fago’s picture

Assigned: fago » Unassigned

oh no, #2136197: Move field/instance/widget/formatter settings out of annotation / plugin definition got committed. Now we've got conflicts in every field type :-/ Running out of time now, continuing later on. If someone else wants to pick up and help solving conflicts, please do! ;)

jessebeach’s picture

Sure, I'll reroll.

jessebeach’s picture

Status: Needs work » Needs review
FileSize
6.91 KB
68.62 KB

Changes due to reroll are in the interfdiff.txt.

Status: Needs review » Needs work

The last submitted patch, 5: d8_field_storage_definition-2226197-5.patch, failed testing.

fago’s picture

Great, thanks!!

fago’s picture

ok, worked more on it and fixed the test fails. It still misses exposing config fields via the hook and test coverage.

fago’s picture

Status: Needs work » Needs review
FileSize
76.84 KB
jessebeach’s picture

reviewing...

jessebeach’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -478,6 +478,62 @@ protected function buildBundleFieldDefinitions($entity_type_id, $bundle, array $
    +        foreach ($module_definitions as $field_name => $definition) {
    +          // @todo Remove this check one FieldDefinitionInterface exposes a
    +          //   proper provider setter. See https://drupal.org/node/2225961.
    +          if ($definition instanceof FieldDefinition) {
    +            $definition->setProvider($module);
    +          }
    +          $field_definitions[$field_name] = $definition;
    +        }
    

    Should we be checking for instance of FieldDefinitionInterface here?

  2. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
    @@ -68,6 +68,37 @@ public static function create($type) {
    +   * Creates a new field definition based upon a field storage definition.
    +   *
    +   * In cases where one needs a field storage definitions to act like full
    +   * field definitions, this creates a new field definition based upon the
    +   * (limited) information available. That way it is possible to use the field
    +   * definition in places where a full field definition is required; e.g., with
    +   * widgets or formatters.
    +   *
    +   * @param FieldStorageDefinitionInterface $definition
    +   *   The field storage definition to base the new field definition upon.
    +   *
    +   * @return $this
    +   */
    +  public static function createFromFieldStorageDefinition(FieldStorageDefinitionInterface $definition) {
    +    return static::create($definition->getType())
    +      ->setCardinality($definition->getCardinality())
    +      ->setConstraints($definition->getConstraints())
    +      ->setCustomStorage($definition->hasCustomStorage())
    +      ->setDescription($definition->getDescription())
    +      ->setLabel($definition->getLabel())
    +      ->setName($definition->getName())
    +      ->setProvider($definition->getProvider())
    +      ->setQueryable($definition->isQueryable())
    +      ->setRequired($definition->isRequired())
    +      ->setRevisionable($definition->isRevisionable())
    +      ->setSettings($definition->getSettings())
    +      ->setTargetEntityTypeId($definition->getTargetEntityTypeId())
    +      ->setTranslatable($definition->isTranslatable());
    +  }
    +
    +  /**
    

    Hmmm, this smells a little bit. Is this a backwards-compatibility method? Is there no way to iterate through the $definition and invoke getters rather than enumerating them like this?

  3. +++ b/core/lib/Drupal/Core/Field/FieldDefinitionInterface.php
    @@ -52,80 +52,7 @@
    -  /**
    -   * Returns the field settings.
    -   *
    -   * Each field type defines the settings that are meaningful for that type.
    -   * For example, a text field can define a 'max_length' setting, and an image
    -   * field can define a 'alt_field_required' setting.
    -   *
    -   * @return array
    -   *   An array of key/value pairs.
    -   */
    -  public function getSettings();
    

    I don't see where these methods went. To another interface?

  4. +++ b/core/lib/Drupal/Core/Field/FieldStorageDefinitionInterface.php
    @@ -0,0 +1,287 @@
    +  /**
    +   * Returns the field settings.
    +   *
    +   * Each field type defines the settings that are meaningful for that type.
    +   * For example, a text field can define a 'max_length' setting, and an image
    +   * field can define a 'alt_field_required' setting.
    +   *
    +   * @return array
    +   *   An array of key/value pairs.
    +   */
    +  public function getSettings();
    

    Ah, here are the methods. Can we use git mv here to effect a rename?

  5. +++ b/core/modules/field/lib/Drupal/field/Entity/FieldConfig.php
    @@ -182,13 +179,6 @@ class FieldConfig extends ConfigEntityBase implements FieldConfigInterface {
    @@ -535,8 +525,7 @@ public function getSettings() {
    
    @@ -535,8 +525,7 @@ public function getSettings() {
         $field_type_manager = \Drupal::service('plugin.manager.field.field_type');
     
         $settings = $field_type_manager->getDefaultSettings($this->type);
    -    $instance_settings = $field_type_manager->getDefaultInstanceSettings($this->type);
    -    return $this->settings + $settings + $instance_settings;
    +    return $this->settings + $settings;
       }
    

    Why have the instance settings been removed? Or have they been moved somewhere else?

  6. +++ b/core/modules/field/lib/Drupal/field/Plugin/views/field/Field.php
    @@ -40,9 +41,13 @@ class Field extends FieldPluginBase {
    +   * A field storage definition turned into a field definition, so it can be
    +   * used with widgets and formatters. See
    +   * FieldDefinition::createFromFieldStorageDefinition().
    

    What makes these things different?

fago’s picture

Assigned: Unassigned » fago

Thanks, here some comments:
ad 1) We cannot, see the cited todo :)
ad 2) No, as getters/setters do not follow an exact patter (isTranslatable() ...)
ad 4) I'll let Git try to detect the rename for the next re-roll (via -M)
ad 5) They got removed as the field storage (aka field config for configurable fields) does not have instance settings. Previously it was acting like a field definition/instance although it wasn't, so it had to add in the defaults as well. Now as these got cleaned up, it can be removed. The defaults will be added in as soon as a FieldDefinition is created via createFromFieldStorageDefinition() anyway.
ad 6) Views, operating on fields on the entity level has no knowledge of per-bundle field metadata. So for fields put on bundles it can only go with the FieldStorageDefinition being shared across all bundles. The storage field is fine for everything related to storage, but is not suited for the regular display/widget cases which expects to operate on full field definitions. That's why we've got to come up with field definitions based on the storage fields in that special case.

I've noted my patch mixes up the terms "StorageFieldDefinition" and "FieldStorageDefintion". I could see us going with StorageFieldDefinition instead, as it avoid potential confusing around FieldStorageDefintion being a plugin definition of a field storage plugin (which existed in d7).

Working on it again.

fago’s picture

Completed the EM implementation and added docs for it now. Still missing test coverage to ensure it works as it should.

jessebeach’s picture

I cannot figure out the code path to get Drupal\Core\Entity\EntityManager::getStorageFieldDefinitions() or Drupal\Core\Entity\EntityManager::buildStorageFieldDefinitions() to invoke.

Do you have a form or page your were working on while developing these methods that I can use as well?

jessebeach’s picture

Status: Needs review » Needs work

So, just talked with fago. The two methods I mentioned in #14 are not called. They are API additions that are part of a larger refactoring to unify field definitions: #2116363: Unified repository of field definitions (cache + API).

This patch just needs unit tests for the new methods.

fago’s picture

fago’s picture

Status: Needs work » Needs review

Finally managed to debug mocked object calls. So here is a complete patch with test coverage.

Also, renamed FieldStorageDefinitionInterface to StorageFieldDefinitionInterface as suggested in #12.

This should be ready then.

fago’s picture

Found a docu mistake.

fago’s picture

Issue summary: View changes
fago’s picture

Title: Introduce FieldStorageDefinitionInterface in the Entity Field API » Introduce StorageFieldDefinitionInterface in the Entity Field API
fago’s picture

Updated the documentation example.

fago’s picture

Issue summary: View changes
Xano’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -99,6 +99,13 @@ class EntityManager extends PluginManagerBase implements EntityManagerInterface
    +   * @var array
    

    If this is a complex array, it will need some more documentation to describe the array structure.

  2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -486,9 +493,68 @@ protected function buildBundleFieldDefinitions($entity_type_id, $bundle, array $
    +          // @todo Remove this check one FieldDefinitionInterface exposes a
    

    "once" instead of "one".

  1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -99,6 +99,13 @@ class EntityManager extends PluginManagerBase implements EntityManagerInterface
    +   * @var array
    

    Is this @var \Drupal\Core\Field\FieldStorageDefinitionInterface?

  2. +++ b/core/lib/Drupal/Core/Field/StorageFieldDefinitionInterface.php
    @@ -0,0 +1,288 @@
    +  public function getType();
    

    It doesn't return the actual type (a plugin instance), but an ID, so maybe getTypeId()?

  3. +++ b/core/lib/Drupal/Core/Field/StorageFieldDefinitionInterface.php
    @@ -0,0 +1,288 @@
    +   * @return array
    +   *   An array of key/value pairs.
    

    @return mixed[].

  4. +++ b/core/lib/Drupal/Core/Field/StorageFieldDefinitionInterface.php
    @@ -0,0 +1,288 @@
    +   * @return integer
    

    int instead of integer.

  5. +++ b/core/lib/Drupal/Core/Field/StorageFieldDefinitionInterface.php
    @@ -0,0 +1,288 @@
    +   * @return array
    

    string[]

  6. +++ b/core/lib/Drupal/Core/Field/StorageFieldDefinitionInterface.php
    @@ -0,0 +1,288 @@
    +   * @return array
    

    array[]

  7. +++ b/core/lib/Drupal/Core/Field/StorageFieldDefinitionInterface.php
    @@ -0,0 +1,288 @@
    +   * @return array
    

    array[]

  8. +++ b/core/lib/Drupal/Core/Field/StorageFieldDefinitionInterface.php
    @@ -0,0 +1,288 @@
    +   * @return array
    +   *   An array of validation constraint definitions, keyed by constraint name.
    +   *   Each constraint definition can be used for instantiating
    +   *   \Symfony\Component\Validator\Constraint objects.
    

    array[], and the actual structure will need to be documented, or a reference will need to be added to the documentation elsewhere.

  9. +++ b/core/lib/Drupal/Core/Field/StorageFieldDefinitionInterface.php
    @@ -0,0 +1,288 @@
    +   * @return array
    

    Same here.

  10. +++ b/core/modules/system/entity.api.php
    @@ -756,6 +761,49 @@ function hook_entity_bundle_field_info_alter(&$fields, \Drupal\Core\Entity\Entit
    +    return $fields;
    

    Wrong code example.

  11. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
    @@ -476,6 +476,33 @@ public function testGetFieldDefinitions() {
    +      ->will($this->returnValue(array('storage_field' => $storage_field_definition)));
    

    Add a with() expectation to test that the right module is invoked?

plach’s picture

This patch is awesome! I think it provides the missing pieces to finally fix #2143291: Clarify handling of field translatability. I could not find much too complain about, aside from what @Xano already noted:

  1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -486,9 +493,68 @@ protected function buildBundleFieldDefinitions($entity_type_id, $bundle, array $
    +   *   \Drupal\Core\Entity\ContentEntityInterface are supported
    

    Missing trailing dot

  2. +++ b/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Plugin/Field/FieldType/HiddenTestItem.php
    @@ -26,8 +26,9 @@ class HiddenTestItem extends TestItem {
    +
    

    Bogus empty line

Berdir’s picture

  1. +++ b/core/modules/system/entity.api.php
    @@ -706,6 +706,9 @@ function hook_entity_base_field_info_alter(&$fields, \Drupal\Core\Entity\EntityT
      *
    + * Bundle fields either have to override an existing base field, or need to
    + * provide a storage field via hook_entity_storage_field_info().
    + *
    

    Based on other discussion, this should somehow mention the computed fields use case.

  2. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
    @@ -476,6 +476,33 @@ public function testGetFieldDefinitions() {
    +
    +    $this->moduleHandler->expects($this->any())
    +      ->method('invoke')
    +      ->will($this->returnValue(array('storage_field' => $storage_field_definition)));
    +
    

    Possibly include with('phpunit') here to verify that it is called with the correct argument? Or is that going to mess with the bas fields? But if it does, then we might be doing something unexpected internally anyway, so possibly use exact order for the mocking? phpunit is also a bit a strange module name, usually we use something like example_module or so, to make it clear what it is.

jessebeach’s picture

Assigned: fago » jessebeach

Assigning this to myself as I go through the feedback. Fago can have it back whenever he wants :)

jessebeach’s picture

Assigned: jessebeach » Unassigned
Status: Needs work » Needs review
FileSize
113.27 KB
31.47 KB
25.14 KB

Is this @var \Drupal\Core\Field\FieldStorageDefinitionInterface?

re:#1, it's an array, keyed by entity type ID. Each key contains an array of \Drupal\Core\Field\FieldDefinition. I annotated it like this. Not sure about my docs syntax.

/**
 * Static cache of storage field definitions per entity type.
 *
 * Elements of the array:
 *  - $entity_type_id: \Drupal\Core\Field\FieldDefinition[]
 *
 * @var array
 */
protected $storageFieldDefinitions;

Comments #3-#9 were extremely difficult to address because the code examples in the review contain no line context. I made best guesses. For example.

+++ b/core/lib/Drupal/Core/Field/StorageFieldDefinitionInterface.php
@@ -0,0 +1,288 @@
+   * @return array
string[]

The following comment does not have enough context or explanation. I cannot address it.

+++ b/core/modules/system/entity.api.php
@@ -756,6 +761,49 @@ function hook_entity_bundle_field_info_alter(&$fields, \Drupal\Core\Entity\Entit
+    return $fields;

Wrong code example.

This addresses all the comments in #23 except #11, which I will address with feedback from #25.

The refactor in #23-2 (the second 2) is included as a separate diff for easier scanning.

Status: Needs review » Needs work

The last submitted patch, 27: d8-field-storage-definition-2226197-27.patch, failed testing.

yched’s picture

Really not sure about getType() --> getTypeId(), the notion of "field type" is scattered all over the place, comments, docs...
At any rate, it's in no way related to the task here (and adds like 30% to the patch size), so if anything, it should really be a followup.

fago’s picture

Status: Needs work » Needs review
FileSize
92.51 KB
11.4 KB

It doesn't return the actual type (a plugin instance), but an ID, so maybe getTypeId()?

Not so sure about this either - howsoever this is a pre-existing method, this patch just moves it in a upper interface. Changing it to getTypeId() would be an unrelated API change, thus needs its own issue. I.e., reverted that part (thanks for the separated interdiff!).

Here is an updated patch which also addresses #24 and #25. Also, I improved to docs to clarify that values provided by SFDI (StorageFieldDefinitionInterface) may not changed/altered by the respective bundle fields. Then, I moved isRequired() to FDI as it's defined by field instance. Also moved the field cardinality to FDI, while I've kept isMultiple() in SFDI which is the only thing we need to know for storage.

yched’s picture

Also moved the field cardinality to FDI, while I've kept isMultiple() in SFDI which is the only thing we need to know for storage.

Not sure I get that part. How can cardinaility be assigned per instance/bundle if isMultiple() is per SFD ?
Cardinality is currently a field-level, cross-bundle property, changing that seems out of scope here (IIRC, this has an impact on Views)

andypost’s picture

Then it makes sense to file follow-up to convert cardinality into validation constraint for field item list

yched’s picture

@andypos: regardless of where cardinality lives, I thought we already had a constraint for that ?

fago’s picture

@andypos: regardless of where cardinality lives, I thought we already had a constraint for that ?

yes, we have.

Not sure I get that part. How can cardinaility be assigned per instance/bundle if isMultiple() is per SFD ?

The thinking was that to the storage it does not matter whether it may be exactly X items or not, to the storage only being multiple or not matters.

Cardinality is currently a field-level, cross-bundle property, changing that seems out of scope here (IIRC, this has an impact on Views)

It's not changed, as it stays that way for configurable fields. Not putting it on SFDI allows other implementations to do it differently though, what I could totally see being useful. How this implementation figures out to decide on isMultiple() for the storage field is up to them, but a developer should have no troubles deciding that.

Regarding Views I shortly checked that, and it seems to use it for providing options for the delta_limit. Not sure it's an issue to not have that, but as the Views integration is for config fields and lives in field.module it's fine to rely on FieldConfigInterface there anyway.

If it's problematic to put cardinality on FDI I'm happy to move it back. I just thought it makes more sense that way as to the storage it shouldn't matter but is more a validation constraint.

Related, I figured current storage respects the cardinality while loading/saving values to limit the loaded/saved values by the configured cardinality, what was quite a WTF to me. Validation should happen before save and storage should give me whatever is stored but do not silently filter values during load/save. So this is something that would have to go then - yes. @yched: Would you agree with that? Else it wouldn't make sense to keep it that way yeah.

yched’s picture

Validation should happen before save and storage should give me whatever is stored but do not silently filter values during load/save

Filtering before save is because D7 never ran validation on programmatic saves. Not fully sure whether we're completely confident all saves are validated now ?
Filtering on load is for the case where you change cardinality for a field from N to

re cardinality on SFD / FI : yeah, i'd rather discuss that in a dedicated issue and not sneak this in here :-)

Berdir’s picture

We are not, but it's the responsibility of the caller to call validate() if he receives input that needs to be checked. But that's tricky because for example migrate can not validate, as there's no way to deal with validation errors, it has troubles with mock entities and so on.

I thought we validate to not allow to set the cardinality to lower than what you have in the database, but I'm not sure anymore...

yched’s picture

I thought we validate to not allow to set the cardinality to lower than what
you have in the database, but I'm not sure anymore...

Not that I know of. I'm not even sure we'd be able to write that EFQ if we wanted to (find the max delta present in the DB for that field ?)

andypost’s picture

Suppose storage should be dumb for cardinality because that property could change in definitions.
It's up to validation to limit min|max values via constraint before save

fago’s picture

One can save data that does not validate - it shouldn't be the job of the storage to enforce validation passes?

Filtering on load is for the case where you change cardinality for a field from N to

So some values go automatically away? That sounds quite weird and violates our rule of keeping the users data intact.

Not sure efq can query like that either, but I think validating that when it is changed makes most sense also. As discussed over at #2002180: Entity forms skip validation of fields that are edited without widgets I think that's the general approach we have to take anyway.

yched’s picture

So some values go automatically away? That sounds quite weird and violates our rule of keeping the users data intact.

Well, that's what D7 does :-) Not sure how it violates user data more that allowing users to delete a field ? Both actions are initiated by the user ?
Aside from forbidding cardinality change (or at least forbidding cardinlity being lowered) once a field has data, I'm not sure how we can prevent that.

fago’s picture

Tested on D7 - it indeed silently dismisses additional values without telling you anything about it :-( At least the data is not deleted as long as no one edits the entity. But if someone then edits the entity, it deletes any previously entered additional values.

Aside from forbidding cardinality change (or at least forbidding cardinlity being lowered) once a field has data, I'm not sure how we can prevent that.

Yeah, imo we should do that. Sounds like something we could spawn as separate issue to help #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions?

tstoeckler’s picture

Regarding cardinality change we also need to prevent to change from 1 to more than one, as that has storage implications. Not for configurable fields, but for base fields.

andypost’s picture

Cardinality should be used to build a list of formatters and widgets, not sure it affects storage (that always multiple by default, from d7 as I remamber)

plach’s picture

Regarding cardinality change we also need to prevent to change from 1 to more than one, as that has storage implications. Not for configurable fields, but for base fields.

A change in cardinality for base fields is just one of the changes we will have to prevent, not sure whether we should add a special case for it here.

tstoeckler’s picture

Re #44: I think it is just the case of 1 -> many or many -> 1 that we have to prevent. I.e. if for some reason I want to restrict the 'roles' field of users to cardinality 10 (or whatever) that's not a schema change.

plach’s picture

Yes, single to multiple or viceversa (just for base fields obviously).

chx’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

I do not know what is a 'storage field'; it's not evident from the IS; the patch doesn't explain it either; quite probably the name is entirely wrong. Perhaps the field is stored in some way..? but it is not a field on the storage so storageField makes no sense.

yched’s picture

@chx: yes the current name is totally not ideal - having a name allows code to be written :-), but I really hope we can come up with something better in the end.

To clarify: the "thing" that this patch formalizes, and currently calls StorageFieldDefinition, is what D7 called $field (as opposed to $instance).
- It's unfortunate IMO to call it "storage definition". Only the EntityStorage defines a "storage".
- It is the definition of a named, abstract "data bucket" (mostly, a data schema : name, columns, is it multiple, translatable...), that some actual "fields" on some actual entity types and bundles will use to put their data.
The fact that two fields in different bundles use the same "bucket" (field name == bucket name) means that their data is in the same "place" and can be queried together: e.g. "fetch all nodes created today, whatever the node type").
It's only the EntityStorage that makes a real storage location (columns in an sql table, records in a mongo db...) out of that abstract "bucket definition".

[edit: to be extra clear, this is not a new thing conceptually, it's exactly the current split between $field and $instance. It's just that Entity Field API currently uses the single notion of "FieldDefinition" for both. This patch formalizes an official "API shape" (name & interface) for the former]

So yeah, name...
FieldDataBucketDefinition ? :-(

chx’s picture

I thought we used the phrase "configurable fields" as the name for the thing that was $field in Drupal 7...?

Berdir’s picture

We did, but the point of this issue is to have a common interface for configurable and base field "storage information".

chx’s picture

To clarify: the "thing" that this patch formalizes, and currently calls StorageFieldDefinition, is what D7 called $field

the point of this issue is to have a common interface for configurable and base field "storage information".

Huh?

yched’s picture

@chx: Trying again.

The $field / $instance conceptual split only exists for configurable fields currently, it's a field.module thing, Core/Entity and Core/Field don't know about it and see everything as "FieldDefinitions". That's a lie: a $field is *not* a complete FieldDefinition; you shouldn't create storage tables out of $instances; etc...

This issue is about moving this conceptual split up into the Core Field system.

fago’s picture

It's only the EntityStorage that makes a real storage location (columns in an sql table, records in a mongo db...) out of that abstract "bucket definition".

"storage field definition" means "bucket definition" to me. It's not the storage itself, but it defines the a field entity storage we'll have to take care of. So for me the name would work - but I'm happy to go with whatever makes most sense for people.

fago’s picture

Discussed naming a bit on IRC with msonnabaum, berdir and chx. msonnabaum suggested StorageMappingDefinition or StorageMapping what fits good the language of having ODM/ORMs behind. Thoughts?

chx’s picture

Mapping from what to what?

yched’s picture

- re: StorageFieldDefinition
I read that as : definition of a "storage field"
I kind of see how "storage field" could mean what I called "data bucket" ("abstract field in an abstract storage"),
but IMO it would still be an unfortunate clash with the notions of "base fields, bundle fields, configurable fields" we already have.

In "storage field", it's "field" as in "a database field" (general IT meaning)
In "base fields, bundle fields, configurable fields", it's "field" as in "entity field" (drupalism)

Or "storage field" = the entity (base-, configurable-...)field as seen by the entity storage layer ?
-> "those two entity fields use the same storage field"

The world "field" has always been used with different meaning in different APIs, and we've lived with that. But using it with different meanings in the same API (the *Field* API :-p) feels like a recipe for confusion :-/

- re:StorageMapping
Hmm. Interesting.
Feels a bit disconnected and lacking context though. As @chx points, it's a bit vague about what it maps to what.

fago’s picture

Status: Needs work » Needs review
FileSize
92.69 KB
1.98 KB

Or "storage field" = the entity (base-, configurable-...)field as seen by the entity storage layer ?
-> "those two entity fields use the same storage field"

That's how I'd see it.

The mapping from StorageMapping is about mapping the PHP object(s) of a field to the storage, i.e. the database columns. As said, the commonly used terms ORM or ODM use mapping like that - it's just no terminology we've been adopting so far. Although the entity api is our ORM/ODM :-)

I've re-rolled the patch and fixed conflicts and moved field cardinality to the SFDI as per our discussions. I'll open a follow-up for discussing moving it to FDI.

fago’s picture

jessebeach’s picture

+++ b/core/modules/field/field.module
@@ -174,6 +174,16 @@ function field_system_info_alter(&$info, Extension $file, $type) {
 /**
+ * Implements hook_entity_storage_field_info().
+ */
+function field_entity_storage_field_info(\Drupal\Core\Entity\EntityTypeInterface $entity_type) {
+  // Expose storage fields for all exposed bundle fields.
+  if ($entity_type->isFieldable()) {
+    return Field::fieldInfo()->getFields();
+  }
+}
+
+/**

Is it even necessary to check if the $entity_type is fieldable? We're getting a list of field "data buckets" that exist outside of any particular entity type, no? At least, nothing in buildStorageFieldDefinitions ever checks the fields against the particular $entity_type.

Berdir’s picture

It is necessary, because a) we want avoid calling those functions of non-fieldable (extendable) entity types and b) that really should be checking for a specific entity_type, looks like there's no public API anymore for that other than using the field map and then loading all fields for that?

jessebeach’s picture

  1. +++ b/core/lib/Drupal/Core/Field/StorageFieldDefinitionInterface.php
    @@ -0,0 +1,287 @@
    +   * Returns the maximum number of items allowed for the field.
    +   *
    +   * Possible values are positive integers or
    +   * FieldDefinitionInterface::CARDINALITY_UNLIMITED.
    +   *
    +   * @return int
    +   *   The field cardinality.
    +   */
    +  public function getCardinality();
    +
    

    StorageFieldDefinitionInterface?

    This is defined in StorageFieldDefinitionInterface even though most code will interact with the constant through a FieldDefinitionInterface object. In this instance, the code comment knows too much about an interface that implements this one.

    interface StorageFieldDefinitionInterface {
    
      /**
       * Value indicating a field accepts an unlimited number of values.
       */
      const CARDINALITY_UNLIMITED = -1;
    ...
    
  2. +++ b/core/lib/Drupal/Core/Field/StorageFieldDefinitionInterface.php
    @@ -0,0 +1,287 @@
    +  /**
    +   * Returns the ID of the type of the entity this field is attached to.
    +   *
    +   * This method should not be confused with EntityInterface::entityType()
    +   * (configurable fields are config entities, and thus implement both
    +   * interfaces):
    +   *   - FieldDefinitionInterface::getTargetEntityTypeId() answers "as a field,
    +   *     which entity type are you attached to?".
    +   *   - EntityInterface::getEntityTypeId() answers "as a (config) entity, what
    +   *     is your own entity type".
    +   *
    +   * @return string
    +   *   The name of the entity type.
    +   */
    +  public function getTargetEntityTypeId();
    

    StorageFieldDefinitionInterface?

  3. +++ b/core/lib/Drupal/Core/Field/StorageFieldDefinitionInterface.php
    @@ -0,0 +1,287 @@
    +  /**
    +   * Returns the field columns, as defined in the field schema.
    +   *
    +   * @return array[]
    +   *   The array of field columns, keyed by column name, in the same format
    +   *   returned by getSchema().
    +   *
    +   * @see \Drupal\Core\Field\FieldDefinitionInterface::getSchema()
    +   */
    +  public function getColumns();
    

    StorageFieldDefinitionInterface?

jessebeach’s picture

I've started the Change Notice here: https://drupal.org/node/2236285

chx’s picture

PersistableField?

tstoeckler’s picture

Issue tags: +Naming things is hard

So while I like the "persist" vocabulary, the problem is that - as far as I understand - this is not about some type of field, i.e. a persistable field vs. a computed field etc. but instead this is about that part of the definition that relates to the storage.

That said I wonder if "FieldStorageDefinition" is not a better term. The separation of "StorageFieldDefinition" and "FieldDefinition" sort of implies that there are two different fields, while that is not really true. I.e. there will be a "StorageFieldDefinition" and a "FieldDefinition" for the node.title field.

So, yeah, I would propose "FieldStorageDefinition" or per #63 even "FieldPersistanceDefinition".

fago’s picture

Not sure what persistence vs storage buys as here, to me both terms can be used synonymously. As we usually use "storage" I think we should stick with storage, except there is a reason that this is persistence and not storage?

I.e. there will be a "StorageFieldDefinition" and a "FieldDefinition" for the node.title field.

Yes, while for configurable fields there will be a "storage field" and "field definition", whereas still the field definition will have the *complete* definition, i.e. it includes storage information. Still, it's right that "storage field" implies this is a different flavor of fields along with "base field" and "bundle field" - what's not the case.

FieldStorageDefinition & $EM->getFieldStorageDefinitions() would work for me as well.

amateescu’s picture

How about going "back to basics" (and to what everyone is already used to from D7) and have FieldDefinition / FieldInstanceDefinition? That would mean we'd have to rename the current FieldDefinition from core to FieldInstanceDefinition and make this propsed StorageFieldDefinition be the new FieldDefinition.

IMO, this is what should've happened in #2114707: Allow per-bundle overrides of field definitions, but I had this idea too late and I didn't want to derail that issue.

jessebeach’s picture

FileSize
94.73 KB

Even thought fago switched from "FieldStorageDefintion" to "StorageFieldDefinition" in #12, I'd like to suggest we switch back. fago suggests the same in #65

FieldStorageDefinition & $EM->getFieldStorageDefinitions() would work for me as well.

Here is my reason why in addition. This is the current set of "Field" related interfaces.

  • FieldInstanceConfigInterface
  • FieldConfigInterface
  • FieldDefinitionInterface
  • StorageFieldDefinitionInterface

StorageFieldDefinitionInterface breaks the pattern. We are creating "field (storage definitions)", not "(storage field) definitions". We have a nice set of classes if we go back to FieldStorageDefintion

  • FieldInstanceConfigInterface
  • FieldConfigInterface
  • FieldDefinitionInterface
  • FieldStorageDefinitionInterface

re: #66

How about going "back to basics" (and to what everyone is already used to from D7) and have FieldDefinition / FieldInstanceDefinition? That would mean we'd have to rename the current FieldDefinition from core to FieldInstanceDefinition and make this propsed StorageFieldDefinition be the new FieldDefinition.

The concept of 'instance' is employed by configuration fields, but not by base fields, yet the FieldDefinitionInterface is used by both. Using FieldInstanceDefinitionInterface for base fields would be misapplied.

I had to draw this out because the associations were getting a little to multiple for my brain. This is the UML for the relationships proposed in this issue. I've used FieldStorargeDefinitionInterface in the diagram.

A UML diagram of the Field interfaces.

jessebeach’s picture

FileSize
80.34 KB

So, let's break this down. First, here is the UML that represents Drupal 8.x HEAD before this patch.

UML BEFORE

A UML diagram of Drupal field interfaces before the patch in this issue

UML AFTER

A UML diagram of the Field interfaces.

Let's look at some implications, before and after for classes.

CODE BEFORE

$field_definition instanceof FieldDefinitionInterface === TRUE
$field_instance_config instanceof FieldDefinitionInterface === TRUE
$field_config instanceof FieldDefinitionInterface === TRUE

CODE AFTER

$field_definition instanceof FieldDefinitionInterface === TRUE
$field_instance_config instanceof FieldDefinitionInterface === TRUE
$field_config instanceof FieldDefinitionInterface === FALSE

After the changes in this patch, field instance configuration objects equate with raw field definition objects, but not with a field configuration object.

amateescu’s picture

The concept of 'instance' is employed by configuration fields, but not by base fields, yet the FieldDefinitionInterface is used by both. Using FieldInstanceDefinitionInterface for base fields would be misapplied.

When FieldDefinitionInterface was introduced, base fields didn't have the concept of "per bundle" overrides. Now that we realised that we do need this feature after all, and since we already have the concept of fields and instances for configurable fields, how is it misapplied when base fields have the same need?

Maybe it helps if I say it this way: field and instance definitions would be the same concept for both base and configurable fields, the only difference would be that their properties are stored in code / the configuration system.

jessebeach’s picture

amateescu, here is a link to the document I used to create the UML diagrams.

http://www.lucidchart.com/invitations/accept/5346993d-eb4c-4d7d-b9c9-75e...

Can you add another version to it with your proposed name changes? I created a tab in the document called "amateescu's proposal"

yched’s picture

Of the options mentioned above, I still prefer the initial FieldStorageDefinition - although I still have the same reservations than expressed in #48.

Not really sure what would be wrong with FieldDataBucket, TBH. In the posts above as well as in various live / hangout discussions, the expression "data bucket" has consistently been the one that made people understand the concept best, so why introduce a different, more ambiguous one ?

jessebeach’s picture

I don't have any opposition to FieldDataBucket. I like it in fact. It neatly avoids the ambiguity of "Storage" which we already use for controllers in D8.

fago’s picture

The definitions have a relationship to storage, so having storage in there is no bad ambiguity to me. It establishes the relationship. FieldDataBucket sounds like it would be a separate storage for itself, but it isn't.

Maybe it helps if I say it this way: field and instance definitions would be the same concept for both base and configurable fields, the only difference would be that their properties are stored in code / the configuration system.

Yeah, but we do not have stand-a-lone storage fields (aka field_config) for non-configurable fields, so it's not exactly the same.

amateescu’s picture

Yeah, but we do not have stand-a-lone storage fields (aka field_config) for non-configurable fields, so it's not exactly the same.

Why would we need field_config objects for non-configurable fields?

Maybe it's related to #2144263: Decouple entity field storage from configurable fields? Or maybe I didn't understand what you said at all :)

jessebeach’s picture

This issue is blocking #2116363: Unified repository of field definitions (cache + API). We really need to hold our noses, make a best effort, get something in, and move on. Also, I have no great stake in this issue other than to get us moving and working on followups; the name doesn't really matter to me. Only core devs will have to live with it :)

So, to that end I've taken the various comments into account and changed StorageFieldDefinitionInterface to FieldStorageDefinitionInterface.

Justification:

#64

So, yeah, I would propose "FieldStorageDefinition" or per #63 even "FieldPersistanceDefinition".

#65

FieldStorageDefinition & $EM->getFieldStorageDefinitions() would work for me as well.

#71:

Of the options mentioned above, I still prefer the initial FieldStorageDefinition

#73

The definitions have a relationship to storage, so having storage in there is no bad ambiguity to me.

The interface changed from FieldStorageDefinitionInterface to StorageFieldDefinitionInterface in #12 with the following explanation:

I've noted my patch mixes up the terms "StorageFieldDefinition" and "FieldStorageDefintion". I could see us going with StorageFieldDefinition instead, as it avoid potential confusing around FieldStorageDefintion being a plugin definition of a field storage plugin (which existed in d7).

Naming the interface StorageField... sets up an anti-pattern in D8 whereby a "field" interface starts with the word "storage". I'd rather avoid inconsistencies within a major version than defend against name confusion across major versions.

I've included the refactor as its own diff. The only other change I made is to a docblock

/**
   * Returns the maximum number of items allowed for the field.
   *
   * Possible values are positive integers or
   * FieldDefinitionInterface::CARDINALITY_UNLIMITED.
   *
   * @return int
   *   The field cardinality.
   */
  public function getCardinality();

to

/**
   * Returns the maximum number of items allowed for the field.
   *
   * Possible values are positive integers or
   * FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED.
   *
   * @return int
   *   The field cardinality.
   */
  public function getCardinality();

The constant is declared in FieldStorageDefinitionInterface.

The test coverage for this new interface looks adequate. It's time to get this in and let postponed work continue, no?

n.b. amateescu, I'm really sorry, but I can't map your recommendation to the existing architecture. I mean mentally, I can't see how the concepts your recommending would fit together. So I'm pushing ahead here with what we've got. We really need to start working on the unified repository of field definitions in order to get the beta unblocked.

jessebeach’s picture

Title: Introduce StorageFieldDefinitionInterface in the Entity Field API » Introduce FieldStorageDefinitionInterface in the Entity Field API
chx’s picture

While I can't grasp the full width of the issue and UML diagrams make me run screaming (due to some childhood^Wuniversity trauma) FieldStorageDefintion definitely sounds more sane than StorageFieldDefintion.

tstoeckler’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -486,9 +496,74 @@ protected function buildBundleFieldDefinitions($entity_type_id, $bundle, array $
    +      // Add all non-computed base fields.
    +      foreach ($this->getBaseFieldDefinitions($entity_type_id) as $field_name => $definition) {
    +        if (!$definition->isComputed()) {
    +          $this->fieldStorageDefinitions[$entity_type_id][$field_name] = $definition;
    +        }
    +      }
    

    Is there a reason this is not happening in buildFieldStorageDefinitions()?

  2. +++ b/core/lib/Drupal/Core/Entity/EntityManagerInterface.php
    @@ -57,6 +57,28 @@ public function getBaseFieldDefinitions($entity_type_id);
    +   * This returns all storage field definitions for base fields and bundle
    +   * fields of an entity type. Note that storage field definitions of a base
    +   * fields equal the full base field definition (i.e. they implement
    +   * \Drupal\Core\Field\FieldDefinitionInterface), while the storage fields for
    +   * bundle fields may implement
    +   * \Drupal\Core\Field\FieldStorageDefinitionInterface only.
    

    Minor, but we don't have to use the fully-qualified namespace here, which would make this a bit easier to read.

  3. +++ b/core/lib/Drupal/Core/Entity/EntityManagerInterface.php
    @@ -57,6 +57,28 @@ public function getBaseFieldDefinitions($entity_type_id);
    +   *   The entity type ID. Only entity types that implement
    +   *   \Drupal\Core\Entity\ContentEntityInterface are supported.
    

    Wouldn't saying "Only content entities are supported be sufficiently clear?

  4. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
    @@ -68,6 +68,37 @@ public static function create($type) {
    +   * @param FieldStorageDefinitionInterface $definition
    

    Here we actually do need the fully-qualified namespace per our coding standards.

  5. +++ b/core/lib/Drupal/Core/Field/FieldStorageDefinitionInterface.php
    @@ -2,57 +2,29 @@
    + * FieldDefintionInterface), which is responsible for defining how the field is
    

    Typo: FieldDefin*i*tionInterface

  6. +++ b/core/modules/field/field.module
    @@ -174,6 +174,16 @@ function field_system_info_alter(&$info, Extension $file, $type) {
    +  if ($entity_type->isFieldable()) {
    +    return Field::fieldInfo()->getFields();
    +  }
    

    It is strange that $entity_type is not used in the return line. Shouldn't this be Field::fieldInfo()->getFields()[$entity_type->id()]?

  7. +++ b/core/modules/field/lib/Drupal/field/Entity/FieldConfig.php
    @@ -630,7 +620,7 @@ public function isRequired() {
    +    return ($cardinality == FieldDefinitionInterface::CARDINALITY_UNLIMITED) || ($cardinality > 1);
    

    It's inherited, so this works, but wouldn't it be more correct to use FieldStorageDefinition::CARDINALITY_UNLIMITED ?

Berdir’s picture

Status: Needs review » Needs work

Yes, 6. has already been pointed out above, The method does not have an entity_type argument, but we must filter it somehow.

As this is about *replacing* FieldInfo, I would suggest we do not use it at all but instead switch to an entity query that filters by entity_type or possible does a STARTS_WITH on the ID, because that's something we can optimize in the entity query implementation to be very fast.

fago’s picture

Status: Needs work » Needs review
FileSize
96.86 KB
15.55 KB

Thanks for the updates and review. I've continued from #75 and finalized the rename back to FSD - there were many occurrences in the documentation left. I hope I got all of them now.

ad #78:
1. Yes, buildFieldStorageDefinition() covers only the separately cached FSDs.
2,3. ok, changed it that way.
4,5,6,7 fixed - thanks

I've also updated field_entity_field_storage_info() as suggested in #79. Unfortunately, the previous version was broken as it did not filter by entity type. Sadly, the unit test did not cover that as this part was mocked - so I added an integration test for that to field.module.

Updated patch and interdiff attached.

jessebeach’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Thanks for finding the rest of the storage-field-to-field-storage word swaps. I hadn't thought to search for them as separate words. I've verified that there are no more relevant instances of "storage field" and its various permutations.

The integration test for the fix to field_entity_field_storage_info() looks sufficient.

This issue is ready to be committed. We've addressed all the review concerns and reached an acceptable consensus on the interface name. This will allow us to unpostpone two more beta blockers.

I've updated the change notice to reflect changes made in #75.

alexpott’s picture

Assigned: Unassigned » alexpott

Looking at this.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed acbd5fa and pushed to 8.x. Thanks!

  • Commit acbd5fa on 8.x by alexpott:
    Issue #2226197 by fago, jessebeach: Introduce...

Status: Fixed » Closed (fixed)

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

fago’s picture