Skip to content

Conversation

@jrfnl
Copy link
Member

@jrfnl jrfnl commented Nov 25, 2021

Pull Request Type

  • I have checked there is no other PR open for the same change.

This is a:

  • Bug fix
  • New feature
  • Code quality improvement

Detailed Description

The $priority parameter in the register() method is intended to allow for prioritizing the order in which callbacks are run, however, this priority was effectively not used, so callbacks would be run in the order different priorities were registered, not ordered by the priorities.

This commit fixes this + adds a unit test to safeguard the fix.

I've elected to add the sorting to the dispatch() method, rather than the register() method for performance reasons.
Sorting arrays can be expensive and this way, only the subarrays of those hooks which are actually being run will be sorted, instead of sorting the subarray every time a new callback is registered, whether the hook on which the callback is registered will be called or not.

Fixes #452

Quality assurance

  • This change does NOT contain a breaking change (fix or feature that would cause existing functionality to change).
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added unit tests to accompany this PR.
  • The (new/existing) tests cover this PR 100%.
  • I have (manually) tested this code to the best of my abilities.
  • My code follows the style guidelines of this project.

Documentation

For new features:

  • I have added a code example showing how to use this feature to the examples directory.
  • I have added documentation about this feature to the docs directory.
    If the documentation is in a new markdown file, I have added a link to this new file to the Docs folder README.md file.

The `$priority` parameter in the `register()` method is intended to allow for prioritizing the order in which callbacks are run, however, this priority was effectively not used, so callbacks would be run in the order different priorities were _registered_, not ordered _by_ the priorities.

This commit fixes this + adds a unit test to safeguard the fix.

I've elected to add the sorting to the `dispatch()` method, rather than the `register()` method for performance reasons.
Sorting arrays can be expensive and this way, only the subarrays of those hooks which are actually being run will be sorted, instead of sorting the subarray every time a new callback is registered, whether the hook on which the callback is registered will be called or not.

Fixes 452
@jrfnl jrfnl added this to the 2.x Next milestone Nov 25, 2021
@jrfnl jrfnl requested a review from schlessera November 25, 2021 15:03
@jrfnl jrfnl mentioned this pull request Nov 25, 2021
@schlessera schlessera merged commit 4b892d6 into develop Feb 7, 2022
@schlessera schlessera deleted the feature/432-hooks-should-respect-priority branch February 7, 2022 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hook priority not working

3 participants