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

Hotfix Stable solrting #664

Merged
merged 1 commit into from Jun 19, 2018
Merged

Hotfix Stable solrting #664

merged 1 commit into from Jun 19, 2018

Conversation

bpolaszek
Copy link
Contributor

@bpolaszek bpolaszek commented Jun 19, 2018

Fixes #663
Related to #657

@bpolaszek bpolaszek mentioned this pull request Jun 19, 2018
@goetas
Copy link
Collaborator

goetas commented Jun 19, 2018

Can you please explain the reason of this change?

@bpolaszek
Copy link
Contributor Author

@goetas Sorry, this is related to #663 - this PR fixes a different behavior between PHP versions, handlers wrongly sorted on PHP7+ and correctly sorted in PHP5.

@goetas
Copy link
Collaborator

goetas commented Jun 19, 2018

Can you please add some test (by having a big array of listeners)?

@bpolaszek
Copy link
Contributor Author

@goetas Not sure it's worth the work, since we're dealing with a PHP internal.

JMS\SerializerBundle\Tests\DependencyInjection\CustomHandlerPassTest::testSubscribingHandlerMustRespectPriorities() is already written for that, and keep in mind that with uasort, the result may, or may not, be what is expected.

Try out the following snippet (borrowed from CustomHandlersPass's internal code):

// A closure which will always return 0 because all items will have the same 'fixed_value'.
$sorter = function ($a, $b) {
    return $b['fixed_value'] == $a['fixed_value'] ? 0 : ($b['fixed_value'] > $a['fixed_value'] ? 1 : -1);
};

// Copied from CustomHandlersPass
function stable_uasort(array &$array, $value_compare_func)
{
    $index = 0;
    foreach ($array as &$item) {
        $item = [$index++, $item];
    }
    $result = uasort($array, function ($a, $b) use ($value_compare_func) {
        $result = call_user_func($value_compare_func, $a[1], $b[1]);
        return $result == 0 ? $a[0] - $b[0] : $result;
    });
    foreach ($array as &$item) {
        $item = $item[1];
    }
    return $result;
}


// Now, create an array with a fixed value and a random one.
$a = [];
for ($i = 0; $i < 1000; $i++) {
    $a[] = [
        'fixed_value'  => 'foo',
        'random_value' => mt_rand(0, 10000),
    ];
}


$c = $b = $a; // We now have 3 strictly equal arrays
var_dump($b === $a); // true
var_dump($c === $a); // true

// Use PHP built-in sorting function
uasort($b, $sorter); // $b is sorted, but $b is no longer equal to $a
var_dump($b === $a);// false


stable_uasort($c, $sorter); // $c is sorted, and $c is still strictly equal to $a
var_dump($c === $a);// true

Tested on PHP 7.2 and PHP 5.6. This confirms what CMB said: Stable sorting is not guaranteed with PHP sort functions, even since PHP7+.

I could write a test with 1000 subscribing handlers, sort them and still have them correctly sorted, the point is that PHP's uasort() is clearly unreliable, and in some cases like mine, it will fail.

The existing stable_uasort method is already working and tested with travis on PHP 5+. I have added no code, I just removed the PHP7+ exception.

If you want to be sure that all items are correctly sorted, only the stable_uasort function must be unit tested (then it could be subscribing handlers, foo/bars or fruits), but it's currently protected.

I hope you'll understand my point.

Thank you,
Ben

@goetas
Copy link
Collaborator

goetas commented Jun 19, 2018

alright!

@goetas goetas merged commit 22eb9a2 into schmittjoh:2.x Jun 19, 2018
@goetas goetas changed the title Hotfix #657 #663 - 2.x Hotfix Stable solrting Jun 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants