Updated: Comment #72

Problem/Motivation

Typed data plugins need to have dependencies injected. See #2903549: Replace REQUEST_TIME with time service in CreatedItem as an example of plugin that can benefit from this

Proposed resolution

1. Make TypedDataManager container aware (use ContainerAwareTrait)
2a. Check if created plugin implements ContainerAwareInterface, and if yes, call its setContainer() method.
2b. Check if plugin definition contains "services" section. If no - same logic as before. If yes,
3a. Inject services in additional parameter of createInstance()
3b. Assume that plugin implements ContainerFactoryPluginInterface, call ContainerFactoryPluginInterface::create()
3c. Directly call plugin constructor

2a is 2a and 3b are fully backwards compatible.
3a should be BC as well (on assumption that no plugins use extra parameters to createInstance())

Remaining tasks

Decide what is the best approach
Write the patch
Test it
Review it

User interface changes

None

API changes

Add @service_container as a parameter of typed_data_manager service

#1951268: Convert /forum and /forum/% to new router, remove forum_forum_load(), forum_get_topics(), create Forum service
#2914419: Allow Plugins to specify Services via Annotation

CommentFileSizeAuthor
#71 2053415-71.patch1.94 KBvalthebald
#71 interdiff-2053415-68-71.txt4.02 KBvalthebald
#68 2053415-68.patch4.63 KBvalthebald
#68 interdiff-2053415-66-68.txt1.16 KBvalthebald
#66 2053415-66.patch4.6 KBvalthebald
#66 interdiff-2053415-61-66.txt3.56 KBvalthebald
#61 2053415-61.patch3.22 KBvalthebald
#44 interdiff.txt1.8 KBdixon_
#44 2053415-typeddata-container-44.patch9.46 KBdixon_
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#42 2053415-typeddata-container-42.patch9.35 KBdixon_
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#40 drupal_2053415_40.patch10 KBXano
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#37 typed_data_inject-2053415-37.patch19.72 KBsmiletrl
FAILED: [[SimpleTest]]: [MySQL] 57,876 pass(es), 31 fail(s), and 22 exception(s). View
#37 interdiff-35-37.txt1.4 KBsmiletrl
#35 typed_data_inject-2053415-35.patch19.73 KBsmiletrl
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#35 interdiff-33-35.txt1.15 KBsmiletrl
#33 typed_data_inject-2053415-33.patch20.08 KBsmiletrl
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch typed_data_inject-2053415-33.patch. Unable to apply patch. See the log in the details link for more information. View
#33 interdiff-30-33.txt16.85 KBsmiletrl
#30 typed-data-inject-2053415.29.interdiff.txt767 byteslarowlan
#30 typed-data-inject-2053415.29.patch17.07 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 54,978 pass(es), 415 fail(s), and 289 exception(s). View
#25 typed-data-inject-2053415.25.interdiff.txt1.29 KBlarowlan
#25 typed-data-inject-2053415.25.patch17.07 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 55,054 pass(es), 862 fail(s), and 465 exception(s). View
#19 typed-data-inject-2053415.19.interdiff.txt680 byteslarowlan
#19 typed-data-inject-2053415.19.patch16.41 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 57,801 pass(es). View
#17 typed-data-inject-2053415.17.interdiff.txt986 byteslarowlan
#17 typed-data-inject-2053415.17.patch16.36 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 57,461 pass(es), 3 fail(s), and 1 exception(s). View
#14 typed-data-inject-2053415.14.interdiff.txt5.76 KBlarowlan
#14 typed-data-inject-2053415.14.patch15.4 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 57,549 pass(es), 92 fail(s), and 16 exception(s). View
#12 typed-data-inject-2053415.12.interdiff.txt4.62 KBlarowlan
#12 typed-data-inject-2053415.12.patch10.19 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 48,355 pass(es), 1,028 fail(s), and 229,678 exception(s). View
#10 typed-data-inject-2053415.10.interdiff.txt7.78 KBlarowlan
#10 typed-data-inject-2053415.10.patch5.57 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#2 typed-data-inject-2053415.1.patch3.85 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

