Fixing Bug #79375

[kamil-tekiela]

This commit fixes the bug:
https://bugs.php.net/bug.php?id=79375
https://bugs.php.net/bug.php?id=77163
It also fixes side effects in PDO:
https://bugs.php.net/bug.php?id=79914

There was another PR submitted about a month ago that tried to address this issue in a different way and now has been made obsolete with the fix in mysqlnd

Marking this as a draft since I have not figured out how to throw an error from row-by-row fetching in mysqli. The PDO part of it seems to be working correctly but is missing a proper test case.

[cmb69]

cc @SjonHortensius

[SjonHortensius]

I have tested this PR and I can confirm it fixes #77163

I'm fine with closing the other PR and let this one pass instead

[nikic]

Why these changes? Isn't failure determined by null return?

We will not be able to merge this to stable releases with this change.

[kamil-tekiela]

In mysqlnd.h there was a ternary operator that I removed which was casting this value to a boolean.

#define mysqlnd_stmt_store_result(stmt)	(!mysqlnd_stmt_field_count((stmt)) ? PASS:((stmt)->m->store_result((stmt))? PASS:FAIL))

As far as I am aware the actual result of these functions was never exposed to mysqli or PDO_mysql. The result was meant to be stored inside of the object property so returning it didn't make much sense. If you grep for mysql_stmt_store_result you will see that the only time this function is used is in boolean context. Same with default_rset_handler which is only ever used in void context.
I agree that we could keep it as a nullable object type and then immediately cast it to boolean but why? What purpose does that serve? I am genuinely asking because I am no C experts so I don't know. Is there any memory consideration? Are internals of mysqlnd exposed to any other extensions? If there's no reason then I think it is cleaner to let that function return PASS/FAIL instead of using that ternary operator.

What does it mean that we will not be able to merge it into stable releases? Is there something that makes this unstable?

[nikic]

Are internals of mysqlnd exposed to any other extensions?

Yes, unfortunately. For this reason mysqlnd APIs are subject to ABI stability guarantees. We can make this change on master, but we cannot make it on PHP-7.4.

[kamil-tekiela]

It's fine with me to only push it to PHP 8. We can make a separate patch for PHP 7.4 where we only set the stmt->error_info and add DBG_RETURN(NULL);.
If you decide that it is better to leave the method signature alone, then we can do that too. As I said mysqli and PDO don't care about that function as they never access it directly.
Let me know what you decide. I can roll back that change and add the ternary back in.

[nikic]

I would prefer to first fix the issue and then do refactoring on master. It will be hard to merge this across branches otherwise. The diff currently also contains lots of unrelated changes that make it hard to see what you're actually changing. E.g. all the changes in ext/mysqli seem unnecessary? Please avoid changing whitespace on unrelated lines.

[kamil-tekiela]

I have no idea how to throw an exception from the iterator. There is no reference to mysqli object anywhere.

This test generates a very strange warning instead of the Out of sync exception:

<?php

mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
$gw03 = new mysqli('localhost', 'inet', '5432', 'test');

$r = $gw03->query('SELECT 1,2 UNION SELECT 10,15;', MYSQLI_USE_RESULT);
foreach ($r as $row) {
	$gw03->close(); // should cause Out of sync on the second iteration
}

[nikic]

This should be in a mergeable state now. The handling of errors when fetching rows directly on a result is not ideal, but that would require some more invasive changes that are best handled separately.

It would be good to also add some test coverage from the PDO side.

[nikic]

Added a PDO test and fixed a segfault that I encountered while writing it.

Do you see any further issues, or maybe things that need to be tested?

[nikic]

We also have some reports where the error occurs during commit: https://bugs.php.net/bug.php?id=76525 https://bugs.php.net/bug.php?id=66528

However, I'm having a hard time coming up with a test case for this. No matter what I try, the error always occurs before the commit already.

[nikic]

Merged as b03776a.