php / pecl-networking-gearman

PHP wrapper to libgearman
https://pecl.php.net/package/gearman
Other
33 stars 25 forks source link

Fix return types in proto comments #24

Closed cracksalad closed 7 months ago

cracksalad commented 1 year ago

I have noticed, that some return types in the proto comments and in gearman.stub.php do not match with the actual return statements and changed the comments accordingly. I hope, I filled in a valid syntax. | and ? seem to be allowed and if constant values like true, false and null are also allowed, everything should be fine.

To test if the library still works, I basically ran all the commands from the readme:

Everything ran successfully.

Next I will do a PR for the manual pages for php.net.

esabol commented 1 year ago

If you look at the code for gearman_worker_return_code, just for example, the code can RETURN_NULL, but you have removed that from this function's proto and many others. Why?

Girgias commented 1 year ago

If you look at the code for gearman_worker_return_code, just for example, the code can RETURN_NULL, but you have removed that from this function's proto and many others. Why?

What version of PHP does the gearman extension support? As that's very relevant to if the RETURN_NULL is dead code or not.

cracksalad commented 1 year ago

The gearman extension v2.1 supports PHP 7.0 - 8.1.99 according to pecl.php.net and 7.0 - 8.0 according to the readme, although it also supports 8.2 as far as I have tested it. Still, are these null return values worth documenting if they occur only when you misuse the method's signature? Either way, the original point of this PR was to fix return types that were definitely missing/wrong. Depending on how the above question is answered, the third commit is to be reverted, which still leaves a lot of changes.

esabol commented 1 year ago

If you look at the code for gearman_worker_return_code, just for example, the code can RETURN_NULL, but you have removed that from this function's proto and many others. Why?

What version of PHP does the gearman extension support? As that's very relevant to if the RETURN_NULL is dead code or not.

The README says "PHP 7.0-8.0".

Girgias commented 1 year ago

If you look at the code for gearman_worker_return_code, just for example, the code can RETURN_NULL, but you have removed that from this function's proto and many others. Why?

What version of PHP does the gearman extension support? As that's very relevant to if the RETURN_NULL is dead code or not.

The README says "PHP 7.0-8.0".

Considering you can only get proper union type support and ReflectionType support as of PHP-8.0 not adding the UB case of ZPP failures is what makes the most sense to me.

esabol commented 1 year ago

Still, are these null return values worth documenting if they occur only when you misuse the method's signature?

I would say yes, but what is the norm with regards to other PHP extensions?

esabol commented 1 year ago

Considering you can only get proper union type support and ReflectionType support as of PHP-8.0 not adding the UB case of ZPP failures is what makes the most sense to me.

I don't know what any of that means, tbh. What I do know is that PHP 7.0 support is still important to me.

Girgias commented 1 year ago

Considering you can only get proper union type support and ReflectionType support as of PHP-8.0 not adding the UB case of ZPP failures is what makes the most sense to me.

I don't know what any of that means, tbh. What I do know is that PHP 7.0 support is still important to me.

Adding information to the stubs does not remove PHP 7 support.

Also every other extension does not document ZPP failure types

esabol commented 1 year ago

@Girgias wrote:

Adding information to the stubs does not remove PHP 7 support.

What I'm concerned is about is the removing of information though.

Also every other extension does not document ZPP failure types

Well, OK then.