Updated: Comment #0

Problem/Motivation

Typed data plugins need to have dependencies injected.

Proposed resolution

Allow injection in TypedDataManager::createMethod ala check if the class implements \Drupal\Core\Plugin\ContainerFactoryPluginInterface and use that.

Remaining tasks

Patch it
Test it
Review it

User interface changes

Zilch

API changes

Nada

#1951268: Convert /forum and /forum/% to new router, remove forum_forum_load(), forum_get_topics(), create Forum service

Files: 
CommentFileSizeAuthor
#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 ]
#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 ]

Comments

larowlan’s picture

Assigned:Unassigned» larowlan

before @timplunkett grabs it ;)

larowlan’s picture

Assigned:larowlan» Unassigned
Status:Active» Needs review
StatusFileSize
new3.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
StatusFileSize
new5.57 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
new7.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
StatusFileSize
new10.19 KB
FAILED: [[SimpleTest]]: [MySQL] 48,355 pass(es), 1,028 fail(s), and 229,678 exception(s).
[ View ]
new4.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
StatusFileSize
new15.4 KB
FAILED: [[SimpleTest]]: [MySQL] 57,549 pass(es), 92 fail(s), and 16 exception(s).
[ View ]
new5.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
StatusFileSize
new16.36 KB
FAILED: [[SimpleTest]]: [MySQL] 57,461 pass(es), 3 fail(s), and 1 exception(s).
[ View ]
new986 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
StatusFileSize
new16.41 KB
PASSED: [[SimpleTest]]: [MySQL] 57,801 pass(es).
[ View ]
new680 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:

<?php
 
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
StatusFileSize
new17.07 KB
FAILED: [[SimpleTest]]: [MySQL] 55,054 pass(es), 862 fail(s), and 465 exception(s).
[ View ]
new1.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
StatusFileSize
new17.07 KB
FAILED: [[SimpleTest]]: [MySQL] 54,978 pass(es), 415 fail(s), and 289 exception(s).
[ View ]
new767 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
StatusFileSize
new16.85 KB
new20.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.

<?php
-        $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

<?php
-    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.

<?php
-  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.

<?php
-  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:

<?php
+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
StatusFileSize
new1.15 KB
new19.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
StatusFileSize
new1.4 KB
new19.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
StatusFileSize
new10 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
StatusFileSize
new9.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
StatusFileSize
new9.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
new1.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.