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

Docs: full review of Hook documentation #646

Merged
merged 1 commit into from
Feb 7, 2022

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Nov 25, 2021

Inspired by issue #220, I've:

  • Verified that all hooks listed in the documentation still exist in the codebase.
  • Verified the parameters for all hooks listed in the docs with the actual parameters in the hook calls in the codebase.
  • Verified that all hooks in the codebase are listed in the Hooks documentation page, with the exception of the transport.internal.... hooks.

Note:

  • The hook descriptions for the added hooks can probably use further improvement. I wasn't always sure how best to describe the hook event.
  • The ordering of this list looks to be a loose "requests hooks, curl hooks, fsockopen hooks and those in the order they are called" order.
    With some of the additional hooks, it wasn't always clear where to place them within this loose order, so suggestions to improve the order are welcome.

Also note that the naming of the request.progress hook seems inconsistent. I do understand why the hook uses request instead of requests, but this may be something to revisit at a later point in time.

@jrfnl jrfnl added this to the 2.x Next milestone Nov 25, 2021
@jrfnl
Copy link
Member Author

jrfnl commented Nov 25, 2021

Also related to #321

@schlessera
Copy link
Member

Hmm, still on the fence about this one, as I'm not yet sure whether the documentation for the optional $info args is sufficient here. It might be more robust to go with @johnbillion's suggestion and provide an $info array in all cases, with an empty one being used for non-blocking requests. I'll think about this some more...

@jrfnl
Copy link
Member Author

jrfnl commented Dec 8, 2021

Hmm, still on the fence about this one, as I'm not yet sure whether the documentation for the optional $info args is sufficient here. It might be more robust to go with @johnbillion's suggestion and provide an $info array in all cases, with an empty one being used for non-blocking requests. I'll think about this some more...

Those are two separate issues to address though. The documentation is issue 1, at least with this PR it would be "hook complete", which is much better then it is now.

Deciding whether or not to always pass $info is a different issue and if/when it is so decided, the documentation can be further updated to reflect that change.

Generally speaking I'm in favour of John's suggestion and would recommend for hooks to have the same number of arguments every time they are called, however, making this change now could be considered a breaking change as functions which "hook in" very likely will need updating (callbacks may have a different default value set - null - for the optional parameter from the empty array we would pass, callbacks may use isset() on the result of function_get_args() for this parameter, which would break, etc).

docs/hooks.md Show resolved Hide resolved
docs/hooks.md Show resolved Hide resolved
Inspired by issue 220, I've:
* Verified that all hooks listed in the documentation still exist in the codebase.
* Verified the parameters for all hooks listed in the docs with the actual parameters in the hook calls in the codebase.
* Verified that all hooks in the codebase are listed in the Hooks documentation page, with the exception of the `transport.internal....` hooks.

Note:
* The hook descriptions for the added hooks can probably use further improvement. I wasn't always sure how best to describe the hook event.
* The ordering of this list looks to be a loose "requests hooks, curl hooks, fsockopen hooks and those in the order they are called" order.
    With some of the additional hooks, it wasn't always clear where to place them within this loose order, so suggestions to improve the order are welcome.

Also note the the naming of the `request.progress` hook seems inconsistent. I do understand why the hook uses `request` instead of `requests`, but this may be something to revisit at a later point in time.
@jrfnl jrfnl force-pushed the feature/docs-improve-hook-documentation branch from 8a11961 to 807166b Compare February 7, 2022 16:21
@schlessera schlessera merged commit e86ac2a into develop Feb 7, 2022
@schlessera schlessera deleted the feature/docs-improve-hook-documentation branch February 7, 2022 16:38
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.

None yet

2 participants