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

Added stable merge sort, for event sorting #45

Closed
wants to merge 1 commit into from

Conversation

dstockto
Copy link

This allows that events at the same priority level are called in the order they are added. #44 mentions this issue. It's not so much that it won't ever be right, it's just undefined. usort doesn't do a stable sort. This PR introduces a merge sort function which is stable so that the events are called predictably for identical priorities, that is called in the order they were added.

… at same priority level are called in the order they are added
@frankdejonge
Copy link
Member

@dstockto while this may seem to be the case, it's actually not true. The events are sorted using usort which doesn't care about the initial order the in which the array is. Also, if you care about the order of events being emitted, you should use priority instead or a different construct altogether to solve this. So apart from the priority you can expect the listeners to be called as if they were shuffled. To illustrate, you can run this in your terminal:

php -a
$range = range(1, 100);
usort($range, function () { return 0; });
var_dump($range);

The output of this won't be the same as the initial range.

Listeners of an event should not care in which order they're called. If one listener depends on another, a better solution would be to have some sort of pipeline operation in place which get's triggered when an event is emitted. Not only will ensure that your code doesn't care about when listeners are assigned, but an added bonus is that you've now made it explicit that procedure A should be invoked before procedure B is.

So instead of doing something like this:

$emitter->addListener('event', function () {
    echo 1;
});

$emitter->addListener('event', function () {
    echo 2;
});

$emitter->emit('event');

You'd be better off with something like (pseudo code):

$pipeline = new SomePipeline(...$operations);
$listener = new PipelineListener($pipeline);
$emitter->addListener('event', $listener);
$emitter->emit('event');

I hope this helps you out.

@dstockto
Copy link
Author

@frankdejonge this PR was to address the issue brought up in #44 which was that if there are multiple listeners added at the same priority, they are not called in the order they were added. Perhaps your reply is better there? It would have saved me an evening of wasted time working on something that appeared to be a legitimate gripe.

Your first paragraph says almost the same thing I did. Usort is not stable - that's mentioned in the PR description as well as in the source comments. I don't use this package, I was just going through repos looking for open issues I could address. Thank you for your comments.

@frankdejonge
Copy link
Member

@dstockto meant to place this there. I referenced this PR in the issue. Sorry you had your evening wasted on this.

@dstockto
Copy link
Author

@frankdejonge No worries, and sorry for replying here, not sure where else would be appropriate. Is there a way to know whether issues are either legit and something you want in the project or something you'd want a PR on? For instance the wildcard event listener one, could be a legit request but may also be something you just don't want in the project as well. I'd like to pitch in on this and/or other thephpleague projects but don't want to step on toes or waste time if it's not effort that's wanted. Thank you.

@frankdejonge
Copy link
Member

I've come back to this and ensured events are now emitted in order while respecting priority. Consider this fixed.

@dstockto dstockto deleted the stablesort branch January 28, 2016 06:17
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