MySQLi support - undefined drupal_maintenance_theme()

big +1, however not tested yet (will do later today)

See duplicate issue: #24264.

Using that file, I get this error right away:

Parse error: syntax error, unexpected ',' in C:\Documents and Settings\tserbinski\My Documents\websites\drupal_cvs\drupal\includes\database.mysqli.inc on line 34

Doing some digging through the documentation, it looks like the mysqli_real_connect() is being used *improplerly*.

This function call works with no problems: @mysqli_real_connect($connection, $url['host'], $url['user'], $url['pass'], (substr($url['path'], 1)), $url['port'], null, MYSQLI_CLIENT_FOUND_ROWS);

If this is indeed the correct call (it appears to be but can't really test whether the ports, sockets part is working), then the code circa line 27 that checks for the port can be deleted since the function call handles this.

Additionally, the database check on line 51 needs to be moved up since the database is selected during the mysqli_real_connect() now.

Overall, code needs some work and cleanup.

Peformance wise, noticed a 10ms decrease in page generation times for a few pages (not thorougly tested performance wise however, just a quick and dirty observation).

Forgot to mention tested this with PHP5.0.3 and MySQL 4.1.11 on Windows 2003 SP1.

Thox, the reported duplicate issue is using an out of date database.mysql.inc file. I believe Drumm added quite a bit of code for friendlier errors. Seems those two files need to be merged for proper MySQLi support. I'll see what I can do later today if I have some time.

Ok I've taken the latest HEAD version of database.mysql.inc, merged this with NSK's recommendations, and then merged that with Thox's database.mysqli.inc (see link a few comments up) whose database code was based on out of an out of date version of the include file.

Also went through and tweaked and it looks like this patch is ready to go. Just tested it here and it is running *now* on average, 40ms *faster* than the regular mysql library.

Patch needs some more testing but I believe it is ready to go. Optionally, lines 27 can be removed and the mysqli_real_connect() can be updated with the $url['port']. No way to confirm if this will work 100% but we can get rid of some extraneous code if someone can confirm.

See attached file.

Updated status of feature request.

Tested the file -> however des not work with HEAD due to [1].
However with a slight change it works well.
I have attached the changed one (renamed so the content is not htmlscrambled)

[1] http://drupal.org/node/22911

Thomas this looks like the regular mysql file... nothing changed in this. Attached wrong file I am thinking?

your right. sorry, wrong file.

Great catch, working perfect now with latest HEAD. Any more testers, I think this code is just about ready to submitted to core.

From the performance point of view, MySQLi can be slower or faster than regular MySQL, depending on how ti is used.

According to this benchmark, using mysqli_stmt() has a performance edge over mysql_query(), which is in turn faster than mysqli_query().

NSK or Thomas can take a look and decide if it is worth so to do it.

tested - works for me.

oops. set to proper status

Although I do agree this code is working, we should in fact rewrite it to make user of the more advanced MySQLi techniques, notably mysqli_stmt() for *true* performance gains. Otherwise this patch doesn't really offer any incentives.

one step at a time, folks. prepared statements is a much more complex undertaking. there are benefits without using that feature. i quote from http://www.zend.com/php5/articles/php5-mysqli.php:

-----------------------
Beyond gaining access to the new features of MySQL 4.1+, why would anyone want to switch to using ext/mysqli?

In addition to the functionality mentioned above, ext/mysqli also has some other serious benefits:

* Greater speed. Enhancements in both the extension and in MySQL have made most operations faster, with certain operations becoming up to 40 times faster as compared to ext/mysql.
* Better security. In older versions of the MySQL RDBMS, the possibility existed for an attacker to extract weak password hashes from the network and then recreate a user's password. The new authentication procedure is much more robust and mirrors the attack-resistant authentication procedure of tools like SSH.

very true. i guess this patch then is indeed a good "first" step to get the initial, working functionality into core. then after that though, we do need to revisit the issue and update code to make better use of mysqli_stmt(), which will not only provide better performance, but much *better* security, and we could probably start to get rid of redudant code elsewhere in the code for such checks.

I spent 30 minutes reading about mysqli and it looks nice, although it is not clear whether the old (or current) mysql extension will continue to evolve, or whether that is going to be deprecated. Anything about this in the PHP roadmap? Such information would help value the importance of this patch.

As pointed out in the comments, the current Drupal mysqli backend has no advantage over the Drupal mysql backend. To me, it is not clear what potential performance improvements we are looking at or how this would affect our database API. Also, is any of this ANSI SQL? How is this going to affect the PostgreSQL backend/support?

I'd like to see more discussion, before adding this to core. At this point, I don't think the code is ready to be committed simply because it's not clear how we'll go forward afterwards. While working, Let's try to outline what is ahead.

As I understand this is just another database type, like mysql or pgsql. So you can use $db_url = 'mysqli://blahblah' and it implements normal db_* functions, am I right?

No, MySQLi is the *new* version of MySQL that is incorporated by default with PHP5. It cannot use the same MySQL functions because a lot has changed from 4.0 to 4.1 and this is future upgradable with the forthcoming 5.0 as well. It is my best guess that future versions of PHP will deprecate the current MySQL functions as the new MySQL 4.1 (and soon 5.0) becomes more and more widespread (both of which work with MySQLi only).

I don't understand, can I use it as $db_url = 'mysqli://blah' or not?

Yes you can use that to connect to a MySQL 4.1 database, *however* the code *will* be broken. regular mysql_ functions won't work, they to be mysqli_. Sorry for the confusion.

maverick, can you point us to additional information that states that mysqli is the default mysql extension for PHP5? Anything put forward by the PHP team on the issue of mysql versus mysqli? Such information is not critical, but I'd like to get good understanding of the situation.

Some extra insights from Goba: http://lists.drupal.org/archives/drupal-docs/2005-08/msg00076.html.

Either I don't uderstand what is this about, or you do not understand my questions.

Why mysqli support can't be made the same way as mysql and pgsql? They are all php extensions and might be (un)available.

Dries, what we are looking forward with mysqli instead of mysql is:

• You can only connect to a mysql 4.x or 5.x database with mysqli, unless you use old_password() to set the MySQL user password. The new password encoding in MySQL 4+.x is more secure, this is why it is pushed forward.
• People expect mysqli to be faster when dealing with mysql 4.x+, since it uses a new API (might use speedier stuff not supported by previous database clients). This is just something people assume, it would be nice to benchmark.
• The real plus in using Drupal with MySQL 4.1.x+ is that it finally supports real uft8 text handling, ie. sorting, searching, etc. This is IMHO where Drupal is looking forward. I don't know whether the old mysql client in PHP can utilize the full potential of the new charset features, since these might not be client dependent.

So the immediate pluses in MySQL 4.x+ are more secure passwords, real utf8 support and perceived faster operation.

Ok. Thanks for the additional information. From the looks we need mysqli support in Drupal 4.7. So let's focus on the technical aspects first. Is anyone opposed to introducing a new backend, or do some people prefer to add checks in the existing mysql backend? We should be able to commit this shortly.

I'd prefer to have it as another backend. reason: Drupal might be moving to mysql 4 and we might want a mysql3 backend or Drupal continues to support mysql 3 and somebody wants a mysql4 backend (not that it would help too much).
Also, a new backend is much cleaner and you avoid a lot of if-thens in the course of executing a page.

Separate backend +1. It is much easier to maintain as well as it is much more performant (ie there is no need to load and continually skip unused code).

It's simply not a question to have a separate backend or not. It's absolutely non-drupal to have two backends in one .inc.

With a separate mysqli included file, the administrator would have to specifically require their Drupal installation to use mysqli. This could cause some confusion for people who upgrade from php4 to php5 and their compilation only provides mysqli in php5, they may not even know about mysqli and wonder why Drupal doesn't work after they upgraded to php5, until someone informs them that they have either to recompile with mysql or change their settings.php in Drupal. Good documentation could help with this, of course.

So we only need to inform them...

IMO it's important enough that if you're confused as a user you should be, because then you start wondering what has changed. If you maintain a site and upgrade essential tools like mysql and php it's only normal to know what is going on.

For users who don't or can't maintain their own servers we should provide some information on the system requirements page and in the install.txt

Well, we can include an 'compatibity_test.php' script inside the main drupal distro, which checks if all things which are required are present? (PHP-version, compiled with MySQL, etc)
And after that does the same test for optional requirements like GD, Mod_Rewrite, mbstring, etc...

How about that?

Stefan

Now correct me if I'm wrong, but can't we just stick to how the original patch is working, e.g, create a new file database.mysqli.inc and then just change the $db_url string in settings.php so it knows to pickup up mysqli instead of mysql? This seems to be working great.

As of a side note, on Windows, it is *much* easier to run the MySQL and MySQLi extensions in parallel with MySQL 4.1 ... that is how I'm currently running my dev machine and I can switch back and forth between regular database.mysql.inc and database.mysqli.inc just by changing the $db_url in settings.php.

It would be my best recommendation to keep this approach for a few Drupal release cycles (say up to 5.1/5.2) and then deprecate the database.mysql.inc file and then eventually remove it as PHP5.1 (forthcoming) and MySQL 4.1 become dominant default installs.

compatibity_test.php or something along the lines is going to be part of the install system anyway, as far as I know.

I prefer separate backend too. I think the auto-switching between mysql and mysqli is great and easy to implement. Just create an if() block right before we include database.x.inc. That happens in db_set_active() [line 112 of includes/database.inc]. The if should look like:

// automatically use mysqli if loaded. it is more secure: http://www.php.net/mysqli 
if ($db_type == 'mysql' &&  extension_loaded("mysqli")) {
  $db_type == 'mysqli';
}

Sorry, I can't reroll the patch and re-test at the moment.

Ok here is a patch that fixes some references to mysql and adds mysqli as well.

This is the patch from comment #13 that has been tested and working great on WinXP with PHP 5.0.4 and MySQL 4.1.11. Sorry couldn't get both of these patches to roll into one.

Also, I see no *reason* to include code to auto detect MySQLi over MySQL unless we add support to do this for PostgreSQL as wel. Otherwise that isn't consistent. It is clear in the settings.php that either mysql, mysqli, or pgsql can be used to setup a database. Easy enough.

Correction, that last file should be placed in includes/ as database.mysqli.inc. Then just edit $db_url in settings.php to make use of it and switch from MySQL to MySQLi support.

Further testing does show one *big* break in Drupal, updates.inc.

Updates.inc is testing to see if ($GLOBALS['db_type'] == 'mysql'). If you set the the $db_url to mysqli, *no* updates work. The code will be the same for mysql and mysqli, so I think $GLOBALS['db_type'] needs to be ammended so that it accounts for both mysql and mysqli connections (since they are essentially the same in respect to SQL code).

if (substr($GLOBALS['db_type'], 0, 5) == 'mysql') big problem fixed.

+1 on chx's big problem fix :)

+1 to that fix as well :) Just need to make note of that in any furture database updates after MySQLi support is added.

