Problem/Motivation

A fixture in a Layout Builder update path was causing unserialize errors only in PHP 7.3
See #3026698-44: Allow section storage to provide a more granular ID for tempstore

In order to write an update path, Layout Builder needed to provide a tempstore entry of a SectionStorage plugin.
This meant generating a serialized string representing that plugin.
After creating the serialized string, it was manually edited to change one value.
This caused no errors in PHP 7.2.
Also, this string was generated via a PHP script, not from within Drupal itself.
This may have affected the result.

Proposed resolution

Unserializing and serializing the fixture string from within a bootstrapped Drupal resulted in a slightly different string with the correct references intact.
This new string passes on both 7.2 and 7.3

Remaining tasks

N/A

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

CommentFileSizeAuthor
#2 3033691-serialize-2.patch420 bytestim.plunkett

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Active » Needs review
StatusFileSize
new 3033691-serialize-2.patch420 bytes

tim.plunkett credited tacituseu.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Title: Fix serialized Layout Builder fixture to work in PHP 7.3 » Serialized Layout Builder fixture is invalid in PHP 7.3
Priority: Normal » Critical
Issue summary: View changes

The error as reported by @tacituseu was 

Drupal\Tests\layout_builder\Functional\Update\TempstoreKeyUpdatePathTest::testRunUpdates
Exception: Notice: unserialize(): Error at offset 7543 of 10758 bytes
Drupal\Component\Serialization\PhpSerialize::decode()() (Line: 21)

The part of the serialized string at offset 7543 was
r:248
Now it reads
r:250

I have NO idea what this part means.
a is array, b is Boolean, s is string, O is object. r could be reference, or resource, but idk what the number means. Anyway, from what I understand of the upstream PHP bug is that 7.3 is much more strict about enforcing that the string must be identical as if you were to unserialize it and reserialize.

Raising to critical for now because it breaks PHP 7.3 tests

tacituseu’s picture

tacituseu commented

'r' means object reference (see: var.c, and var_unserializer.re)
Also, from https://bugs.php.net/bug.php?id=77302

[2019-01-22 19:57 UTC] dmitry@php.net
7.2 and below are affected by the same problem.
The old versions don't fail, but silently produce incorrect result (not the same as was serialized).

tacituseu’s picture

tacituseu commented
Related issues: +#3001920: Investigate PHP 7.3 workarounds

This rang a bell #3001920-25: Investigate PHP 7.3 workarounds.

tacituseu’s picture

tacituseu commented
Status: Needs review » Reviewed & tested by the community

The difference is that in fixed version:
$obj->data['section_storage']->context['entity']->contextDefinition->label is an object reference to $obj->data['section_storage']->context['entity']->contextData->definition->definition['label'], which is an instance of the class
Drupal\Core\StringTranslation\TranslatableMarkup containing string "Content", both in PHP 7.2 and PHP 7.3.

In the badly serialized version it is decoded by PHP 7.2 as a reference to the array
$obj->data['section_storage']->context['entity']->contextData->definition->definition[]
and PHP 7.3 catches that and throws an exception.

It is the only encoded reference in that serialization string, so I'd say it is a proper fix.

xjm’s picture

xjm
she/her
Primary language English
commented
Assigned: Unassigned » tim.plunkett
Status: Reviewed & tested by the community » Needs work

Discussed this with @tim.plunkett; I am comfortable with the fix based on that explanation. Tim will update the IS with a bit more detail. Once that's done this can be set back to RTBC and it's good to go!

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +PHP 7.3

Wow.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 3033691-serialize-2.patch, failed testing. View results

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Reviewed & tested by the community

Infra fail

  • xjm committed 0ab01bd on 8.7.x 
    Issue #3033691 by tim.plunkett, tacituseu: Serialized Layout Builder...
xjm’s picture

xjm
she/her
Primary language English
commented

Committed to 8.7.x. Thanks!

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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