php-serialize icon indicating copy to clipboard operation
php-serialize copied to clipboard

Deserialize PHP associative arrays as `new Map()`

Open miso-belica opened this issue 1 year ago • 4 comments

This way the order of the original PHP array is always preserved. It took me a while, sorry 😅

closes #293

miso-belica avatar Nov 10 '23 20:11 miso-belica

Hello @steelbrain I know it's not easy to find a time for PRs sometimes but this one is not so big so if you find some it would be great 🙂

miso-belica avatar Feb 25 '24 15:02 miso-belica

Hi @miso-belica! Sorry it took so long. I'll review this as soon as I can and get back to you. Appreciate the contribution

steelbrain avatar Feb 25 '24 16:02 steelbrain

@miso-belica There's some changes we're going to have to make to this before we can ship it, lmk if you want to make them on your own. I'd also be down to making them on my end. Anyway, here's the review requests:

  • Git seems to recognize the test files as binary, so add a .gitattributes file where we're forcing it to be text
  • Since it's a behavior-breaking change and would require semver-major update, let's make preserveOrder false by default, and opt-in. Maybe it can be true by default in the next semver-major release
  • [].reduce(...) with objects being merged has been described as inefficient, I'd like us to move away from it. See https://prateeksurana.me/blog/why-using-object-spread-with-reduce-bad-idea/
  • Instead of modifying existing tests, I'd like us to keep them in place and just add new tests instead

steelbrain avatar Feb 28 '24 00:02 steelbrain

@steelbrain thank you for the article and other hints. Recently I was blessed and was able to remove WP from the project so I hope I will remove this library soon :) But, thank you for it. It helped us to maintain the project for years. Feel free to make any modification you find useful to merge the code. I hope it helps prevents some bugs to people stuck with WP in their project.

miso-belica avatar May 02 '24 07:05 miso-belica