Skip to content
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

[mysqli] Add stubs for mysqli and mysqli_result classes #2295

Merged
merged 1 commit into from
Sep 29, 2023

Conversation

phansys
Copy link
Contributor

@phansys phansys commented Mar 20, 2023

No description provided.

@phansys phansys marked this pull request as ready for review March 20, 2023 23:47
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@ondrejmirtes
Copy link
Member

What are the types without the stubs added? Also, isn't this platform-dependent? Isn't the precision going to be annoying for someone who knows they're using a specific DB engine?

@phansys
Copy link
Contributor Author

phansys commented Mar 21, 2023

What are the types without the stubs added?

I'm not sure about this question. Are you asking about other missing properties that are defined for these classes?

Also, isn't this platform-dependent?

AFAIK, these types are the same for any platform. At least, the docs are not exposing a distinction regarding this.

Isn't the precision going to be annoying for someone who knows they're using a specific DB engine?

IIUC, these types are specific for the MySQLi extension, regardless the MySQL engine version.

@ondrejmirtes
Copy link
Member

What error are you trying to fix? How the tests fail if you remove changes in stubs?

@phansys
Copy link
Contributor Author

phansys commented Mar 21, 2023

This is the error I'm trying to fix.

@ondrejmirtes
Copy link
Member

Please add a regression test for that too.

@phansys phansys force-pushed the mysqli branch 2 times, most recently from 1633443 to a9ed75e Compare March 21, 2023 14:12
@phansys
Copy link
Contributor Author

phansys commented Mar 21, 2023

Please add a regression test for that too.

Done. Please, let me know if there is something else to address.

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

  1. The namespaces should be more unique. Something like namespace MySqlAffectedRowsType.
  2. Your playground example is about ReturnTypeRule, which means the test should be added in ReturnTypeRuleTest (for methods).

@phansys phansys force-pushed the mysqli branch 5 times, most recently from 6646023 to 1b5a4c4 Compare March 21, 2023 18:39
@phansys
Copy link
Contributor Author

phansys commented Mar 27, 2023

I've changed the namespaces and added the test to returnTypes.php.
I hope these changes are going in the right direction, but please let me know if there is something missing.

Thank you in advance.

@ondrejmirtes
Copy link
Member

Thank you!

@ondrejmirtes ondrejmirtes merged commit e3023e0 into phpstan:1.10.x Sep 29, 2023
410 of 415 checks passed
@phansys phansys deleted the mysqli branch September 29, 2023 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants