Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUGFIX: ArrayObjects are mapped correctly without PHP internal properties #2783

Merged
merged 1 commit into from
Mar 28, 2022

Conversation

kdambekalns
Copy link
Member

@kdambekalns kdambekalns commented Mar 28, 2022

Until now, ArrayObjects would be mapped to array with a structure like this:

array(4) {
  ["arrayCopy"]=>
  array(1) {
    ["foo"]=>
    string(3) "bar"
  }
  ["flags"]=>
  int(0)
  ["iterator"]=>
  object(ArrayIterator)#10459 (1) {
    ["storage":"ArrayIterator":private]=>
    object(ArrayObject)#10460 (1) {
      ["storage":"ArrayObject":private]=>
      array(1) {
        ["foo"]=>
        string(3) "bar"
      }
    }
  }
  ["iteratorClass"]=>
  string(13) "ArrayIterator"
}

This is because getGettablePropertyNames() would return the internal properties which have matching public get* methods on the ArrayObject PHP class https://www.php.net/manual/en/class.arrayobject.php#arrayobject.synopsis

This adds an ArrayObjectConverter that allows to convert to a plain
array containing only the intended custom properties. It uses getArrayCopy() to get the job done.

Fixes #2041

This adds an ArrayObjectConverter that allows to convert to a plain
array. It uses `getArrayCopy()` to get the job done.

Fixes neos#2041
@kdambekalns
Copy link
Member Author

Created this against 6.3 even though it's a FEATURE. The original issue is something I consider a bug, if you agree we could sneak it in as a bugfix. If not, I'll re-target master.

Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

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

+1 by reading

This seems like a clean and b/c way to solve the issues.
But I abstain from the target branch discussion and leave that to the RMs to decide

@albe
Copy link
Member

albe commented Mar 28, 2022

I'm personally fine with having this in 6.3 as a BUGFIX, since the original issue is buggy behaviour of the normal ArrayConverter with ArrayObjects. So I would mark this as BUGFIX and reference the illbehaviour of the old converter in the description unless @mficzel vetos

@mficzel
Copy link
Member

mficzel commented Mar 28, 2022

Am not sure, i would slightly prefer to target master but if you are sure this is safe why not

@albe
Copy link
Member

albe commented Mar 28, 2022

Well, this PR basically just adds a new converter that will be autopicked for ArrayObject sources and doesn't touch a single line of old code. So the only effect to existing code is that such objects now get type converted (and serialized) properly without all the internal properties that no one really wants to have anywhere.

@mficzel
Copy link
Member

mficzel commented Mar 28, 2022

As i said i am not against it as a bugfix. Seems to be safe enough. Feel free to merge.

@albe albe changed the title FEATURE: Add ArrayObjectConverter BUGFIX: ArrayObjects are mapped correctly without PHP internal properties Mar 28, 2022
@albe albe merged commit 0d3c4bf into neos:6.3 Mar 28, 2022
@kdambekalns kdambekalns deleted the feature/arrayobject-converter branch March 28, 2022 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants