-
Notifications
You must be signed in to change notification settings - Fork 809
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
Conversation
There was a problem hiding this 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. :)
It must return either nothing (which defaults to <constant>GEARMAN_SUCCESS</constant>) | ||
or a valid <link linkend="gearman.constants">Gearman return value</link>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably rephrasing this to:
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
.
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. |
There was a problem hiding this 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
This PR adds documentation for the
GearmanClient::setXXXCallback
methods.This includes:
setXXXCallback
methods should be called before callingaddTaskXXX
(see Gearman: setXXXCallback() needs to be called before addTaskXXX() #1369)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 whenGearmanClient::addTaskXXX
was called. As I have tested, otherwisenull
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.