We discussed terminology quite a bit during the entity storage discussion in Prague, see the notes here. Attendees of the discussion were plach (discussion lead), yched, fago, berdir, amateescu, das-peter.

This was discussed again in Szeged with fago, plach, Berdir and tstoeckler.

Problem/Motivation

There is no way to find out the ownership of entity fields. As some fields are provided by the entity type itself in the ::baseFieldDefinitions() and some are added in hook_entity_*_field_info() it is important to distinguish this. Also some fields should be taken care of by the storage controller but we also want to support adding fields and caring about the storage yourself.

Proposed resolution

We want to clarify the providing module of a field definition + who is responsible for storing the field. For this purpose the following methods are being added to FieldDefinitionInterface:

  • getProvider(): The provider of the entity field. For the base fields defined in the entity type the provide will be equal to the provider of the entity type. For fields added in hook_entity_*_field_info() the provider will be the module iplementing the hook. For configurable fields the provider will be 'field'. This allows to distinguish where the field originated. It will also enable to track dependencies, e.g. when a module is configured to rely on a field to be there (a rule configuration needs it to run, a module uses a token of that field, ..).
  • hasCustomStorage(): Returns whether or not the field has a custom storage and thus should be ignored by the storage controller. This is FALSE by default, and hardcoded to FALSE for configurable fields. This will be used by #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities.

Comments

fago’s picture

amateescu’s picture

Status:Postponed» Active

Looks like we can unpostpone this now.

About the plan: 'module' could be easily confused with the same key from D7 which meant the module that provided the field type (changed to 'provider' in D8), should we try to find some other name for it?

sun’s picture

  1. What is the difference between 'module' and 'provider' ?
  2. Off-hand, I do not like the NULL part of the proposal. The base fields are owned by the entity-declaring module, so that is their owner/provider, not some magical NULL provider. This can probably be automated.
plach’s picture

Working on this as it's a pre-requisite for #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities, which in turn is a pre-requisite for #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions :)

I like the suggestions from #2 and #3, but I will stick with the 'module' key for now to let @amateescu reply to @sun.

plach’s picture

Assigned:Unassigned» plach
xjm’s picture

plach’s picture

Status:Active» Needs review
StatusFileSize
new8.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

Initial work, let's see how it behaves. On test coverage now.

Status:Needs review» Needs work

The last submitted patch, 7: entity-field_keys-2143069-7.patch, failed testing.

amateescu’s picture

Re: #3

1) 'provider' is a property that's set automatically by plugin managers on all the plugins (including field types, I suppose), and, in this context, the meaning of 'module' is defined in the issue summary.
2) I kind of agree that a NULL value for base fields is not pretty at all.

How about reusing the 'configurable' TRUE / FALSE flag that just became available after #2191709: Remove the "configurable" flag on field types? And this could also be set automatically by the entity manager in the "field discovery" phase.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new1.43 KB
new8.73 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,971 pass(es), 48 fail(s), and 168 exception(s).
[ View ]

This should be better.

tstoeckler’s picture

Status:Needs review» Active

