laminas / laminas-stdlib

SPL extensions, array utilities, error handlers, and more
https://docs.laminas.dev/laminas-stdlib/
BSD 3-Clause "New" or "Revised" License
190 stars 40 forks source link

Ensure PriorityQueue::extract also removes item from internal list #13

Closed weierophinney closed 3 years ago

weierophinney commented 3 years ago

The behavior of Laminas\Stdlib\PriorityQueue::extract() was such that it simply proxied to the underlying SplPriorityQueue implementation.

However, the class adds a number of additional behaviors such as count(), contains(), and hasPriority() that operate on an internal array of items instead of the SplPriorityQueue instance. As such, calling extract() would leave the PriorityQueue in an invalid state (count would not decrease, value would still be found, etc.).

This patch adds logic to extract() to find the matching value in the internal $items array with the highest priority and then remove it before returning the value.

Fixes #12

weierophinney commented 3 years ago

I've run tests locally against PHP 7.3, 7.4, and 8.0, both with latest and lowest deps. PHP 8 has 2 test errors (division by zero errors when testing the ErrorHandler), and 1 failure (a count() error when testing ArrayObject). The new tests pass, and tests run successfully on 7.3 and 7.4.

CS checks pass.

weierophinney commented 3 years ago

I think its covered with the unit tests but I tend to use PriorityQueue::toArray for comparison just to avoid issues.

It's definitely covered in the tests elsewhere. This is basically to mimic the behavior of the Laminas\Stdlib\SplPriorityQueue in the PriorityQueue wrapper for purposes of ensuring the additional functionality reflects the SplPriorityQueue state, so the salient tests are around count() and contains() behavior.