FieldTypePluginManager cannot instantiate FieldType plugins, good thing TypedDataManager can instantiate just about anything

Edit: the only reason I participate in this because I thought it directly affected my MongoDB work, trying to access the schema of the field types so that I can properly typecast when searching. I *bet* entity query is broken on postgresql too cos of strict typing. I have worked around this and now it doesn't block me but my first attempt was to instantiate the field plugin and call schema(). That doesn't work -- schema() also needs a field definition I do not know where to get from so it's pretty much futile and unnecessary anyways:

$base_field_definitions = \Drupal::entityManager()->getBaseFieldDefinitions($entity_type_id);
if (isset($base_field_definitions[$field_name])) {
  $field_definition = $base_field_definitions[$field_name];
}
else {
  $field_definition = entity_load("field_config", "$entity_type_id.$field_name");
}
var_dump($field_definition->getSchema()

this works.

I elevated this to major because the TypedData codeflow can be a little challenging to follow and having a plugin manager instantiating not just its own plugins but other plugin types and non-plugin classes too (FieldItemList) is really confusing.

Can we please mark this a beta blocker, as it's highly confusing and, to use a buzzhashtag, a #DrupalWTF.

This does not need to be a beta blocker but it can be critical. Using an API as expected and getting a critical is not right.

Yes, that's a long standing gripe with TypedData.
This is a subtopic of #2002138: Use an adapter for supporting typed data on ContentEntities, though.

Agree with @chx

having a plugin manager instantiating not just its own plugins but other plugin types and non-plugin classes too (FieldItemList) is really confusing.

And I would add everything about how plugins are instantiated in general is really confusing. To start with we are instantiating plugins using some constructor that is not part of any interface.
Also everything that extends TypedData like FieldItemBase has atm some hidden dependency on TypedDataManager. See validate() or Map::get() methods.

From #3

the reason that currently everything goes through TypedDataManager is that there is a performance implementation there

I think that shouldn't be an excuse, we should fix the issue first (API, architecture), worry about performance optimizations later.

Btw, if we need such optimizations, then maybe we need them for all plugin managers? Shouldn't they be part of DefaultPluginManager?

About this issue, could we fix it getting FieldtypePluginManager to extend TypedDataManager, like we did with TypedConfigManager, so at least each manager instantiates its own plugins?
Then we just need to fix that 'hidden dependencies' so each manager can also handle its own plugins.

Been a while wince I last struggled with TypedDataManager, and this would totally need @fago's feedback, but solution B (having the DatatDefinition objects be in charge of instanciating their own Data objects) sounds like an interesting approach. We didn't have DatatDefinition inititally.

Then,

- Each DataDefinition defers to the PluginManager it's associated with (FieldTypePluginManager, whatever else). This what does the interface between the PluginManager, and the TypedData system.

- TypedDataManager can still be the go-to "create anything TypedData-ish with "cloned prototypes" optimizations to reduce the number of objects to construct", for regular runtime code like ContentEntity. It is basically a meta-factory that delegates to several actual PluginManagers, through the DataDefinition objects.

- For one-off, non-optimized creation you can directly call the right PluginManager (e.g. FieldTypePluginManager), they work just as you would expect from any PluginManager.

- Hopefully (not sure), it would remove the need to duplicate the "field type" plugin definitions into the "data type plugin definitions". Currently, "field type" plugin definitions are discovered by FieldTypePluginManager, and then, using FieldItemDeriver, re-exposed as "data type" plugin definitions, resulting in duplicate data in persistent cache and in memory cache.
I *think* TypedDataManager would just need its plugin definitions to provide a 'definition_class', without duplicating the rest of the definition.

TypedDataManager can still be the go-to "create anything TypedData-ish with "cloned prototypes" optimizations to reduce the number of objects to construct", for regular runtime code like ContentEntity. It is basically a meta-factory that delegates to several actual PluginManagers, through the DataDefinition objects.

But there's related issue with that cloned prototypes #2221577: Fix assumption that field settings is not a nested array

having the DatatDefinition objects be in charge of instanciating their own Data objects

This doesn't sound very well to me, then we are making it into an object factory, will need to handle injected dependencies, etc...

IMO DataDefinition should just tell the 'Manager' how to instantiate the plug-in which is what it's doing atm, right?

Each DataDefinition defers to the PluginManager it's associated with

Is data definition associated with a plugin-manager?

TypedDataManager can still be the go-to "create anything TypedData-ish with "cloned prototypes" optimizations to reduce the number of objects to construct", for regular runtime code like ContentEntity. It is basically a meta-factory that delegates to several actual PluginManagers,

This sounds interesting, though I think it will work better the other way around. It should be each plugin manager delegating to TypedDataManager.

How I think it should work:
- Each Plugin Manager handles plugin definitions, builds data definitions, etc..
- Plugin Managers delegate actual object creation to TypedDataManager (that should be an injected dependency for them).

But for this we need to:
- Work out a common interface for building these objects. And a constructor is not such a thing.
- Remove static dependencies from TypedData objects on TypedDataManager.

Really I would be more than happy having TypedConfgManager delegating to TypedDataManager instead of extending it. But the issue I've bumped into is TypedDataManager needing to use its own definitions for creating objects:

class TypedDataManager {

  public function createInstance($data_type, array $configuration = array()) {
    $data_definition = $configuration['data_definition'];
    $type_definition = $this->getDefinition($data_type);

    if (!isset($type_definition)) {
      throw new \InvalidArgumentException(format_string('Invalid data type %plugin_id has been given.', array('%plugin_id' => $data_type)));
    }

   .....
}

Then if it needs to have a definition for it -though I don't get what for-, otherwise it throws an exception, it just won't work for 'my' typed config' plugins, that are defined somewhere else.....

I don't know, maybe the issue with other managers is different... ?

This relates pretty much to #2002138: Use an adapter for supporting typed data on ContentEntities, in particular it's sub-issue #2268009: Make typed data object instantiation more flexible . As discussed several times, I very much agree that field items should be instantiated via the field type manager primarily, while typed-data plugins can implement some sort of adapter to typed data's generic creation method.

For that to really work out we need to move over the prototyping approach to the field type manager as well, or at least make the TDM-version usable by the field type manager and proxy the calls through the field type manager.

having the DatatDefinition objects be in charge of instanciating their own Data objects

Interesting idea, not sure it belongs there though. While a short-cut to create instance from the definition makes sense there, I'm not sure the instantiation logic belongs there also. It does not feel right to have to customize a definition class if you need to customize instantiation.
While working on #2002138: Use an adapter for supporting typed data on ContentEntities I started adding a custom static create() method directly to the data type class.

Thanks! Please add a test.

Looks legit indeed - awesome :-)

@fago should probably vet this though. Assigning to him.

+++ b/core/lib/Drupal/Core/Field/FieldTypePluginManager.php
@@ -31,7 +32,8 @@ class FieldTypePluginManager extends DefaultPluginManager implements FieldTypePl
-    parent::__construct('Plugin/Field/FieldType', $namespaces, $module_handler, 'Drupal\Core\Field\FieldItemInterface', 'Drupal\Core\Field\Annotation\FieldType');
+    parent::__construct('Plugin/Field/FieldType', $namespaces, $module_handler, NULL, 'Drupal\Core\Field\Annotation\FieldType');

Why do we remove passing $plugin_interface = 'FieldItemInterface' to the parent __construct() though ?

+++ b/core/lib/Drupal/Core/Field/FieldTypePluginManager.php
@@ -31,7 +32,8 @@ class FieldTypePluginManager extends DefaultPluginManager implements FieldTypePl
+    $this->factory = new TypedDataFactory($this, 'Drupal\Core\Field\FieldItemInterface');

Nitpick: I guess we could add a word of comment that "field type plugins" are FieldItem data objects, and are thus created by the TypedDataFactory ?
[edit : and also that in the regular lifecycle of an entity, they are created by TypedDataManager rather than FieldTypePluginmanager...]

Why do we remove passing $plugin_interface = 'FieldItemInterface' to the parent __construct() ?

Oh, right - because $plugin_interface is only used by ContainerFactory in the DefaultPluginManager.

Is there a way we could add such a $plugin_interface check to TypedDataFactory ?

Note that while this does fix FieldTypePluginManager which is great it doesn't fix the underlying issue which is TypedDataManager instantiating field type plugins. FieldTypePluginManager is still unused. This is probably a non-critical followup which I expect never will be fixed :/

@tstoeckler
Now I know what you tried to talk with me about yesterday.
This is quite a cool solution, as it solves at least one crazyness you get while reading the code.

I'm not sure introducing the TypedDataFactory makes sense, as it promotes an anti-pattern - other plugin manager shouldn't re-use it. What about injecting TypedDataManager as dependency instead?

I'll take a look at it.

What about something like that?

Patch generally works, but probably will require some unit test adaptions. Feedback welcome first. I also changed the calls from the entity / field classes so they only talk to the field type manager, what makes sense but adds another method call.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -363,8 +354,7 @@ protected function getTranslatedField($name, $langcode) {
   -        $entity_adapter = $this->getTypedData();
   -        $field = \Drupal::typedDataManager()->getPropertyInstance($entity_adapter, $name, $value);
   +        $field = \Drupal::service('plugin.manager.field.field_type')->createFieldItemList($definition, $value, $this);
   
   +++ b/core/lib/Drupal/Core/Field/FieldItemList.php
   @@ -56,6 +56,13 @@ public function __construct(DataDefinitionInterface $definition, $name = NULL, T
   +  protected function createItem($offset = 0, $value = NULL) {
   +    return \Drupal::service('plugin.manager.field.field_type')->createFieldItem($this, $offset, $value);
   +  }
   

   Yummy, the actual runtime creation of the FieldItemList and FieldItem objects within an Entity actually goes through the FieldTypePluginManager - this is lovely indeed :-)

2. +++ b/core/lib/Drupal/Core/Field/FieldTypePluginManager.php
   @@ -29,11 +38,110 @@ class FieldTypePluginManager extends DefaultPluginManager implements FieldTypePl
   +  public function createFieldItem(FieldItemListInterface $items, $index, $values = NULL) {
   ...
   +  public function createFieldItemList(FieldDefinitionInterface $definition, $value = NULL, FieldableEntityInterface $entity = NULL) {
   

   It is confusing that the param order is inconsistent between the two methods :
   - with createFieldItemList(), the parent (the Entity) comes last
   - with createFieldItem(), the parent (the List) comes first

   Related, it's insconsistent that:
   - "create an Item with or without a parent" is two separate methods
   - "create an ItemList with or without a parent" is the same method with an optional param ?

   The following would fix that ?
   createFieldItem($items, $index, $values = NULL)
   createFieldItemWithoutParent($definition, $values = NULL)
   createFieldItemList($entity, $field_name, $values = NULL)
   createFieldItemListWithoutParent($definition, $values = array())

ok makes sense, renamed accordingly and moved methods to the interface. Still needs test fixes and added test-coverage for the new methods.

Sorry for letting this slide. Seems moslty good.

1. +++ b/core/lib/Drupal/Core/Field/FieldTypePluginManagerInterface.php
   @@ -17,6 +18,67 @@
   +   * Creates a new field item, without a parent field item list.
   +  public function createFieldItemWithoutParent(FieldDefinitionInterface $definition, $values = NULL);
   ...
   +   * Creates a new field item list, without a parent entity.
   +  public function createFieldItemListWithoutParent(FieldDefinitionInterface $definition, $values = NULL);
   

   IMO we should add a word that items/itemLists without parents are invalid for most regular use cases, and that the "with parent" methods should be used in general be used instead.

   In fact - do we really need to add those two methods to begin with ?

2. +++ b/core/lib/Drupal/Core/Field/FieldTypePluginManagerInterface.php
   @@ -17,6 +18,67 @@
   +   * The entity is assigned as the parent of the list. However, it's the
   +   * job of the caller (i.e. usually the entity object) to make the entity
   +   * aware of the field as well.
   ...
   +  public function createFieldItemList(FieldableEntityInterface $entity, $field_name, $values = NULL);
   

   (phpdoc of createFieldItemList($entity))

   Shouldn't we say something similar for createFieldItem($list) then ? Likewise, it creates an Item with the $list as parent, but doesn't take care of adding the item to the list ?

3. +++ b/core/lib/Drupal/Core/Field/FieldTypePluginManagerInterface.php
   @@ -17,6 +18,67 @@
   +  public function createFieldItemList(FieldableEntityInterface $entity, $field_name, $values = NULL);
   +  /**
   

   Missing newline

Fixes #29
Also test-run in current state

I somehow missed #30, thanks @larowlan :-)

Patch seems to apply with fuzz, rerolled.
If green, this looks ready to me.

I was wondering whether core could leverage the various methods on the field type manager?

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -378,8 +369,7 @@ protected function getTranslatedField($name, $langcode) {
-        $entity_adapter = $this->getTypedData();
-        $field = \Drupal::typedDataManager()->getPropertyInstance($entity_adapter, $name, $value);
+        $field = \Drupal::service('plugin.manager.field.field_type')->createFieldItemList($this, $name, $value);

This line itself makes things much easier to understand.

Some unit tests had to be updated.

Yay, @amateescu++

Then we can replace the:
return \Drupal::typedDataManager()->getPropertyInstance($items, 0);
that #2164601: Stop auto-creating FieldItems on mere reading of $entity->field[N] just added in getOptionsProvider() in FieldStorageConfig and BaseFieldDefinition with
return \Drupal::get('plugin.manager.field.field_type')->createFieldItem($items, 0);

Leaving that to a good soul, so that I can still RTBC ;-)

Feeling brave tonight :)

Well... this is what I get for copy-pasting code without even reading it :/

Hah, right, sorry about that :-)

Thanks! Here we go then...

1. +++ b/core/lib/Drupal/Core/Field/FieldTypePluginManagerInterface.php
   @@ -17,6 +18,82 @@
   +  public function createFieldItemWithoutParent(FieldDefinitionInterface $definition, $values = NULL);
   ...
   +  public function createFieldItemListWithoutParent(FieldDefinitionInterface $definition, $values = NULL);
   

   Both of these methods appear to be untested and used. Why do we need them? And if we do can we add tests?

Yeah... as mentioned in #29.1, I'm not sure why we need those, or if it's really an API we want to provide. Since they have no use case in core atm, I suggest we remove them from the patch, and discuss whether we want them in a separate issue.

Patch removes them, and adjusts the phpdoc for the remaining methods (fixes a couple inaccurracies I didn't notice so far, tweaks phrasing)

I'll be bold and put this back at RTBC, feel free to slap my wrist and ask for someone else to push the button :-)

Reordered methods - conceptually "create item list in an entity" comes before "create item in an item list"
Kept that for a separate patch for easier interdiffs.

+++ b/core/lib/Drupal/Core/Field/FieldTypePluginManager.php
@@ -29,11 +38,50 @@ class FieldTypePluginManager extends DefaultPluginManager implements FieldTypePl
+  /**
+   * {@inheritdoc}
+   *
+   * Creates a field item, which is not part of an entity or field item list.
+   *
+   * @param string $field_type
+   *   The field type, for which a field item should be created.
+   * @param array $configuration
+   *   The plugin configuration array, i.e. an array with the following keys:
+   *   - field_definition: The field definition object, i.e. an instance of
+   *     Drupal\Core\Field\FieldDefinitionInterface.
+   *
+   * @return \Drupal\Core\Field\FieldItemInterface
+   *   The instantiated object.
+   */
+  public function createInstance($field_type, array $configuration = array()) {
+    $configuration['data_definition'] = $configuration['field_definition']->getItemDefinition();
+    return $this->typedDataManager->createInstance("field_item:$field_type", $configuration);
+  }

I don't think we have any implicit or explicit test coverage of this... which is the whole point of the issue.

I think I will have some time this afternoon to add something to FieldTypePluginManagerTest to cover this.

Here is a first pass at a test.

+++ b/core/modules/field/src/Tests/FieldTypePluginManagerTest.php
@@ -26,4 +28,26 @@ function testDefaultSettings() {
+      $this->assertTrue($instance instanceof $class, format_string('Created a @class instance', array('@class' => $class)));

I would vote to use String::format by default, if possible.

On top of it I wonder whether we can get some more values to ensure that for example the configuration is passed along correctly?

OK, not totally sure what you had in mind for additional config, but here are some changes. This is going to fail.

Since FieldTypePluginManager::createInstance()

Creates a field item, which is not part of an entity or field item list.

There is no parent, so FieldItemBase::getFieldDefinition() and friends barf b/c the ->getParent() is baked into those calls. Not sure what the proper thing to do is, or is I just goofed my test config.

Also, should FieldTypePluginManager::createInstance() and the associated docblock be added to FieldTypePluginManagerInterface?

In order to fix the failure we probably have to adapt FieldItemBase to not require a parent all the time, so probably check whether the parent
returns something, and in case it doesn't , use just $this->definition ?

I'll play with this and see what breaks...

Unassigning myself. I haven't made any progress on this, and don't have anything worthwhile to post as an in-progress patch.

Perhaps this?

+++ b/core/modules/field/src/Tests/FieldTypePluginManagerTest.php
@@ -78,12 +79,14 @@ function testCreateInstanceWithConfig() {
-    $this->assertEqual($instance->getFieldDefinition()->getDefaultValue(NULL), 8675309, 'Instance default_value is 8675390');
+    $this->assertEqual($instance->getFieldDefinition()->getDefaultValue($entity), [['value' => 8675309]], 'Instance default_value is 8675390');

why is that change needed

+++ b/core/lib/Drupal/Core/Field/FieldItemBase.php
@@ -78,7 +78,12 @@ public function getLangcode() {
+    if ($this->getParent()) {
+      return $this->getParent()->getFieldDefinition();
+    }

you could store parent in the if

addressed 54 - also changed the number in the assert message which was different :)

re: #55 - getDefaultValue() needs the entity - which why it failed in #48 - I think.

+++ b/core/lib/Drupal/Core/Field/FieldItemBase.php
@@ -78,7 +78,12 @@ public function getLangcode() {
   public function getFieldDefinition() {
-    return $this->getParent()->getFieldDefinition();
+    if ($parent = $this->getParent()) {
+      return $parent->getFieldDefinition();
+    }
+    elseif ($field_definition = $this->definition->getFieldDefinition()) {
+      return $field_definition;
+    }
   }

Hm - then why don't we just always return $this->definition->getFieldDefinition() ?

Hmm, good point

Good idea @yched

The issue summary still describes multiple possible options, let's remove the other one, I think.

+++ b/core/modules/field/src/Tests/FieldTypePluginManagerTest.php
@@ -26,4 +30,63 @@ function testDefaultSettings() {
+  function testCreateInstance() {

Nitpick: Let's make it public, please ... there are instances which are protected, which is kinda problematic, see #2443381: Make all test functions public

Added explicit public declaration (though by default it was already public).

Cleaned up the IS a bit, and hid older files.

Thank you!

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed b0f4b41 and pushed to 8.0.x. Thanks!