Field item properties do not prevent the services they contain from being serialized

This prevents the Payment module from working, so tagging accordingly. I don't think we need tests, it's relatively hard to test in a useful way. We already have unit tests for the trait.

We have this trait on entity, plugin and various other base classes.

Note that this is a minor BC break: subclasses that implement this trait already would get a annoying PHP strict notice (some sort of notice, I think it is strict) but it's very easy to fix. And not adding it can result in *very* hard to track down bugs.

Ah, now I remembed where I've seen this before. It was in #2053415: Allow typed data plugins to receive injected dependencies, back then it was still a base class but same problem. We had to add it when we started to actually inject dependencies there.

TypedData now actually supports it but core doesn't use it much. But this will happen when someone actually uses that.

Retested, looks like a testbot issue.

I can confirm that the patch from #2 fixes this bug.

That bug is indeed really annoying!

This issue thought has a potential API break, right? What happens if other subclasses of TypedData had that trait already, for some reasons ...

A comment would be great, and possibly the @todo for 9.x that @alexpott mentioned to use the trait?

Yeah we better need a comment but yeah I really tried that on debugging some contrib module and something conflicted when you just put it into TypedData using the trait before ...

... What about adding a weekup method as well?

As well as the @todo is it worth a new Base class that does use the trait?

You usually don't extend from TypedData directly, but from e.g. FieldItemBase, so I'm not sure how much that would really help.

Do you mind adding a quick test to ensure this actually works? serialize($entity->name) should be enough.

Looks good even I haven't see the $sut thing

+++ b/core/tests/Drupal/Tests/Core/TypedData/TypedDataTest.php
@@ -0,0 +1,52 @@
+  /**
+   * The subject under test.
+   *
+   * @var \Drupal\Core\TypedData\TypedData
+   */
+  protected $sut;
+
+  /**
+   * {@inheritdoc}
+   */
+  public function setUp() {
+    parent::setUp();
+
+    $data_definition = $this->getMock(DataDefinitionInterface::class);
+    $this->sut = $this->getMockForAbstractClass(TypedData::class, [$data_definition]);
+  }

I think is quite a big assumption that any other test methods added to this unit test will need the same mocked class. Therefore lets get rid of the introduction of sut - the world does not need more tlas to get in the way of understanding stuff - and just do everything in the testSleep() method with no properties.

Per our IRC discussion, lets use a different variable name, $typed_data or something.

Still has $serialized_sut in there ;)

Thank you.

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!