Serialize interface not reliable [#2071969]

We've been seeing weird serialization bugs on entity forms under PHP 5.4 in:
#2015697: Convert field type to typed data plugin for file and image modules (patch breaks on 5.4)
#2025451: Drupal\entity_reference\Tests\EntityReferenceAdminTest fails on PHP 5.4.9 and libxml 2.9.0 (HEAD already broken on 5.4 currently)

Those seem to happen when serializing field $item objects, that contain a reference to their FieldInstance.
It's not exactly clear what is happening, but moving Field & FieldInstance config entities from Serialize interface & serialize() / unserialize() methods to __sleep() / __wakeup() methods fixes the bug.

More generally, Serialize interface behaves subtly but critically differently between 5.3 & 5.4:
From http://www.php.net/ChangeLog-5.php#5.4.0:
"Added support for object references in recursive serialize() calls. (https://bugs.php.net/bug.php?id=36424)"

What this means is that PHP 5.3 fails to preserve object identity when the same object appears at different places that are handled by separate calls to the serialize() function - which is typically the case for objects that handle their serialization through Serialize interface: the serialize() method typically includes a standalone call to the serialize() function.

http://3v4l.org/D7HIa has a code snippet that directly shows the behavior.
On 5.3, references to the same object before serialization come out as references to different objects after unserialization.
On 5.4, "same object" before serialization comes out as "same object" after serialization.

So, I'm kind of skeptical about being able to use Serialize interface on a code base that is supposed to work on both 5.3 & 5.4, given that the change in behavior produces nasty subtle bugs...

Comment	File	Size	Author
#13	field_serialize-2071969-13.patch	4.56 KB	yched
#13
#13	interdiff.txt	1.32 KB	yched
#1	field_serialize-2071969-1.patch	4.21 KB	yched
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

yched CreditAttribution: yched commented 22 August 2013 at 16:52

Status:

Active

» Needs review

File	Size
field_serialize-2071969-1.patch	4.21 KB

Patch for Field & FieldInstance config entities. It's been reported to fix the 5.4 fails observed in the issues linked in the OP.

Comment #2

22 August 2013 at 18:13

Status:

Needs review

» Needs work

The last submitted patch, field_serialize-2071969-1.patch, failed testing.

Comment #3

yched CreditAttribution: yched commented 22 August 2013 at 18:28

Status:

Needs work

» Needs review

#1: field_serialize-2071969-1.patch queued for re-testing.

Comment #4

dawehner

German

CreditAttribution: dawehner commented 22 August 2013 at 20:53

Ouch!
Do you know whether there is explicit test coverage to also document why we need this here?

Comment #5

yched CreditAttribution: yched commented 22 August 2013 at 22:16

Honestly, no, I cannot really isolate a simple reproducible test case...

I still don't have a clear explanation why using the Serialize interface works in 5.3 and produces corrupt unserialized $form_state in 5.4. It's supposed to be the other way around (Serialize preserves object identity in 5.4 and breaks it in 5.3)...

Plus we don't have automated tests on 5.4, and I don't have easy access to a 5.4 environment, so it doesn't make things easy :-).

As to documenting why we need this *here*, well - my feeling is that it's not needed only here, and that we should stay away from Serialize if we plan to support both 5.3 & 5.4...

Comment #6

yched CreditAttribution: yched commented 22 August 2013 at 22:13

Also, note that #1977206: Default serialization of ConfigEntities has a patch to generalize the current serialization code from Field / FieldInstance config entities to ConfigEntityBase (because this logic is generic, really - serialize produces the same data that would be stored in CMI, and unserialize produces the same object that would be read from CMI).

Patch over there still uses Serialized interface, like Field / FieldInstance in current HEAD, but would need to move to __sleep() / __method() as well if we proceed here.

Comment #7

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 22 August 2013 at 22:14

It's 2 days out of date and I don't know when it runs, but https://qa.drupal.org/pifr/test/600303 has a PHP 5.4.17 test run...

Comment #8

yched CreditAttribution: yched commented 22 August 2013 at 22:18

Yes, but it only lets you test code after it has already been committed to HEAD, right ?

Comment #9

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 22 August 2013 at 22:47

Yes :(
Though if this is really a huge problem, we could probably get chx/jthorson to help us out. VDC got a testable-sandbox back in the day...

Comment #10

yched CreditAttribution: yched commented 22 August 2013 at 23:05

OK - I still wouldn't know what more to test, though...
For now things have broken on a corrupt $form_state for a node form with an image field or an entity_ref field. We do have tests for that, and they do fail in 5.4 currently. Without access to a 5.4 env, I have not tried to isolate a more "unit test"-like version of the bug.

Comment #11

yched CreditAttribution: yched commented 25 August 2013 at 23:18

Priority:

Major

» Critical

Bumping, #2015697: Convert field type to typed data plugin for file and image modules is postponed on this.

Comment #12

Berdir

German

Switzerland

CreditAttribution: Berdir commented 26 August 2013 at 07:54

Can we maybe open a follow-up to find a way to test this explicitly (not critical) and also open a php bug report to track this, maybe they know something?

Then I'm fine with getting this in, we already have implicit test coverage for this with the entity reference admin test and will soon have tons more with the file/image field type conversion.

This fixes the warnings in the currently failing EntityReferenceAdminTest on 5.4 but apparently not the final test fail, which doesn't seem to be related to this but some xpath stuff?

Comment #13

yched CreditAttribution: yched commented 26 August 2013 at 13:57

File	Size
interdiff.txt	1.32 KB
field_serialize-2071969-13.patch	4.56 KB

Opened #2074253: Fatal error when using Serializable interface on PHP 5.4 and added a @todo linking to that.

Comment #14

Berdir

German

Switzerland

CreditAttribution: Berdir commented 26 August 2013 at 14:54

Status:	Needs review	» Reviewed & tested by the community
Issue tags:		+PHP 5.4

Thanks!

This is somewhat ugly, but another step to unblock PHP 5.4 on our testbots, we have an issue to investigate further, this smells like a PHP bug, the behavior is too crazy to be by design, even for PHP (unserialized objects end up as FALSE or weird strings depending on which/how many properties are assigned).

Comment #15

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 28 August 2013 at 06:39

Status:

Reviewed & tested by the community

» Fixed

Huh. I learned something new today! :) Yay magic sleep. :P I'm comfortable committing this, given that we have a follow-up to dig into it further.

Committed and pushed to 8.x. Thanks!

I don't know that it's worth a change notice for this, given this behaviour may not last if we get to the bottom of what's screwing up.

Comment #16

yched CreditAttribution: yched commented 28 August 2013 at 14:40

Thanks @webchick.

So yes, the patch here hotfixed one case in Field system where Serialize caused problems. This still leaves the broader fact that Serialize interface, still massively used in the rest of core, behaves differently on 5.3 and 5.4, and thus seems like something we should avoid.