Please add a helper function for that to the update script. Something like using_mysql() or so. I'll commit this patch as soon update script is taken care of. Great work.

Ok here is an updated patch that patches updates.inc as well. I put a check at the top that assigns $GLOBALS['db_type'] to it's only local var $db_type and then uses chx's above recommendation to make the fix. Works great!

Use this patch in conjuctions with the above database.mysqli.inc file!

Fixed a typo.

I don't see how you can use global variables like that; $db_type will be undefined in the update functions.

db_connect() should not return false -- the existing db_connect() functions appear to do their own error handling/reporting. Maybe the PHPdoc needs to be updated.

(On a related note, there appears to be quite a bit of duplicated code in the error handling. A couple weeks ago, when the Drupal database server was down, these database error handling kicked in and we were sharing quite a bit of information -- like the hostname/IP of the database server -- with the rest of the world. Not sure that is a good idea.)

Don't see the problem with $db_type. Only reason I did that in updates.inc is to reduce redunancy. Other option would be to do:

if (($GLOBALS['db_type'] == 'mysql') || ($GLOBALS['db_type'] == 'mysqli')){

That is just so the correct SQL is used. Otherwise, the rest of the time $db_type needs to be mysqli and not just mysql.

Make sense? Comments on which approach is better?

Add a print "DB TYPE = $db_type"; to one of the update functions and see what happens ...

Ok well in order to get updates.inc to work correctly, I've come up with a few simple solutions instead of adding the overhead of another function.

Option 1 - simple and practical
if (($GLOBALS['db_type'] == 'mysql') || ($GLOBALS['db_type'] == 'mysqli'))

Option 2 - less overall code, more intial work to updates.inc. Basically invert the logic so instead of purely testing for MySQL we test for PostgreSQL and then if it isn't that, we assume it is either MySQL or MySQLi so the else catches both of these without having to test for each one like above.
if ($GLOBALS['db_type'] != 'pgsql')

Option 3 - better structure, future support of other databases

switch ($GLOBALS['db_type']):
  case 'pgsql':
     //PostgreSQL here
     break;
  default:
     //MySQL stuff here for both MySQL and MySQL

So what do you guys think is the best method? I'll write a new patch that fixes update.inc based on this so it will properly support MySQLi and I'll fix that include for Dries comments above. Thanks!

I prefer version 3 as it makes less hassle for adding more databases in the future.

I vote for the 'or' method. Allows for the expansoin to other DB's if needed (not that drupal has any plans to support anything other than mysql/postgres).
'mysql' || 'mysqli'

As long as you centralize the check, it really doesn't matter how it is implemented. For example,

// returns the base DB family so that code can ignore insignificant variations
function drupal_db_family() {
  global $db_type;
  switch ($db_type) {
    case: 'pgsql':
     return 'pgsql':
    case 'mysql':
    case 'mysqli':
      return 'mysql'
    default:
      return $db_type;
}

Ok here is a new patch that fixes references to MySQL (adding MySQLi). Also, included is a new comment about how to create new updates in update.inc after 147 to support MySQLi. This uses the switch statement, which after talking about with killes and drumm on IRC seems to be best bet and a better practice for future updates. Also makes adding new database types in the future less of a hassle in terms of updates.

Worked for me great. Remember that you must download and place database.mysqli.inc in your includes directory. I think this is much needed for future development.

Ok here is an updated mysqli file. I took out the following 3 lines:

  if (!function_exists('mysqli_init')) {
    return false;
  }

No reason this should be here. If the user is going to use MySQLi then they have this function installed... if not, then they won't use it. Now it doesn't return false as Dries pointed out as well :)

Dries said:

(On a related note, there appears to be quite a bit of duplicated code in the error handling. A couple weeks ago, when the Drupal database server was down, these database error handling kicked in and we were sharing quite a bit of information -- like the hostname/IP of the database server -- with the rest of the world. Not sure that is a good idea.)

Talked with drumm about this, he says it is on his whiteboard to do list. I do agree that in those cases it could be a security risk however, in most cases this is *very* helpful. Outside the scope of this patch though but something to consider and/or debate.

Just wanted to note a possible, well current problem with mysqli and PHP, depending on what version you
are using.

http://bugs.php.net/bug.php?id=33635&edit=1

http://bugs.php.net/bug.php?id=33772&thanks=6

Hopefully the PHP community will resolve these issues soon. However, seems those of us in the middle b/t 5.1b3+ must compile from source using the proposed patch (temp. fix) for mysqli.

Seems this problem is in the latest builds of PHP, including 5.0.5 and all the 5.1b3+ builds. Was working fine with 5.0.4.

For clarification, this does *not* break Drupal, it merely outputs a *warning* only and this a PHP specific bug.

Committed to HEAD. Thanks.

Applied patch and changed setting.php to receive these new error messages:

PHP Fatal error: Call to undefined function drupal_maintenance_theme() in /srv/www/htdocs/drupal/includes/database.mysqli.inc on line 52

Line 52: drupal_maintenance_theme();

Also, I took out the commas before, save one, before MYSQLI_CLIENT_FOUND_ROWS in order to move to the next error above, otherwise I receive this:

PHP Parse error: parse error, unexpected ',' in /srv/www/htdocs/drupal/includes/database.mysqli.inc on line 34

Line 34: @mysqli_real_connect($connection, $url['host'], $url['user'], $url['pass'], , , , MYSQLI_CLIE
NT_FOUND_ROWS);

Details:
PHP 5.0.4
MySQL 4.1.14
Drupal 4.6.3

The result is a blank page when trying to run index.php.

WiMax MySQLi support only works with Drupal HEAD (aka 4.7) I don't think it will work properly with 4.6.3.

I'm closing this issue since this patch doesn't apply to 4.6.3. If you still encounter this bug with the latest CVS, please reopen. Thanks!