Fixed bug #75448 - In case of failure, mysqli::prepare() returns NULL instead of FALSE

[morozov]

The patch is way larger than initially expected due to the following:

1. The MYSQLI_FETCH_RESOURCE macro which handles the situation of a closed connection is used by many other mysqli_* functions, so the change in the macro affects those functions as well. I reviewed the documentation articles for some of them, and none claimed that the function was expected to return NULL.
2. The failures in mysqli_class_mysqli_properties_no_conn.phpt showed that the values of mysqli class properties should be consistent with the return values of the corresponding functions, so I modified CHECK_STATUS and MYSQLI_GET_MYSQL as well.

[morozov]

I see this patch introduces new failures in the tests. Some functions like mysqli_autocommit() are tested to return NULL in case of failure, however, the documentation says it should also return FALSE.

[KalleZ]

Hi @morozov

Like I mentioned in the bug report, this changes all functionality of mysqli, as other methods/functions uses this check to that now will return false instead of null. Please fix the test to make Travis/AppVeyor pass in case the PR gets a greenlight

[morozov]

@KalleZ right now the master builds are failing both on Travis and AppVeyor. How do I make sure I fix all the newly introduced failures? Maybe I can temporarily rebase to some stable branch?

[…] while I understand that its easier to just have one common fault value to check, and I do agree it makes sense, it does introduce a minor BC break, while still minor, I'm still more reluctant to fix the docs.

Before I proceed with the tests, are we in agreement this change is acceptable? Otherwise, what would the fix to docs look like? Do we need to update documentation for every single affected method and add that actually it can also return null in the case of closed connection? Isn't it even more work to legitimize the existing inconsistent behavior?

Speaking of a potential BC break, what should the code look like to be broken by this change?

[KalleZ]

Hi @morozov

You can manually run the tests like [n]make test TESTS=ext/mysqli/tests or see what Travis/AppVeyor is complaining about here:
https://travis-ci.org/php/php-src/jobs/295177667#L3780
https://ci.appveyor.com/project/php/php-src/build/master.build.4481/job/93f9am4grvd9n8fb#L6633

If the mysql guys agree with this change, then the patch by a brief review looks fine. If we end up with docs having to be updated, then our docs are located in SVN (phpdoc under svn.php.net). In reality we have to update every single .xml file for every method this affects, but we can do some magic here on the docs perspective by defining an entity that can be used for copy/pasting. I can assist you if thats where we are headed.

Code that will be broken by this is code that is relying on specific NULL return values, tho I doubt there is many but it can happen any time you change the return value of something, hence why I want to be cautious :)

[morozov]

At the moment, the following tests fail on ZTS build on travis:

mysqli_begin_transaction() [ext/mysqli/tests/mysqli_begin_transaction.phpt]
mysqli_fetch_field() - flags/field->flags [ext/mysqli/tests/mysqli_fetch_field_flags.phpt]
Forking a child and using the same connection. [ext/mysqli/tests/mysqli_fork.phpt]
mysqli_num_rows() [ext/mysqli/tests/mysqli_num_rows.phpt]
mysqli_stmt_affected_rows() [ext/mysqli/tests/mysqli_stmt_affected_rows.phpt]
mysqli_stmt_num_rows() [ext/mysqli/tests/mysqli_stmt_num_rows.phpt]
mysqli_store_result() [ext/mysqli/tests/mysqli_store_result.phpt]
mysqli_use_result() [ext/mysqli/tests/mysqli_use_result.phpt]
Bug #41125 (PDO mysql + quote() + prepare() can result in seg fault) [ext/pdo_mysql/tests/bug41125.phpt]
Bug #44707 (The MySQL PDO driver resets variable content after bindParam on tinyint field) [ext/pdo_mysql/tests/bug_44707.phpt]
Bug #74376 (Invalid free of persistent results on error/connection loss) [ext/pdo_mysql/tests/bug_74376.phpt]
via [ext/pdo_mysql/tests/common.phpt]
	MySQL PDO Common: serializing [ext/pdo_mysql/tests/pdo_018.phpt]
PDO->beginTransaction() [ext/pdo_mysql/tests/pdo_mysql_begintransaction.phpt]
MySQL PDO->lastInsertId() [ext/pdo_mysql/tests/pdo_mysql_last_insert_id.phpt]
MySQL PDO->prepare(), emulated PS, anonymous placeholder [ext/pdo_mysql/tests/pdo_mysql_prepare_emulated_anonymous.phpt]
Bug #41876 (bindParam() and bindValue() do not work with MySQL MATCH () AGAINST ()) [ext/pdo_mysql/tests/pdo_mysql_prepare_match_against.phpt]
MySQL PDO->prepare(), native PS [ext/pdo_mysql/tests/pdo_mysql_prepare_native.phpt]
MySQL PDO->prepare(), native PS, named placeholder II [ext/pdo_mysql/tests/pdo_mysql_prepare_native_dup_named_placeholder.phpt]
MySQL PDO->prepare(), native PS, named placeholder [ext/pdo_mysql/tests/pdo_mysql_prepare_native_named_placeholder.phpt]
MySQL PDOStatement->rowCount() @ SELECT [ext/pdo_mysql/tests/pdo_mysql_stmt_rowcount.phpt]

Locally, on a ZTS build, I was unable to reproduce any of them. Some of them report memory leaks like:

========DIFF========
005+ done![Sat Nov  4 07:17:24 2017]  Script:  '/home/travis/build/php/php-src/ext/mysqli/tests/mysqli_begin_transaction.php'
005- done!
006+ /home/travis/build/php/php-src/ext/mysqlnd/mysqlnd_result.c(1380) :  Freeing 0x00007f281be91020 (139810243678208 bytes), script=/home/travis/build/php/php-src/ext/mysqli/tests/mysqli_begin_transaction.php
007+ /home/travis/build/php/php-src/ext/mysqlnd/mysqlnd_alloc.c(308) : Actual location (location was relayed)
008+ === Total 1 memory leaks detected ===
========DONE========
FAIL mysqli_begin_transaction() [ext/mysqli/tests/mysqli_begin_transaction.phpt] 

I think I could use some help figuring out how to fix them.

[morozov]

@KalleZ I've just managed to finish the patch. Could you take a look or suggest someone else who could?

[php-pulls]

Comment on behalf of pollita at php.net:

1c393e9