Conversation
The original implementation stored non-object values as PHP array keys, which silently coerce scalar types, causing type-distinct values to collide. Objects were tracked by `spl_object_hash`, so a string holding the same hash would accidentally match. The fix uses `SplObjectStorage` for object identity and `in_array` in strict mode for all other values, preserving type distinctions across scalars, arrays, and resources.
BastianLedererIcinga
left a comment
BastianLedererIcinga
left a comment
There was a problem hiding this comment.
I had discussed a similar approach using in_array() with @nilmerg, while working on the previous implementation, but we agreed on the hashmap to prioritize performance.
If you consider the added test cases necessary the previous implementation is of course not sufficient.
An idea I had when working on the original implementation was a separate cache per scalar type. It would certainly clutter the code a bit, but it would allow us to prevent quadratic runtime for these types, while keeping type-sensitive comparison.
If you think that's overkill I'm fine with that, but I'd like you to consider the idea, since we don't know where this function will be used in the future.
Sure it is slower. But does it matter? Do we call this method always with 1k+ items? Is it used in hot paths? If yes, we must of course change the implementation again. But I absolutely consider the test cases necessary, else this method is just a lie and breaks valid usages.
Do we have other code than Icinga Notifications Web that uses this function/could use this function? If not, I would not have introduced this method as a library function and would have implemented the limited version in the module. |
2 participants
The original implementation stored non-object values as PHP array keys, which silently coerce scalar types, causing type-distinct values to collide. Objects were tracked by
spl_object_hash, so a string holding the same hash would accidentally match.The fix uses
SplObjectStoragefor object identity andin_arrayin strict mode for all other values, preserving type distinctions across scalars, arrays, and resources.