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

Deserialize PHP associative arrays as new Map() #293

Open
miso-belica opened this issue May 1, 2023 · 2 comments · May be fixed by #295
Open

Deserialize PHP associative arrays as new Map() #293

miso-belica opened this issue May 1, 2023 · 2 comments · May be fixed by #295

Comments

@miso-belica
Copy link
Contributor

Hello,
in #291 serialization of the Map was solved. Thank you for PR and review and merge. But I think there is still an issue with deserialization of the PHP arrays which are currently returned as Object.

There is currently a problem with the order of deserialized array. JS orders numbers in Object by their value when iterating but PHP preserves insertion order. Using Map would solve this problem. For example as an extra option to use it or as a breaking change. The problem is at line 63 in

const pairs = parser.getByLength('{', '}', length => unserializePairs(parser, length, scope, options))
const isArray = pairs.every((item, idx) => isInteger(item.key) && idx === item.key)
const result = isArray ? [] : {}
pairs.forEach(({ key, value }) => {
result[key] = value
})
return result

  // this test fails currently
  it('orders correctly', async () => {
    const d1 = unserialize('a:4:{i:5;i:1;i:4;i:2;i:3;i:3;i:2;i:4;}');
    expect(Object.values(d1)).toEqual([1, 2, 3, 4]);
  });
  // I imagine something like this if you don't want to introduce a new major version with backward incompatibility
  // this is just one of many possibilities, important is to get ordered items back
  it('orders correctly using map', async () => {
    const d1 = unserialize('a:4:{i:5;i:1;i:4;i:2;i:3;i:3;i:2;i:4;}', {}, {toObject: 'object' | 'map' | 'pairs'});
    expect([...d1.values()]).toEqual([1, 2, 3, 4]);
  });
  // maybe alternative solution?
  it('orders correctly using map', async () => {
    const d1 = unserialize('a:4:{i:5;i:1;i:4;i:2;i:3;i:3;i:2;i:4;}', {Array: Map});
    expect([...d1.values()]).toEqual([1, 2, 3, 4]);
  });

If I maintain the library I would bump the major number and introduce a breaking change because I believe the only correct implementation is returning Map (principle of least surprise) but I understand a lot of use-cases are just using objects with string keys where it does not matter. We use Wordpress where often pairs of IDs are stored into DB and there it matters a lot. It would save us a lot of trouble if Map is returned in the first place with correct order of the keys so I may be biased by my personal experience 🙂

Thank you for considering any implementation.

@steelbrain
Copy link
Owner

I would be happy to review a PR for this that adds preserveOrder option to unserialize that is turned on by default (so maybe ignoreOrder?) so we're behavior compatible with PHP by default, it could be semver-major and be documented in README

@1valdis
Copy link

1valdis commented Jun 16, 2023

Very much interested in this. I have copied over the library and locally made a change to create a Map in place of array-object no matter what keys and order there is until this lands.

@miso-belica miso-belica linked a pull request Nov 10, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants