Problem/Motivation

ContextDefininition uses the TypedDataTrait and can gen serialized, for example with page_manager/panels.

Proposed resolution

Use the magic DependencySerializationTrait

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#2 core-context-definition-serialization-2831525-1.patch625 bytesberdir

Comments

Berdir created an issue. See original summary.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new core-context-definition-serialization-2831525-1.patch625 bytes
samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Seems like a simple change and passes tests, making as RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

If this is a major bug we should have test coverage of ContentDefinition serialisation.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

What do you propose we test exactly?

I made it major because this issue + #2831521: Avoid unserialization of blockPluginCollection in BlockDisplayVariant are preventing weird segfaults and fatal errors on PHP 7 that are extremely hard to reproduce and debug. And there are a number bug reports in the page_manager issue queue about it and in panels possibly too.

But there is no actual functional bug here that can be shown in a test.

We already have \Drupal\Tests\Core\DependencyInjection\DependencySerializationTest, we could duplicate that somehow for this class but that doesn't seem too useful.

The only other idea I'd have is serializing the object and checking the length of the string or contents in it or similar tricks.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs tests

@Berdir ok I'm happy to proceed here then.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 9e0ae4b to 8.3.x and 410c1ff to 8.2.x. Thanks!

  • alexpott committed 9e0ae4b on 8.3.x 
    Issue #2831525 by Berdir: Add DependencySerializationTrait to...

  • alexpott committed 410c1ff on 8.2.x 
    Issue #2831525 by Berdir: Add DependencySerializationTrait to...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.