Skip to content

[3.6] Regression: ArrayObject with protected ARRAY_AS_PROPS cannot be serialized anymore#39

Merged
weierophinney merged 7 commits intolaminas:3.6.xfrom
Slamdunk:array_object_as_props_bug
Dec 7, 2021
Merged

[3.6] Regression: ArrayObject with protected ARRAY_AS_PROPS cannot be serialized anymore#39
weierophinney merged 7 commits intolaminas:3.6.xfrom
Slamdunk:array_object_as_props_bug

Conversation

@Slamdunk
Copy link
Copy Markdown
Contributor

@Slamdunk Slamdunk commented Nov 29, 2021

Q A
Documentation no
Bugfix yes
BC Break no
New Feature no
RFC yes/no
QA yes/no

Description

In our app we serialize Laminas\Session\Storage\ArrayStorage objects as defined in its package:
https://github.com/laminas/laminas-session/blob/2.12.0/src/Storage/ArrayStorage.php
Which uses ArrayObject::ARRAY_AS_PROPS alongside a protected property $isImmutable.

Such serialization broke after #35 has been released in 3.6 minor.

The proposed test indeed passes on 3.5.x branch.

I hope to find a fix soon, in the meantime I'd like to know if such BC Break was intended or not, ping @weierophinney

Surely a BC Break that deserves a bugfix release, imho.

@Slamdunk Slamdunk force-pushed the array_object_as_props_bug branch from 8950302 to 3c31580 Compare November 29, 2021 14:38
… serialized anymore

Signed-off-by: Filippo Tessarotto <zoeslam@gmail.com>
Signed-off-by: Filippo Tessarotto <zoeslam@gmail.com>
@Slamdunk Slamdunk force-pushed the array_object_as_props_bug branch from 3c31580 to e330333 Compare November 29, 2021 14:42
Signed-off-by: Filippo Tessarotto <zoeslam@gmail.com>
Signed-off-by: Filippo Tessarotto <zoeslam@gmail.com>
Copy link
Copy Markdown
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have the context to review this, sorry.

I'd defer to @weierophinney, but you can pretty much assume that the BC break is not intentional, so a 3.6.x release is very much welcome, if it fixes the BC issue.


if (in_array($key, $this->protectedProperties)) {
throw new Exception\InvalidArgumentException('$key is a protected property, use a different key');
throw new Exception\InvalidArgumentException("$key is a protected property, use a different key");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow, these were changed by CS? :O

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's 8 years these LoC are here in this very form 😄

@Slamdunk
Copy link
Copy Markdown
Contributor Author

Slamdunk commented Nov 29, 2021

Here's the bug explained: child class properties are serialized before parent's ones.
So __set is called on child property before setFlags can be properly filled, in the 3.6.0 foreach loop.
This PR, just like 3.5 branch, sets ArrayObject very properties before any other __set call.

Copy link
Copy Markdown
Member

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a tricky order of operations issue, and you're correct; the change was unintentional.

I've got a suggestion for your proposed solution which should also remove some of the new Psalm errors that were introduced.

Slamdunk and others added 2 commits December 1, 2021 07:35
Co-authored-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Signed-off-by: Filippo Tessarotto <zoeslam@gmail.com>
Signed-off-by: Filippo Tessarotto <zoeslam@gmail.com>
@Slamdunk Slamdunk force-pushed the array_object_as_props_bug branch from a387ed9 to d844fcf Compare December 1, 2021 06:36
Signed-off-by: Filippo Tessarotto <zoeslam@gmail.com>
@weierophinney weierophinney merged commit 6fe0842 into laminas:3.6.x Dec 7, 2021
@Slamdunk Slamdunk deleted the array_object_as_props_bug branch December 8, 2021 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BC Break Bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants