Various MySQLi issues (connection error handling, foreign port support, session_write_close) [#47919]

Hi,

I just wanted to get a fresh drupal head running with mysqli and encountered varoious problems:

- During the connection attempt if(!$connection) is checked to test for success. This is useless because $connection will exist (from mysqli_init) and thus this error will never get reported!
- The database is changed even though the constructor already selects the correct db.
- The host:port synax is used even though mysqli_real_connect has a parameter for that and will not work backwards compatible.

Those are easy, and I tried to fix with my best knowledge. Now the tricky part:

- The register_shutdown_function attempt to make session data be written before the mysql object is destroyed does not work (for me, php 5.0.5). I do not know why this does not work, maybe it is because mysqli has its own shutdown functions that are called before the custom shutdown functions. See [1] for easy reprocedure.

As mentioned in the documentation the way out of the chicken/hen problem are destructors. Because it is tricky to extend mysqli and then use mysqi_init / mysqli_real_connect I have attempted to fix this using a dummy object.

I have crafted a patch that fixes all those issues for me. I have tested the patch and it seems to work fine for me.

[1] http://bugs.php.net/bug.php?id=34377

Comment	File	Size	Author
#9	database.mysqli.inc_3.patch	1.74 KB	m3avrck
#9
#8	database.mysqli.inc_2.patch	1.62 KB	m3avrck
#8
#6	database.mysqli.inc_1.patch	2.56 KB	m3avrck
#6
#5	mysqli.porterror.patch	2.61 KB	Thomas Ilsche
#5
#3	mysqli.2006_0.patch	3.81 KB	Thomas Ilsche
#3
	mysqli.2006.patch	3.8 KB	Thomas Ilsche

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Dries CreditAttribution: Dries commented 4 February 2006 at 21:53

Not sure about the dummy object -- sure there isn't an alternative fix. The register_shutdown_function() solution seems more elagant.

Can somone confirm these problems and the proposed solution?

The code for the dummy class seems to be using tabs.

Comment #2

markus_petrux CreditAttribution: markus_petrux commented 4 February 2006 at 22:37

You might want to read this comment on the PHP manual. It seems safer to explicitly call session_write_close() at the end of the script, which is, btw, also needed to preserve modified session variables when using header('Location: ...').

Comment #3

Thomas Ilsche CreditAttribution: Thomas Ilsche commented 5 February 2006 at 21:43

File	Size
mysqli.2006_0.patch	3.81 KB

> Not sure about the dummy object -- sure there isn't an alternative fix. The register_shutdown_function() solution seems more elagant.

While register_shutdown_function() is really more elegant it does not work for php 5.0.5 (it does for 5.1.2). The reason seems to me that php is changing their shutdown behaviour from version to version, see main.c / void php_request_shutdown in the php cvs. The safest solution is calling session_write_close() at the end of index.php, depends what you like more.

> The code for the dummy class seems to be using tabs.
Fixed that.

Comment #4

m3avrck CreditAttribution: m3avrck commented 9 February 2006 at 06:50

Status:

Needs review

» Needs work

PHP 5.0.5 introduced a bug that prevented register_shutdown functions from working correctly. See my original post/patch here: http://drupal.org/node/36828 ... basically, PHP 5.0.5 *won't* work with MySQLi, either you need to downgrade to 5.0.4 or upgrade to 5.1.x ... because this bug occurs only in a very specific and now outdated version of PHP, I *don't* support that part of the patch.

However, the error checking seems like a valid point, so that would be useful in this patch.

Comment #5

Thomas Ilsche CreditAttribution: Thomas Ilsche commented 9 February 2006 at 15:15

File	Size
mysqli.porterror.patch	2.61 KB

Well attached is a patch just for error handling and non-default port (which I consider worst!)

Anyways, PHP Manual sais:

Write and Close handlers are called after destructing objects since PHP 5.0.5. Thus destructors can use sessions but session handler can't use objects. In prior versions, they were called in the opposite order. It is possible to call session_write_close() from the destructor to solve this chicken and egg problem.

This is why I suggest to stick to what the maunal proposes, using __destruct, this should also be safer for future versions.

Comment #6

m3avrck CreditAttribution: m3avrck commented 15 February 2006 at 17:50

Status:

Needs work

» Needs review

File	Size
database.mysqli.inc_1.patch	2.56 KB

Thomas, I took at the error handling and figured out a better way to test for error conditions using the actuall error number returned by MySQLi. With this new patch, MySQLi will work like MySQL now... if the database server can't be found, you get the appropriate error. If it can be found but the actual database can't be selected, you see that error as well. Working great now, nice find!

Additionally, I put in the port issue, that was another nice find as well.

As for the destructor thing, to the best of my knowledge, register_shutdown_function() would be called at the same time as __destruct() and therefore, works great. However, PHP 5.0.5 has an *explicit* bug with this function, as it isn't called when it should be, hence the problem you ran into. This was fixed in all versions after and before 5.0.5.

If things look and sound good, please set this commit, thanks!

Comment #7

Dries CreditAttribution: Dries commented 15 February 2006 at 21:40

Status:

Needs review

» Fixed

Last patch looks good. Committed to HEAD.

Comment #8

m3avrck CreditAttribution: m3avrck commented 17 February 2006 at 18:43

Status:

Fixed

» Reviewed & tested by the community

File	Size
database.mysqli.inc_2.patch	1.62 KB

Seems a few issues crept up with error reporting, this new patch catches the errors more broadly, preventing loads of warnings and Apache server crashes.