Skip to content

Conversation

@kamil-tekiela
Copy link

This PR removes call_user_func_array which was necessary in PHP 5. The references magic was only needed for call_user_func_array. The only reference needed is for rowBindedValues which is because PHP cannot handle references to properties the same way as it does with variables. In the future version, you can get rid of this completely after switching to get_result.

Copy link
Contributor

@HLeithner HLeithner left a comment

Choose a reason for hiding this comment

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

First thanks for your pull request. I added some comments.

I didn't tested if the reference with mysql statement object but tested it with user defined variable-length argument lists parameter which worked fine.

Can you elaborate what you mean with

after switching to get_result

@kamil-tekiela
Copy link
Author

Can you elaborate what you mean with

after switching to get_result

Right now, composer says that PHP 8.1 is the minimum required version. From PHP 8.2 the get_result becomes always available. Until then, you cannot make the switch as it could potentially break code for people who are running mysqli compiled with libmysqlclient.

Once you make the switch to PHP 8.2, you can replace the current code because you do no use result binding in reality. You just need it because there is no alternative. Using get_result will simplify the code and remove the workaround left by me.

@HLeithner
Copy link
Contributor

Can you elaborate what you mean with

after switching to get_result

Right now, composer says that PHP 8.1 is the minimum required version. From PHP 8.2 the get_result becomes always available. Until then, you cannot make the switch as it could potentially break code for people who are running mysqli compiled with libmysqlclient.

Once you make the switch to PHP 8.2, you can replace the current code because you do no use result binding in reality. You just need it because there is no alternative. Using get_result will simplify the code and remove the workaround left by me.

I think I still don't get you sorry, which function is new in php 8.2? get_result on the statement exists since 5.3 and no changes are in the function description. Also we use bind_result if I have checked the code correctly.

@kamil-tekiela
Copy link
Author

kamil-tekiela commented Oct 18, 2025

Can you elaborate what you mean with

after switching to get_result

Right now, composer says that PHP 8.1 is the minimum required version. From PHP 8.2 the get_result becomes always available. Until then, you cannot make the switch as it could potentially break code for people who are running mysqli compiled with libmysqlclient.
Once you make the switch to PHP 8.2, you can replace the current code because you do no use result binding in reality. You just need it because there is no alternative. Using get_result will simplify the code and remove the workaround left by me.

I think I still don't get you sorry, which function is new in php 8.2? get_result on the statement exists since 5.3 and no changes are in the function description. Also we use bind_result if I have checked the code correctly.

There is no new function. As you can see on https://www.php.net/manual/en/mysqli-stmt.get-result.php in the note box:

Available only with mysqlnd.

And if you recall my RFC that was implemented in PHP 8.2 https://wiki.php.net/rfc/mysqli_support_for_libmysql the feature to compile mysqli with libmysqlclient was dropped. So since PHP 8.2, you can safely use get_result() knowing that it will be available on all user platforms.

My whole point is that you don't need bind_result and your code only uses it because get_result might not have been available. In the future, you can refactor the code and remove bind_result.

@HLeithner
Copy link
Contributor

And if you recall my RFC that was implemented in PHP 8.2 https://wiki.php.net/rfc/mysqli_support_for_libmysql the feature to compile mysqli with libmysqlclient was dropped. So since PHP 8.2, you can safely use get_result() knowing that it will be available on all user platforms.

My whole point is that you don't need bind_result and your code only uses it because get_result might not have been available. In the future, you can refactor the code and remove bind_result.

ok, now I get it, thanks. would be useful for the next major release. If you like I to create a pr I can create a new branch for it.

@kamil-tekiela kamil-tekiela force-pushed the Improve-mysqli-parameter-binding branch from 7f3dce5 to 4f9485d Compare October 26, 2025 12:37
@richard67 richard67 requested a review from HLeithner October 26, 2025 12:45
@richard67
Copy link
Contributor

@HLeithner GitHub still shows that you requested changes, but the review comments are all resolved. Could you review again and approve if ok? Thanks in advance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants