Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Using php5, mysqli, and multiple named database connections via CiviCRM, I get intermittent errors from database.mysqli.inc complaining that $url['port'] is not defined. After some work, I tracked down this problem by using E_ALL and civicrm_error.module. It's possible to avoid this problem by specifying the port in $db_url, but I believe a more general fix is appropriate. That said, I've only seen this problem come up occasionally, and I haven't been able to figure out why it doesn't happen either all the time or never. I first saw this happen in 5.3 but I see that 6.x and HEAD have the same code and thus the same problem, so I'm assigning this to 7.x-dev, and if this is accepted I will open new issues tagged for 5.x and 6.x to fix this there as well.
I believe this is happening now because the behaviour of parse_url() changed sometime between 4.1 and 4.7 to ignore missing elements of a url, as opposed to returning NULL or an empty string.
This problem doesn't occur in database.mysql.inc, but I see that an equivalent problem was fixed for pgsql two years ago: http://drupal.org/node/19868
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | database.mysqli.inc_.patch | 736 bytes | mvc |
| #2 | database.mysqli.inc_.HEAD_.patch | 735 bytes | mvc |
| database.mysqli.inc_.patch | 747 bytes | mvc | |
Comments
Comment #1
Crell CreditAttribution: Crell commentedI've noticed this as well in D6, and an E_ALL error should be fixed in 6.
That said, I think you should use isset($url['port']) instead. It's faster than array_key_exists().
Comment #2
mvcCrell, why did you change the version tag of this patch? I realize the problem also exists in 6.x but after this is accepted against HEAD I'll submit patches for other branches.
That said, I accidentally created the original patch against 5.3, so I'm attaching one actually made against HEAD.
isset($url['port']) returns false if $url['port'] is defined but set to NULL, whereas array_key_exists() correctly returns true. In this particular case it's true that it wouldn't matter but I consider it bad form or at least confusing to use it to check for array values that may be NULL. Besides, array_key_exists() is already used many times in Drupal core (I see 23 instances in HEAD).
Comment #3
Crell CreditAttribution: Crell commentedChanging title to get it noticed properly.
There is no 7.x right now. 6.x IS HEAD. 7.x is a placeholder for "We'll get back to this later sometime", and tells the 6.x committers to ignore this issue.
I suspect that many of those array_key_exists() locations really should be isset(). I don't recall if that's a formal or informal standard, but isset() is a language construct so it's faster than array_key_exists(), which is a function. In this instance, both have the same overall effect so we should use the faster isset().
That said, it's a no-brainer to switch to isset() here, so I will mark this RTBC and let Dries/Gabor decide which they want to do. (But one or the other should be done, as an E_NOTICE is an error.)
Comment #4
Gábor HojtsyUh. Let's use isset() and also conform to coding guidelines with not omitting the brackets and the newlines.
Comment #5
mvcCrell, thanks for explaining the difference between the 7.x and 6.x CVS tags.
Gábor, I've made the changes you requested, so I'm changing this back to 'needs review'.
Comment #6
Crell CreditAttribution: Crell commentedBlargh. Sorry, Gabor. I should have caught that. Applies cleanly and answers both of your statements.
Comment #7
Gábor HojtsyThanks, committed. I also checked whether this applies to database.mysql.inc, but it does not, as a different connection method is used there. Also, database.pgsql.inc uses a different method, so this issue was limited to mysqli.
Comment #8
(not verified) CreditAttribution: commentedAutomatically closed -- issue fixed for two weeks with no activity.