Remove E_STRICT notice in mysqli extension

[Girgias]

Split mysqli error conversion from #4401

[nikic]

I'm not sure on this one. I feel it might make more sense to drop the E_STRICT instead. Basically it tries to force you to write

for (; $mysqli->more_results(); $mysqli->next_result()) {
    // ...
}

instead of

do {
    // ...
} while ($mysqli->next_result());

But probably there's a reason why the former version is recommended (something about error handling?), maybe someone with MySQL familiarity can comment. @johannes?

[johannes]

I believe that is consequence of the original developer staying close to C patterns instead of going for PHP patterns.

I am not sure what state the result is in after next_result failed. Whether it still refers to the last result set or is in some form of invalid state.

[Girgias]

So I'm a bit confused about this code tbh, I've had a look around it trying to understand it but it seems this section calls the mysqlnd driver. I would have thought that the mysqli and mysqlnd extensions/drivers operate independently and that one can use mysqlnd without having mysqli is this the case?

It is true that the E_STRICT notice should probably be dropped. I'm going to commit/push that change and see if it breaks anything on CI.

[cmb69]

@Girgias, ext/mysqli works either with the mysqlnd driver, or with libmysqlclient.

[johannes]

mysqlnd is the "library" providing a C API for other PHP extensions for handling MySQL protocol.

mysqli, pdo_mysql and historically ext/mysql are built on top of it and provide user facing APIs. mysqlnd is built as PHP extension to ease management, while itself it doesn't provide any APIs to PHP users (well, some ini settings etc. but nothing fundamental)

See https://www.php.net/manual/en/mysql.php which has some more details.

Feel free to ping my outside this discussion for question (same username as here @ php.net, just to stay in topic within this item)

[johannes]

If you drop it, would be good to check what happens if you call result APIs after next_result returned false.

Something like

$m->multi_query("SELECT 1"); // returns only one result set
$m->next_result(); // should return false
$m->store_result(); // now what happens here!?
$m->fetch_assoc(); // and here!?

I am not sure what to expect, I think all those later operations should fail gracefully ... They shouldn't return the first result set. On the other hand if they for whatever reason return the first result set it might be good to enforce the has_more check to prevent bad mistakes (or we have to look for a way to catch that)

(Untested and typed on mobile ...)

[Girgias]

Thanks for the explanations :)

Will add a test to see if it fails gracefully or not and will report back.

[Girgias]

Okay added a test, this should work.

I couldn't use fetch_assoc() as this works on a mysli_result class which is provided by mysqli::use_result(), I don't think these functions need to be tested on the result but can always do if it seems necessary.

[Girgias]

@johannes is the added test sufficient or should I do something more specialised?

[nikic]

For reference, this is the usage example from the MySQL docs: https://dev.mysql.com/doc/refman/8.0/en/c-api-multiple-queries.html They use a do/while loop on mysql_next_result.

[nikic]

Looking at the results on your added tests, it seems like @johannes conjecture was right and the first result set is returned, instead of subsequent result set operations failing.

[johannes]

Now the question is: Is that ok?

The easy answer is "yes, if you ignore the return code it's your fault" as long as it doesn't crash we are fine.

Another answer might be "no, all subsequent result operations should fail"

I'm not sure.

[Girgias]

Now the question is: Is that ok?

The easy answer is "yes, if you ignore the return code it's your fault" as long as it doesn't crash we are fine.

Another answer might be "no, all subsequent result operations should fail"

I'm not sure.

I mean it was always possible to do that if you ignored the E_STRICT warning so I would say leaving it as if would be the better option. I don't know if this should be changed in PHP 8? So should I replace that by a deprecated notice?

[johannes]

The difference is: The "old" suggestion is "check using has_more and then switch using next_result without having to check the return value" and now we are going to tell users "use next_result only".

The question is what the livelihood of errors is. i.e. if users issue a CALL to a stored procedure and they misinterpret the result ... will the (slightly) nicer code bring more benefits or will this cause more bugs. I tend to "most ignore E_STRICT anyways, so improvement wins" but I want to everybody to think about it for a moment.

[cmb69]

will the (slightly) nicer code bring more benefits or will this cause more bugs

It seems to me that do {} while ($mysqli->more_results() && $mysqli->next_result()) and do {} while ($mysqli->next_result()) produce identical results, so the additional check wouldn't have any benefits. (@nikic's for loop is bogus, since it would skip the last result set.) However, $mysqli->more_results() does not necessarily imply $mysqli->next_result() succeeds, which could be utilized to avoid checking $mysqli->errno.

Anyhow, would the code work the same with libmysqlclient?

[nikic]

@cmb69 Uh yeah sorry, my code doesn't make sense.

I think in the end the two relevant use cases here are a) you are reading results in a loop, in which case just using next_result() would be preferable and the warning should go away or b) you are reading a specific (known) number of result sets without checking the next_result return value, in which case the warning might help you realize your mistake.

[cmb69]

you are reading a specific (known) number of result sets without checking the next_result return value, in which case the warning might help you realize your mistake.

Consider the following:

$conn->multi_query("SELECT 1;SELECT * FROM missing_table");
var_dump($conn->next_result());
var_dump($res = $conn->store_result());
var_dump($res->fetch_array());

This fetches the result of the first query without any warning/notice. I don't think the internal more_results() check triggering a warning/notice is actually that helpful, since users need to check the return value of ::next_result() anyway.

Anyhow, would the code work the same with libmysqlclient?

yes

[Girgias]

@johannes @cmb69 @nikic is it possible to land this? Or are still some open issues/question that may need to be resolved?

[nikic]

Consensus seems to be on dropping the E_STRICT notice, so let's do it.

[Girgias]

Merged in as e895e96