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
#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

Assigned:Unassigned» larowlan

before @timplunkett grabs it ;)

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

Lets see

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

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.

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

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

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

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

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.

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.

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.

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.

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.

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

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}

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

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

thanks will do.
berdir++

Assigned:Unassigned» larowlan

tackling

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

Assigned:larowlan» Unassigned

Status:Needs review» Needs work

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

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

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

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.

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

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.

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.

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.

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.

Issue summary:View changes

Updated issue summary.

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.