Add getProvider() and hasCustomStorage to FieldDefinitionInterface

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?

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.

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.

Beta-blocking as a part of #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions.

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

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.

This should be better.

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

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

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() ?

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

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

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

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

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.

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.

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

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

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

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

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

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.

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

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.

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

OK, that works for me.

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

RTBC.

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

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

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

I forgot about the line order issue...

plach++

Back to RTBC then.

41: entity-field_keys-2143069-41.patch queued for re-testing.

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

Committed/pushed to 8.x, thanks!