larowlan’s picture

Assigned: Unassigned » larowlan

before @timplunkett grabs it ;)

larowlan’s picture

Assigned: larowlan » Unassigned
Status: Active » Needs review
FileSize
3.85 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Lets see

tim.plunkett’s picture

+++ b/core/modules/text/lib/Drupal/text/TextProcessed.phpundefined
@@ -35,9 +39,17 @@ class TextProcessed extends TypedData {
-  public function __construct(array $definition, $name = NULL, TypedDataInterface $parent = NULL) {
+  public function __construct(array $definition, $name = NULL, TypedDataInterface $parent = NULL, FieldInfo $field_info = NULL) {

There is no reason to have = NULL for any of these

+++ b/core/modules/text/lib/Drupal/text/TextProcessed.phpundefined
@@ -46,6 +58,13 @@ public function __construct(array $definition, $name = NULL, TypedDataInterface
+    return new static($configuration, $plugin_id, $parent, $container->get('field.info'));

Might as well wrap these onto multiple layers.

+++ b/core/modules/text/lib/Drupal/text/TextProcessed.phpundefined
@@ -67,14 +86,16 @@ public function getValue($langcode = NULL) {
+      // @todo Replace when check_markup() is a service provided by filter
+      //   module.

Good idea!

Status: Needs review » Needs work

The last submitted patch, typed-data-inject-2053415.1.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
+++ b/core/modules/text/lib/Drupal/text/TextProcessed.phpundefined
@@ -35,9 +39,17 @@ class TextProcessed extends TypedData {
+
+  /**
    * Overrides TypedData::__construct().
    */
-  public function __construct(array $definition, $name = NULL, TypedDataInterface $parent = NULL) {
+  public function __construct(array $definition, $name = NULL, TypedDataInterface $parent = NULL, FieldInfo $field_info = NULL) {

Needs doc changes.

Berdir’s picture

Status: Needs review » Needs work

I think you picked a somewhat bad example because I think that this class could now get the information it needs from the $field/fieldItem class and doesn't need the field info service. $field->getFieldDefinition()->getSetting(). That would also make it re-usable without a configurable field (or at least a step into that direction).

larowlan’s picture

Damn, I spent nearly 30 mins looking for an example.
I need it for forum container, but wanted this in separate.

larowlan’s picture

\Drupal\Core\TypedData\Plugin\DataType\Map is a good candidate as it uses \Drupal::typedData()

Berdir’s picture

The TypedData base class itself uses the typed data manager too, for constraint/validation stuff for example.

larowlan’s picture

Status: Needs work » Needs review
FileSize
5.57 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
7.78 KB

This does TypedData and Map and at least installs (which is a start).
Lets see what else is broken.

Status: Needs review » Needs work

The last submitted patch, typed-data-inject-2053415.10.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
10.19 KB
FAILED: [[SimpleTest]]: [MySQL] 48,355 pass(es), 1,028 fail(s), and 229,678 exception(s). View
4.62 KB

This one installs locally (For real this time).

Status: Needs review » Needs work

The last submitted patch, typed-data-inject-2053415.12.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
15.4 KB
FAILED: [[SimpleTest]]: [MySQL] 57,549 pass(es), 92 fail(s), and 16 exception(s). View
5.76 KB

so TypedConfigManager and LocaleTypedConfig need some of the injection action too...
I wish api.d.o was up to date for the inheritance map.
And I mixed a property with a method.
I think that quantity of fails and exceptions is a personal record.

Status: Needs review » Needs work

The last submitted patch, typed-data-inject-2053415.14.patch, failed testing.

larowlan’s picture

Exception: Serialization of 'Symfony\Component\HttpFoundation\File\UploadedFile' is not allowed in serialize() (line 99 of /core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.php)
Going to need some pointers/help on finding/fixing that.

larowlan’s picture

Status: Needs work » Needs review
Issue tags: +Needs profiling
FileSize
16.36 KB
FAILED: [[SimpleTest]]: [MySQL] 57,461 pass(es), 3 fail(s), and 1 exception(s). View
986 bytes

TypedData->typedDataManager->languageManager->request->files was the culprit.
We've got some serious serializing going on here, I think this needs profiling.

Status: Needs review » Needs work

The last submitted patch, typed-data-inject-2053415.17.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
16.41 KB
PASSED: [[SimpleTest]]: [MySQL] 57,801 pass(es). View
680 bytes

NodeTest was new, but EntityTranslation was valid, request object on LanguageManager is optional

dawehner’s picture

Nearly RTBC.

+++ b/core/lib/Drupal/Core/TypedData/TypedData.phpundefined
@@ -49,13 +59,43 @@
+  /**
+   * Creates an instance of the plugin.
+   *
+   * @param \Symfony\Component\DependencyInjection\ContainerInterface $container
+   *   The container to pull out services used in the plugin.
+   * @param array $configuration
+   *   A configuration array containing information about the plugin instance.
+   * @param string $plugin_id
+   *   The plugin ID for the plugin instance.
+   * @param array $plugin_definition
+   *   The plugin implementation definition.
+   * @param \Drupal\Core\TypedData\TypedDataInterface $parent
+   *   (optional) The parent object of the data property, or NULL if it is the
+   *   root of a typed data tree. Defaults to NULL.
+   *
+   * @return static
+   *   Returns an instance of this plugin.

You could just use {@inheritdoc}

larowlan’s picture

The TypedDataInterface param isn't in the parent, hence I didn't use inheritdoc

Berdir’s picture

$node = node_load(5);
dpm(strlen(serialize($node)));

HEAD:
8401

With this patch:
81032

That's not cool :)

Let's just drop and re-fetch the typed data manager in serialize()/unserialize of typed data objects.

Later on, I want to investigate if we can to something like this in EntityNG or on the typed data level:

  public function serialize() {
    return serialize($this->getPropertyValues());
  }

Thats 5553 for my node. That however isn't going to be trivial and needs thorough profiling in cpu vs. memory, especially the unserialize part, so this is something to figure out later...

larowlan’s picture

thanks will do.
berdir++

larowlan’s picture

Assigned: Unassigned » larowlan

tackling

larowlan’s picture

Issue tags: -Needs profiling
FileSize
17.07 KB
FAILED: [[SimpleTest]]: [MySQL] 55,054 pass(es), 862 fail(s), and 465 exception(s). View
1.29 KB

We've handled the serialization issue so removing the tag.
Fixes #22

larowlan’s picture

Assigned: larowlan » Unassigned

Status: Needs review » Needs work

The last submitted patch, typed-data-inject-2053415.25.patch, failed testing.

larowlan’s picture

infinite recursion when serializing basically.
so we'll probably need #22 sooner, giving it a try locally

Berdir’s picture

+++ b/core/lib/Drupal/Core/TypedData/TypedData.phpundefined
@@ -226,4 +226,27 @@ public function getPropertyPath() {
+    // Remove the typedDataManager to reduce the size of the object when
+    // serialized.

Maybe TypedData manager instead of typedDataManager?

+++ b/core/lib/Drupal/Core/TypedData/TypedData.phpundefined
@@ -226,4 +226,27 @@ public function getPropertyPath() {
+      $this->{$key} = $value;

This will trigger __get() :(

Mabe use __sleep()/__wakeup() instead?

larowlan’s picture

Status: Needs work » Needs review
FileSize
17.07 KB
FAILED: [[SimpleTest]]: [MySQL] 54,978 pass(es), 415 fail(s), and 289 exception(s). View
767 bytes

Perhaps this?

Status: Needs review » Needs work

The last submitted patch, typed-data-inject-2053415.29.patch, failed testing.

Berdir’s picture

This might be easier if we wait for #2004282: Injected dependencies and serialization hell then we can use the same pattern..

smiletrl’s picture

Status: Needs work » Needs review
Issue tags: +API change, +Field API, +type data
FileSize
16.85 KB
20.08 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch typed_data_inject-2053415-33.patch. Unable to apply patch. See the log in the details link for more information. View

I come from #2083937-8: Make typed data class dependency injectable., so here're some changes related.

1).
TypedData extends DependencySerialization, instead of implementing \Serializable to deal with serialization.

2).
Because typedData class may have __get() and __set(), rewrite __sleep() of DependencySerialization to avoid possible indirect modification of overloaded property.

-        $this->_serviceIds[$key] = $value->_serviceId;
+        $this->_serviceIds += array($key => $value->_serviceId);

3).
class_implements($class, '\Drupal\Core\Plugin\ContainerFactoryPluginInterface') will ALWAYS return true, as long as $class has implemented interface(s). So

-    if (class_implements($class, '\Drupal\Core\Plugin\ContainerFactoryPluginInterface')) {
+    if (in_array('Drupal\Core\Plugin\ContainerFactoryPluginInterface', class_implements($class))) {

4).
Not all typed data need to inject services, so set the last paramter $typedDataManager to be optional for abstract class TypedData.

-  public function __construct(array $definition, $name = NULL, TypedDataInterface $parent = NULL, TypedDataManager $typed_data_manager) {
+  public function __construct(array $definition, $name = NULL, TypedDataInterface $parent = NULL, TypedDataManager $typed_data_manager = NULL) {

For example, LocaleTypedConfig doesn't need the injection at all. So remove all unnecessary changes associated with this class, which changes are simply to be consistent with the inject pattern, but doesn't make much sense for the actual usage.

Two questions:
1).
ContainerFactoryPluginInterface is not a perfect interface for our usage here. Its ::create includes an extra paramter array $plugin_definition which is never be used here. But we have to add this parameter for typed Data's ::create method and add this redundant pamater every time when this ::create method is called.

Is it worth to create a new TypedDataInjectionInterface to avoid this redundant parameter, i.e., array $plugin_definition from ContainerFactoryPluginInterface, and match the exact TypedData's create pattern?

2).
Current code has added the type hint TypedDataManager $typed_data_manager = NULL in typed Data's construct method.

-  public function __construct(array $definition, $name = NULL, TypedDataInterface $parent = NULL) {
+  public function __construct(array $definition, $name = NULL, TypedDataInterface $parent = NULL, TypedDataManager $typed_data_manager = NULL) {

Therefore, we need to add following line too:

+use Drupal\Core\TypedData\TypedDataManager;

I'm wondering is it really necessary to add the type hint here. The inject pattern allows developers to inject pseudo/different services/objects into class in unit test or even contrib code. If it has limited object $typed_data_manager to be instance of calss "Drupal\Core\TypedData\TypedDataManager", then what's the point of this dependency inject pattern? I guess we should remove the type hint here imho.

Status: Needs review » Needs work

The last submitted patch, typed_data_inject-2053415-33.patch, failed testing.

smiletrl’s picture

Status: Needs work » Needs review
FileSize
1.15 KB
19.73 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Rerolled.

Also, TextProcessed doesn't need injection too. So, remove unnecessary changes.

Status: Needs review » Needs work

The last submitted patch, typed_data_inject-2053415-35.patch, failed testing.

smiletrl’s picture

Status: Needs work » Needs review
FileSize
1.4 KB
19.72 KB
FAILED: [[SimpleTest]]: [MySQL] 57,876 pass(es), 31 fail(s), and 22 exception(s). View

Fix some bugs

Status: Needs review » Needs work

The last submitted patch, typed_data_inject-2053415-37.patch, failed testing.

smiletrl’s picture

This problem happens to "Drupal\Core\Entity\Field\Field".

I feels it's kind of related with TypedDataManager::getPropertyInstance, and more specificly, it's about the prototyping. During the prototype process, dynamic property _serviceIds (which was created in __sleep() phase) has been overridden. Porperty typedDataManager, which was created in __construct phase, was overridden.

For someone interested, here's how to reproduce the problem:

  1. Install the standard drupal profile.
  2. Add a new page, with book form setting to create a new book.
  3. Click save, and you get the error.

Or, you may try:

  1. Add an text field with unlimited values setting for node -- page.
  2. Add a new page, with three values for the new text field.
  3. Click save, and you get the error.
smiletrl’s picture

Issue summary: View changes

Updated issue summary.

Xano’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
10 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Re-roll.

Status: Needs review » Needs work

The last submitted patch, 40: drupal_2053415_40.patch, failed testing.

dixon_’s picture

Status: Needs work » Needs review
FileSize
9.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

Re-roll with some manual updates. Things apparently changed a lot since 6 months ago ;)

  1. +++ b/core/modules/locale/lib/Drupal/locale/LocaleConfigManager.php
    @@ -36,6 +36,14 @@ class LocaleConfigManager extends TypedConfigManager {
       /**
    +   * The typed data plugin manager.
    +   *
    +   * @var \Drupal\Core\TypedData\TypedDataManager
    +   */
    +  protected $typedDataManager;
    +
    +
    +  /**
    

    I'm not sure why this was introduced here. Doesn't seem to be used anywhere. Removed in attached patch.

  2. +++ b/core/lib/Drupal/Core/TypedData/TypedData.php
    @@ -56,10 +69,38 @@
    +  public static function create(ContainerInterface $container, array $configuration, $plugin_id, array $plugin_definition, TypedDataInterface $parent = NULL) {
    +    return new static(
    +      $configuration,
    +      $plugin_id,
    +      $parent,
    +      $container->get('typed_data')
    +    );
    

    $configuration seems to be the wrong argument here. I believe we need the data definition instead. Fixed in the attached patch.

Status: Needs review » Needs work

The last submitted patch, 42: 2053415-typeddata-container-42.patch, failed testing.

dixon_’s picture

Status: Needs work » Needs review
FileSize
9.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
1.8 KB

Never mind regarding #42.2, it's of course the ContainerFactoryPluginInterface dictating that...

I'm still getting some install errors, but uploading patch for others to take a look at.

Status: Needs review » Needs work

The last submitted patch, 44: 2053415-typeddata-container-44.patch, failed testing.

Berdir’s picture

Yeah, but ContainerFactoryPluginInterface doesn't work for us, we're not using the default factory either as our constructor is different, and create() has to match __construct().

Needs a typed data specific interface I guess?

Jose Reyero’s picture

Hey, about fixing the TypedConfigManager, see #2277945: Typed configuration implements TypedDataInterface improperly

I think that should help this patch. Also this patch will help fixing a number of issues with typed config, like validation, etc, and overall it will help adding some more consistency.

Jose Reyero’s picture

Berdir’s picture

That has nothig to do with this, it only allows to inject the typed data manager into something, not inject other services into typed data plugins.

Jose Reyero’s picture

@Berdir,
Maybe not, you're right, I've re-read the issue and it's not exactly what I thought it was, but..

...anyway, this is how I see it, let me know whether this makes sense or not:

We happen to have typed configuration and typed configuration manager reusing all this stuff. The whole point of TypedConfigManager is being able to use all typed data plugins for typed configuration. And with the changes proposed here that will be still harder.

Which other services do we need to be injected? Because the only one I see plugins using is typed data manager, and that is the one that differs with typed config and we need to be able to inject/change it.

+  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition, TypedDataInterface $parent = NULL) {
+    return new static(
+      $plugin_definition,
+      $plugin_id,
+      $parent,
+      $container->get('typed_data_manager')
+    );
   }

I don't think that works if we want to be able to use these plugins with a different 'typed data manager' which is the case for configuration. We need to pass the typed data manager as a parameter for that.

On the other side having two simple methoget/setTypedDataManager() would fix it for both TypedDataManager and TypedConfigManager.

So please let's try for typed data to be reusable and not making it still more difficult.

larowlan’s picture

shameless bump

Xano’s picture

Needs a typed data specific interface I guess?

Either that, or we need to expand the interface so we can inject the extra data. Abusing the configuration array for this (which is plugin-specific and not plugin-type-specific) is a bad idea.

On the one hand introducing a new factory interface isn't very pretty, but adding setters on the plugin type interface can give people the false idea that those values might change during a plugin instance's lifetime.

bojanz’s picture

Is there still a chance to move forward with this?

TypedData not supporting dependency injection is one of its most visible ugly sides.

wizonesolutions’s picture

Version: 8.0.x-dev » 8.1.x-dev

I'm guessing this has to be part of 8.1.x now?

Given the changes to Drupal 8, what would be the best way to reroll this? From what I'm reading, we want to avoid using ContainerFactoryPluginInterface...but at the same time, we're not sure if we want to make another injection interface just for TypedData.

I came here because I saw FileFieldItemList using the global Drupal container to call services, and that didn't feel right since it's not procedural. A couple tweets later, and I was pointed to this issue.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

ohthehugemanatee’s picture

I'm here because I saw EntityReference calling the Entity Storage service with the global container. This issue seems pretty dead though... What's the next step here?

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

pwolanin’s picture

It looks like some parts of this patch are outdated perhaps? Overall it should not be that hard, but need to watch out for side effects.

Seems a bit tricky since \Drupal\Core\TypedData\TypedData::createInstance is a static method

It might make sense to use setter injection here so the main constructor is unchanged?

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

valthebald’s picture

Issue summary: View changes
Status: Needs work » Needs review
Related issues: +#2903549: Replace REQUEST_TIME with time service in CreatedItem
FileSize
3.22 KB
joachim’s picture

+++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
@@ -88,7 +98,7 @@ public function createInstance($data_type, array $configuration = []) {
+    $typed_data = $class::createInstance($data_definition, $configuration['name'], $configuration['parent'], $this->container);

Shouldn't this be the same pattern as plugins, where the factory checks whether the plugin implements ContainerFactoryPluginInterface?

Berdir’s picture

No because other plugins have different arguments, that doesn't work here.

mradcliffe’s picture

@@ -62,7 +71,8 @@ class TypedDataManager extends DefaultPluginManager implements TypedDataManagerI
    * @param \Drupal\Core\DependencyInjection\ClassResolverInterface $class_resolver
    *   The class resolver.
    */
-  public function __construct(\Traversable $namespaces, CacheBackendInterface $cache_backend, ModuleHandlerInterface $module_handler, ClassResolverInterface $class_resolver) {
+  public function __construct(ContainerInterface $container, \Traversable $namespaces, CacheBackendInterface $cache_backend, ModuleHandlerInterface $module_handler, ClassResolverInterface $class_resolver) {
+    $this->container = $container;

Would it be better to add this as the last argument instead of the first to match the pattern of the other plugin managers?

@@ -88,7 +98,7 @@ public function createInstance($data_type, array $configuration = []) {
     if (!isset($class)) {
       throw new PluginException(sprintf('The plugin (%s) did not specify an instance class.', $data_type));
     }
-    $typed_data = $class::createInstance($data_definition, $configuration['name'], $configuration['parent']);
+    $typed_data = $class::createInstance($data_definition, $configuration['name'], $configuration['parent'], $this->container);

Maybe we need a new injection interface for type data plugins to implement that defines a new static method (something like createInjectableInstance?), and then typed data manager can check if the class implements that interface and passes the container into it. That probably is best as the first argument to match the other injection patterns.

Status: Needs review » Needs work

The last submitted patch, 61: 2053415-61.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

valthebald’s picture

Status: Needs work » Needs review
FileSize
3.56 KB
4.6 KB

Fix the failing test (it fails because TypedDataManager has additional constructor parameter), and make container the last parameter per #64.

Regarding new interface... not sure about this proposal. This will require checking the field type, and the only difference is having container in (ignored by non-DI fields) parameter. I think single (existing) interface is better

ndobromirov’s picture

Status: Needs review » Needs work

This is adding an API change on the create method for all plugins, so it will cause contrib code to break.

valthebald’s picture

Status: Needs work » Needs review
FileSize
1.16 KB
4.63 KB

@mradcliffe: actually, we don't need new interface, but plugins that need container can implement ContainerAwareInterface (and related trait). Here's the patch:

mradcliffe’s picture

we don't need new interface, but plugins that need container can implement ContainerAwareInterface (and related trait).

A new interface with a different method name would be backwards-compatible (see @Berdir's comment in #63). I don't like the idea of a new interface, but the create and createInstance methods already exist on the TypedData class, and that makes a big API change in my opinion.

valthebald’s picture

Having container aware plugins is also backward compatible, no?

valthebald’s picture

Simpler version. Inject container into TypedDataManager via "calls". This should work with no test modification.
Data plugins that need injection should implement ContainerAwareInterface

mradcliffe’s picture

Ah, yes, it would be for TypedData classes.

I think that ContainerInjectionInterface::create would be the pattern that other plugin systems use though and not ConatinerAwareInterface.

valthebald’s picture

ContainerInjectionInterface::create() does not fit, since it has only 1 parameter. The closest by parameters (and logically) is ContainerFactoryPluginInterface::create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition);

Question is how to determine if plugin needs injection or not? Possible way it to add services list to plugin annotation:

/**
 * Defines the 'created' entity field type.
 *
 * @FieldType(
 *   id = "created",
 *   label = @Translation("Created"),
 *   description = @Translation("An entity field containing a UNIX timestamp of when the entity has been created."),
 *   no_ui = TRUE,
 *   default_widget = "datetime_timestamp",
 *   default_formatter = "timestamp",
 *   services = {"datetime.time"}
 * )
 */

Then, in TypedDataManager::createInstance():

    if (!isset($type_definition['services'])) {
      $type_definition['services'] = [];
    }
    $services = $type_definition['services'];
    foreach ($type_definition['services'] as $service) {
      $services[$service] = $this->container->get($service);
    }

$services then can be appended to $class::createInstance() parameters - either in a single array parameter, or every service appended as a separate parameter. I'd prefer every service being separate parameter, as it won't affect "container unaware" (read "all existing") typed data plugins

Opinions?

mradcliffe’s picture

ContainerInjectionInterface::create() does not fit, since it has only 1 parameter. The closest by parameters (and logically) is ContainerFactoryPluginInterface::create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition);

That's why I suggest a new interface. An interface solves whether or not to inject the container or not because we can detect whether a class implements an interface. If it does, call the static. If not, call the constructor with the default arguments.

valthebald’s picture

1. Do you suggest the new interface only for plugins that needs injection? If yes, how do you distinguish between "injectable" and "regular" plugins?
2. Why injectable plugins cannot implement ContainerFactoryPluginInterface?

valthebald’s picture

The idea of using plugin annotation to describe what services it needs, is suggested also in #2914419: Allow Plugins to specify Services via Annotation

valthebald’s picture

mradcliffe’s picture

Chatted with valthebald in Slack and he pointed me to a similar effort in #2914419: Allow Plugins to specify Services via Annotation. If that pattern is OK, then I think Typed Data annotations can do similarly though we cannot use the ServiceFactory introduced there because of the argument differences. Unless ServiceFactory was changed to also send in default arguments from the plugin manager.

@@ -89,6 +92,9 @@ public function createInstance($data_type, array $configuration = []) {
       throw new PluginException(sprintf('The plugin (%s) did not specify an instance class.', $data_type));
     }
     $typed_data = $class::createInstance($data_definition, $configuration['name'], $configuration['parent']);
+    if ($typed_data instanceof ContainerAwareInterface) {
+      $typed_data->setContainer($this->container);
+    }
     $typed_data->setTypedDataManager($this);
     return $typed_data;
   }

I think it should be something like the following rather than injecting the container into the typed data object if we need to implement this on our own without ServiceFactory.

  if (!empty($services)) {
    // Get constructor arguments / resolve services.
    $typed_data::__construct($args);
  }
  else {
    $typed_data::createInstance(...$defaults);
  }
valthebald’s picture

Issue summary: View changes

Updated issue summary to list what are possible options. Time to decide :)

valthebald’s picture

Issue summary: View changes
ndobromirov’s picture

I would block this until https://www.drupal.org/project/drupal/issues/2914419 is resolved, as it's essentially the same issue and 2 core plugin maintainers are on it.

Once there is an agreed way of how to do this in core this issue will be trivial.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.