-
Notifications
You must be signed in to change notification settings - Fork 325
generate arginfo from stub for PHP 7 and 8 #463
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
|
Diff of reflection with PHP 7 Notices:
|
| Z_PARAM_ARRAY(entries) | ||
| Z_PARAM_OPTIONAL | ||
| Z_PARAM_LONG(expiration) | ||
| Z_PARAM_LONG(ignored) |
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.
Not in arginfo, and not used
| Z_PARAM_ARRAY(entries) | ||
| Z_PARAM_OPTIONAL | ||
| Z_PARAM_LONG(expiration) | ||
| Z_PARAM_LONG(ignored) |
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.
Not in arginfo, and not used
|
Fix also fix the test suite (only tests/*phpt) Pass with 7.3 and 8.0.0RC1 |
|
Reflection with PHP 8 - need to be carefully reviewed |
|
Still TODO (not planed for this PR): check need to promote some warning to exception, for consistency with php-src |
1685a49 to
e8f5377
Compare
|
This is great! How do the stub files get regenerated on changes? |
;) But you have to use PHP 8. You can also generate it manually using (this should work with PHP 7.2+, using gen_stub.php from php-src tree)
P.S. rather, the stub file is manually edited, the arginfo headers are generated. |
|
I buy it! Thanks for making this improvement! |
:)
welcome Can you please add the " |
|
|
||
| public function get(string $key, callable $cache_cb=NULL, int $get_flags=0): mixed {} | ||
| public function getByKey(string $server_key, string $key, callable $cache_cb=NULL, int $get_flags=0): mixed {} | ||
| public function getMulti(array $keys, int $get_flags=0): false|array {} |
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.
I've just accidentally come across this PR, and noticed the MEMC_METHOD_FETCH_OBJECT macro (mostly because PHP 8 throws an Error for most uninitialized objects, e.g.: https://github.com/php/php-src/blob/74fe9170b65740bcdc41c9706ec38c31654c12f6/ext/pdo/pdo_stmt.c#L37).
So the reason why I'm writing is that it seems to me that the return in MEMC_METHOD_FETCH_OBJECT is not reflected in the return types in the stubs. Unless I'm missing something, the macro can result in a null return value.
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.
Your are right and I think memcached should also raise an exception in such case (and thus, don't pollute arginfo with null case)
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.
@kocsismate see pr #465
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.
@remicollet Looks good, I agree that it's the best thing to do :)
Pros:
Cons: