Add ext/mysqli stubs

[tvlooy]

No description provided.

[carusogabriel]

@tvlooy Thanks for your contribution man. Could you please rebase with master?

[kocsismate]

@tvlooy I could only review very few functions/methods.

[kocsismate]

these are not nullable parameters (there is no ! in the ZPP). That said, the $name parameter's default value should be UNKNOWN AFAIK.

[tvlooy]

how can I know that it should be UNKNOWN?

[kocsismate]

There is some information written about it by Nikita: https://externals.io/message/106522 (the whole thread is interesting, but you should search for "UNKNOWN"). :)

[kocsismate]

@tvlooy Do you plan to continue working on these stubs in the foreseeable future? If so, I'll try to help you by reviewing the code, otherwise I can also continue it.

[tvlooy]

@tvlooy Do you plan to continue working on these stubs in the foreseeable future? If so, I'll try to help you by reviewing the code, otherwise I can also continue it.

I changed the docblocks like you asked. I rebased and sqashed commits. Tests still pass.

I might have underestimated this a bit. I have trouble figuring out the return types and need help with it. Maybe it is better if I hand it over to you.

[kocsismate]

@tvlooy You feel right, the return types are usually (much) more difficult to find out than parameter types.

What you have to look for in most cases is RETVAL_* (e.g. RETVAL_BOOL) and RETURN_* (e.g. RETURN_BOOL) macros, or sometimes the return_value variable itself. The first ones set the return type of the function (by setting the value of the return_value variable), the second ones additionally return. In order to determine the exact return type of a function, you have to take the return value of all branches into account (and that's where union types come into play).

There are lots of functions in mysqli, so I can understand that you feel it's too much. I'd suggest you to make smaller changes next time (by splitting the work) so it's easier for you when you try to learn how all these things work, and it's easier for the reviewers as well. 😎

My plan is to first work on the SPL (probably after the holidays) so that you have some extra time to decide if you want to proceed with the work.

[tvlooy]

Okay. Then I will do a full review this week.

[tvlooy]

@kocsismate I'm curious how the return_value actually works. ps: I'm reviewing but not done yet. Learning a lot. Thank you for your patience 😊

[cmb69]

@tvlooy, http://www.phpinternalsbook.com/php7/internal_types/zvals/basic_structure.html#setting-the-value gives an overview.

[kocsismate]

@tvlooy Take your time, I have plenty other stuffs to do :) You could have a look at this too https://github.com/php/php-src/blob/master/docs/parameter-parsing-api.md (if you haven't seen it yet), it also helped me a lot.

[tvlooy]

@kocsismate

questions about close():

• now that the null is gone, should this return true or bool in the signature?
• if it will only return true, why not just make that return nothing and change the method?

question about character_set_name():

• what does this return when cs_name is false?

[nikic]

* now that the null is gone, should this return true or bool in the signature?

For now, bool. We currently don't support true as a type, so these are typed as bool.

* if it will only return true, why not just make that return nothing and change the method?

In principle yes, but there's backwards compatibility considerations, so if we do this, it should be as a separate change.

[kocsismate]

what does this return when cs_name is false?

By default, the return value is null.

[tvlooy]

what does this return when cs_name is false?

By default, the return value is null.

I will change the signature for character_set_name() and also for get_client_info(), get_server_info() which seems to have the same case

[nikic]

What's the state of this PR, is it ready for review?

[tvlooy]

What's the state of this PR, is it ready for review?

@nikic @kocsismate the PR is updated and ready for review.
Do check if __construct() of mysqli_warning, mysqli_result and mysqli_stmt can be improved.

[kocsismate]

Really nice job! I could only find a few issues so far (I reached to mysqli_warning).

[kocsismate]

it's invalid PHPDoc (+ there is no parameter added in the param list), but as no exception is thrown in case a wrong object is provided, you should fallback to the object type declaration

[tvlooy]

Fixed docblock but didn't change method signature (things break if I do). What should be in the signature?

[nikic]

FYI I killed the mysqli reflection tests in 541f8b7. They are useless and just cause extra work.

[kocsismate]

what kind of errors are thrown when you add the parameters in the definition? 🤔

[tvlooy]

changed and it works with object for mysqli_result::__construct and mysqli_warning::__construct()

[tvlooy]

I get a "Fatal error: Arginfo / zpp mismatch during call of mysqli_stmt::__construct() in Unknown on line 0" when I change the signature to (object $mysqli_link, string $statement)

[tvlooy]

FYI I killed the mysqli reflection tests in 541f8b7. They are useless and just cause extra work.

rebased. Thanks for zapping the files

[kocsismate]

That's all the issues I could find. Mostly just a category issue with nullable return types. Huge work, @tvlooy!

[tvlooy]

That's all the issues I could find. Mostly just a category issue with nullable return types. Huge work, @tvlooy!

I should have spotted those return types. Thanks for the review!

[kocsismate]

@tvlooy There is one failing test left related to mysqli_insert_id(). The issue is that the MYSQLI_RETURN_LONG_INT() macro can return string as well, so please double check the return type everywhere where it is used.

[tvlooy]

MYSQLI_RETURN_LONG_INT

Fixed! And rebased

[kocsismate]

The PR looks good to me but I think Nikita should have a look at it too before merging. :)

[nikic]

I didn't comprehensively review this again, just added the missing mysqli_stmt::__construct signature and merged...

Thanks for your work on this!