This is looking great. I have a few minor remarks.

  1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -328,11 +328,28 @@ protected function buildBaseFieldDefinitions($entity_type_id) {
    +    $provider = $entity_type->getProvider();
         $base_field_definitions = $class::baseFieldDefinitions($entity_type);
    +    foreach ($base_field_definitions as $definition) {
    +      $definition->setProvider($provider);
    +    }

    +100. This makes a huge amount of sense!

  2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -328,11 +328,28 @@ protected function buildBaseFieldDefinitions($entity_type_id) {
    +        // Ensure the module key actually matches the name of the module defining
    +        // the field.
    +        foreach ($module_definitions as $definition) {
    +          $definition->setProvider($provider);
    +        }

    This should be $module right?

  3. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -402,10 +419,29 @@ protected function buildBundleFieldDefinitions($entity_type_id, $bundle, array $
    +        // Ensure the module key actually matches the name of the module defining
    +        // the field.
    +        foreach ($module_definitions as $definition) {
    +          $definition->setProvider($provider);
    +        }

    See above.

  4. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
    @@ -151,6 +151,27 @@ public function setSetting($setting_name, $value) {
    +   * @return static

    @@ -453,4 +474,26 @@ public static function getReservedColumns() {
    +   * @return static

    Should be @return $this per our new standards.

  5. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
    @@ -453,4 +474,26 @@ public static function getReservedColumns() {
    +  public function isCustomStorage() {
    +    return !empty($this->definition['custom_storage']);
    +  }

    Hmm.. I think *has*CustomStorage might be a better name? Not sure though...

  6. +++ b/core/lib/Drupal/Core/Field/FieldDefinitionInterface.php
    @@ -336,4 +344,16 @@ public function getSchema();
    +   * This indicates whether the storage controller should take care of storing
    +   * the field values or whether the field module will do it.

    Suggestion:
    Indicates whether the *entity type's* storage controller should take care of storing the field values or whether *the module providing the field* will do that.

  7. +++ b/core/modules/field/lib/Drupal/field/Entity/FieldConfig.php
    @@ -576,6 +583,26 @@ public function setTranslatable($translatable) {
    +   * @return static
    +   *   The object itself for chaining.
    ...
    +  public function setProvider($provider) {
    +    // Do nothing: provider is readonly property.
    +  }

    Liar! :-)

tstoeckler’s picture

Status:Active» Needs review
plach’s picture

StatusFileSize
new4.09 KB
new9.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

Actually correcting stuff after copy/pasting usually helps...

Status:Needs review» Needs work

The last submitted patch, 13: entity-field_keys-2143069-11.patch, failed testing.

The last submitted patch, 10: entity-field_keys-2143069-10.patch, failed testing.

yched’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -328,11 +328,28 @@ protected function buildBaseFieldDefinitions($entity_type_id) {
    +    $provider = $entity_type->getProvider();
         $base_field_definitions = $class::baseFieldDefinitions($entity_type);
    +    foreach ($base_field_definitions as $definition) {
    +      $definition->setProvider($provider);
    +    }

    Nitpick, code organization:
    Collect $baseFieldDefinition, then define $provider and assign it to each definition. Also, the code block could use a comment, just like the subsequent steps have their own comment.

  2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -328,11 +328,28 @@ protected function buildBaseFieldDefinitions($entity_type_id) {
    +      $module_definitions = call_user_func($module . '_' . $hook, $entity_type);

    Could we use moduleHandler->invoke() rather than a c_u_f() ?
    Friendlier with PHPstorm's "find hook invocations" Drupal integration feature)

    (for the same reason, better to avoid having the hook name in a var)

  3. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -328,11 +328,28 @@ protected function buildBaseFieldDefinitions($entity_type_id) {
    +      if (isset($module_definitions) && is_array($module_definitions)) {

    Why isset() + is_array() ? The hook is supposed to return an array, so just the foreach should be enough ?

  4. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -328,11 +328,28 @@ protected function buildBaseFieldDefinitions($entity_type_id) {
    +        // Ensure the module key actually matches the name of the module defining
    +        // the field.

    80 char error

  5. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -328,11 +328,28 @@ protected function buildBaseFieldDefinitions($entity_type_id) {
    +        $return = NestedArray::mergeDeep($base_field_module_definitions, $module_definitions);

    - Not sure why we need a mergeDeep() rather than a flat array merge here, but well, seems like it's what we have currently
    - var $return is misnamed + unused, you probably mean $base_field_module_definitions :-)
    (probably explains the fails)
    - couldn't we just merge directly into $base_field_definitions here in the loop, instead of going through a temporary var and then doing one final merge in the main array ?

  6. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -402,11 +419,29 @@ protected function buildBundleFieldDefinitions($entity_type_id, $bundle, array $
    +    $provider = $entity_type->getProvider();
         $bundle_field_definitions = $class::bundleFieldDefinitions($entity_type, $bundle, $base_field_definitions);
    +    foreach ($base_field_definitions as $definition) {
    +      $definition->setProvider($provider);
    +    }
    +
    +    // Retrieve base field definitions from modules.
    +    $bundle_field_module_definitions = array();
    +    $hook = 'entity_bundle_field_info';
    +    foreach ($this->moduleHandler->getImplementations($hook) as $module) {
    +      $module_definitions = call_user_func($module . '_' . $hook, $entity_type, $bundle, $bundle_field_module_definitions);
    +      if (isset($module_definitions) && is_array($module_definitions)) {
    +        // Ensure the module key actually matches the name of the module defining
    +        // the field.
    +        foreach ($module_definitions as $definition) {
    +          $definition->setProvider($provider);
    +        }
    +        $return = NestedArray::mergeDeep($bundle_field_module_definitions, $module_definitions);
    +      }
    +    }

         // Invoke 'per bundle' hook.
    -    $result = $this->moduleHandler->invokeAll('entity_bundle_field_info', array($entity_type, $bundle, $base_field_definitions));
    -    $bundle_field_definitions = NestedArray::mergeDeep($bundle_field_definitions, $result);
    +    $bundle_field_definitions = NestedArray::mergeDeep($bundle_field_definitions, $bundle_field_module_definitions);

    Same remarks as above :-)

  7. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -402,11 +419,29 @@ protected function buildBundleFieldDefinitions($entity_type_id, $bundle, array $
         // Invoke 'per bundle' hook.
    -    $result = $this->moduleHandler->invokeAll('entity_bundle_field_info', array($entity_type, $bundle, $base_field_definitions));
    -    $bundle_field_definitions = NestedArray::mergeDeep($bundle_field_definitions, $result);
    +    $bundle_field_definitions = NestedArray::mergeDeep($bundle_field_definitions, $bundle_field_module_definitions);

    Comment is now stale, the invoke happens above

  8. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
    @@ -453,4 +474,26 @@ public static function getReservedColumns() {
    +  public function isCustomStorage() {

    "is" in method name looks weird, a field can be "multiple" or "translatable", but it cannot *be* "custom storage".

    usesCustomStorage() ? hasCustomStorage() ?

yched’s picture

(also, looks like @tstoeckler's #11 still needs to be addressed)

yched’s picture

(also, looks like the issue title + several points in the issue summary should be updated to reflect the current patch - s/module/provider, not NULL for base fields, 'storage' no longer a constant-based flag...)

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new8.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
new8 KB

This should address #11 and #16, which once again demonstrates how copy/pasting is rarely a good long-term strategy :)

Status:Needs review» Needs work

The last submitted patch, 19: entity-field_keys-2143069-19.patch, failed testing.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new1.52 KB
new8.98 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,735 pass(es).
[ View ]

Mmh, I guess entity manager test coverage in not perfect yet...

tstoeckler’s picture

Looks great!

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -328,11 +328,25 @@ protected function buildBaseFieldDefinitions($entity_type_id) {
+          $definition->setProvider($provider);

@@ -402,11 +416,24 @@ protected function buildBundleFieldDefinitions($entity_type_id, $bundle, array $
+          $definition->setProvider($provider);

I still think this should use $module not $provider. I might be missing something.

yched’s picture

Thanks @plach.

+++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
@@ -151,6 +151,27 @@ public function setSetting($setting_name, $value) {
+   * @return $this
+   *   The object itself for chaining.

Nitpick: for "@return $this", no description is needed.

plach’s picture

I still think this should use $module not $provider. I might be missing something.

I spoke with @fago about this and we both agree that provider is more conistent with what we already have in the entity type definition.

New patch coming as soon as my laptop's CPU cools down a bit :(

andypost’s picture

+1 to Provider, provider could be a 'Core' so module makes no sense

tstoeckler’s picture

Title:Add module and storage to entity field definitions» Add getProvider() and hasCustomStorage to FieldDefinitionInterface
Issue summary:View changes
Related issues:+#2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities

Revamped the issue summary a bit.
I don't think we need a (draft) change notice, since this is completely backwards-compatible.

tstoeckler’s picture

Issue summary:View changes
plach’s picture

StatusFileSize
new5.04 KB
new10.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,734 pass(es).
[ View ]

This adds test coverage for the provider key and addresses #22
(I completely missed what Tobias was saying, sorry)

plach’s picture

StatusFileSize
new1.65 KB
new11.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch entity-field_keys-2143069-29.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

And this adds actual coverage for getters/setters. We should be done here.

tstoeckler’s picture

Leaving at needs review for 1., otherwise RTBC.

  1. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
    @@ -553,6 +553,39 @@ public function testGetBaseFieldDefinitionsInvalidDefinition() {
    +    $field_definition = $this->getMockBuilder('Drupal\Core\Field\FieldDefinition')
    +      ->disableOriginalConstructor()
    +      ->getMock();

    I was going to ask whether you didn't use FieldDefinitionInterface because you're using setProvider(), but then I realized that we're calling it unconditionally on the return value of ::baseFieldDefinitions() (and hook_entity_*_info()) so it should really be on FieldDefinitionInterface.

  2. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
    @@ -553,6 +553,39 @@ public function testGetBaseFieldDefinitionsInvalidDefinition() {
    +    // We expect two calls as the field definition will be returned from both
    +    // base and bundle entity field info hook implementations.

    Thank you so much for commenting your tests!!!! This is soooo helpful!! +1000

  3. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
    @@ -553,6 +553,39 @@ public function testGetBaseFieldDefinitionsInvalidDefinition() {
    +      ->with($this->matches($module));

    Does this add an assertion that it is called with $module? I've never heard of $this->matches(), very interesting.

Status:Needs review» Needs work

The last submitted patch, 29: entity-field_keys-2143069-29.patch, failed testing.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new3.16 KB
new11.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,726 pass(es).
[ View ]

Added setProvider() to the interface. Initially I was wondering whether this would make sense too, but then concluded that, since the interface has only getters, for consistency it would be better to avoid that.

But in the end there is really no point in preferring consistency over correctness :)

tstoeckler’s picture

  1. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
    @@ -151,6 +151,21 @@ public function setSetting($setting_name, $value) {
    +  public function getProvider() {
    +    return $this->definition['provider'];
    +  }

    +++ b/core/tests/Drupal/Tests/Core/Entity/FieldDefinitionTest.php
    @@ -194,4 +194,26 @@ public function testFieldRequired() {
    +  public function testFieldProvider() {
    +    $definition = FieldDefinition::create($this->fieldType);
    +    $provider = $this->randomName();
    +    $definition->setProvider($provider);
    +    $this->assertEquals($provider, $definition->getProvider());
    +  }

    I wanted to RTBC, but I just had small question come to my mind: In getProvider() should we provide some sort of fallback if $this->definition['provider'] is not set? I noticed that in the test you are not calling getProvider() before calling setProvider() first. As it's sort of a misconfiguration to have a field without a provider we could also decide that it's perfectly fine to throw a notice if there's no provider specified, but I'm not sure...

  2. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
    @@ -553,6 +553,38 @@ public function testGetBaseFieldDefinitionsInvalidDefinition() {
    +    $field_definition = $this->getMockBuilder('Drupal\Core\Field\FieldDefinitionInterface')
    +      ->getMock();

    Not marking "needs work" for that, but with interfaces you can just do ->getMock('FooInterface') directly.

plach’s picture

Well, we are in the same situation with most (all?) other field definition keys, so I don't think provider needs a special treatment.

tstoeckler’s picture

Status:Needs review» Reviewed & tested by the community

OK, that works for me.

In this particular case we're pretty safe anyway, as we're always calling setProvider() ourselves.

RTBC.

fago’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new10.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
new5.03 KB

But in the end there is really no point in preferring consistency over correctness :)

I do not agree it is though, as the bogus implementations for configurable fields shows it does not make sense for all field definition implementations. That's why we have setters only on the FieldDefinition class. I do not see what should make provider special, so let's follow it here as well.

Also had some other minor remarks, and suggested docu improvements - so just re-rolled it accordingly. Please review.

plach’s picture

Status:Needs review» Needs work
+++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
@@ -472,8 +477,8 @@ public function hasCustomStorage() {
+   *   Pass FALSE if the storage controller takes care of storing the field,
+   *   TRUE otherwise.

I don't get the reason for this change: FALSE is the default value :(

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -336,11 +336,25 @@ protected function buildBaseFieldDefinitions($entity_type_id) {
+      $definition->setProvider($provider);
...
+          $definition->setProvider($module);

@@ -410,11 +424,24 @@ protected function buildBundleFieldDefinitions($entity_type_id, $bundle, array $
+      $definition->setProvider($provider);
...
+          $definition->setProvider($module);

Any field definition that won't provide a setter will automatically break as soon we try to retrieve field definitions now. We need at very least to add a method_exists() check.

The last submitted patch, 36: d8_field_definition_provider.patch, failed testing.

yched’s picture

I tend to agree with @fago that so far we avoided providing setters on stuff that is not writable for config fields. But then (just like has been done for similar cases), the ->setProvider() calls need to be made within an instanceof FieldDefinition check.

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -410,11 +424,24 @@ protected function buildBundleFieldDefinitions($entity_type_id, $bundle, array $
     // Allow the entity class to override the base fields.
+    $provider = $entity_type->getProvider();
     $bundle_field_definitions = $class::bundleFieldDefinitions($entity_type, $bundle, $base_field_definitions);
+    foreach ($bundle_field_definitions as $definition) {
+      $definition->setProvider($provider);
+    }

Same as was done in buildBaseFieldDefinitions(): please move $provider = ... line one line below :-)
Also

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new3.67 KB
new11.67 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,735 pass(es).
[ View ]

This should fix failures.

I am not very happy with the current solution so, after discussing this again with @fago, I opened #2225961: Introduce "controlled" setters on FieldDefinitionInterface to avoid special-casing FieldDefinition. Better handle any controversial detail in a non-critical non-beta-blocker issue :)

plach’s picture

Related issues:
StatusFileSize
new11.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,735 pass(es).
[ View ]
new853 bytes

I forgot about the line order issue...

tstoeckler’s picture

Status:Needs review» Reviewed & tested by the community

plach++

Back to RTBC then.

fago’s picture

Changes look good to me as well + commented on the follow-up.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 41: entity-field_keys-2143069-41.patch, failed testing.

plach’s picture

plach’s picture

Status:Needs work» Reviewed & tested by the community

#40 was green and #41 just swapped a couple of unrelated lines so it should be ok to move this back to RTBC.

catch’s picture

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

  • Commit c15c6c1 on 8.x by catch:
    Issue #2143069 by plach, fago: Add getProvider() and hasCustomStorage to...

Status:Fixed» Closed (fixed)

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