Skip to content

Fix #1369 + #1370: adding GearmanClient::setXXXCallback documentation #3861

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

Merged
merged 6 commits into from
Oct 16, 2024

Conversation

cracksalad
Copy link
Contributor

@cracksalad cracksalad commented Oct 12, 2024

This PR adds documentation for the GearmanClient::setXXXCallback methods.

This includes:


In the PECL extension you can see, that all callback functions are called (equally) by this code snippet:

https://github.com/php/pecl-networking-gearman/blob/a52052cdd712a95091ce926be3bcdca41c730696/php_gearman_task.c#L32-L48

If you look closely, you might note, that task->zdata (which is called $context in PHP) will be provided if it has been set when GearmanClient::addTaskXXX was called. As I have tested, otherwise null is passed and no warning/error/exception is raised.

For some of the GearmanClient::setXXXCallback methods, there are already examples in this documentation mentioning, that those two arguments are provided.

@cracksalad cracksalad marked this pull request as draft October 13, 2024 08:06
@cracksalad cracksalad changed the title Fix #1370: adding GearmanClient::setXXXCallback callback args doc Fix #1369 + #1370: adding GearmanClient::setXXXCallback documentation Oct 13, 2024
@cracksalad cracksalad marked this pull request as ready for review October 13, 2024 10:57
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering the amount of times this text is repeated, I would recommend defining a new XML entity in language-snippets.ent or use XIncludes. :)

Comment on lines 42 to 43
It must return either nothing (which defaults to <constant>GEARMAN_SUCCESS</constant>)
or a valid <link linkend="gearman.constants">Gearman return value</link>.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably rephrasing this to:

Suggested change
It must return either nothing (which defaults to <constant>GEARMAN_SUCCESS</constant>)
or a valid <link linkend="gearman.constants">Gearman return value</link>.
It should return a valid <link linkend="gearman.constants">Gearman return value</link>.
However, if &null; is returned it defaults to <constant>GEARMAN_SUCCESS</constant>.

as void functions do return null.

@cracksalad
Copy link
Contributor Author

cracksalad commented Oct 15, 2024

Considering the amount of times this text is repeated, I would recommend defining a new XML entity in language-snippets.ent or use XIncludes. :)

As far as I understand, XIncludes would require relative pathes, which might be error-prone. In addition to that, the language-snippets.ent already contains entities for the Gearman extension. So I have added one there.


@Girgias If I summarize correctly, your other remark is about the void return type and the invalid union type (I guess "invalid" in this case means, that it is not valid PHP code, although it works fine in the docs). As I do not like the idea of expecting people to know that void functions actually return null, I tried to work around that.

@cracksalad cracksalad requested a review from Girgias October 15, 2024 13:28
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment about the structure of the entity, but LGTM otherwise

@cracksalad cracksalad requested a review from Girgias October 15, 2024 19:20
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@Girgias Girgias merged commit 6b53a02 into php:master Oct 16, 2024
2 checks passed
@jdkfx jdkfx mentioned this pull request Nov 10, 2024
11 tasks
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.

2 participants