d6lts / drupal

Fork of Drupal core for Drupal 6 LTS support.
https://www.drupal.org
GNU General Public License v2.0
130 stars 45 forks source link

Issue #3052207: Switch db_version_compare() to using $db_type rather than $db_url #38

Closed dsnopek closed 5 years ago

dsnopek commented 5 years ago

I haven't tested this yet, just putting up to show the idea.

Attempt to fix https://www.drupal.org/project/d6lts/issues/3052207

The issue is introduced by #35 - @f1mishutka do you have any ideas?

f1mishutka commented 5 years ago

I think it would be better to handle $db_url variable in more universal way in this case.

I'll ask for $db_url variable setup example at bug report page and will try to provide you a patch for it.

dsnopek commented 5 years ago

I look forward to your experiments! However, based on my quick reading of the code, I'm not sure there is a way to do it with $db_url.

We want to do the check against the current database connection, however, the function to change which connection is currently being used (db_set_active()) doesn't store the name of the active connection anywhere. So, I don't know how we'd know which key in $db_url to check. However, db_set_active() does change the $db_type to the correct value for the current connection, and I think that's all we really need for db_check_version() anyway.

In any case, I still haven't had a chance to test my PR or do any real experimenting. I'm open to anything that'll work. :-)

f1mishutka commented 5 years ago

David, take a look at db_set_active() please.

There is static variable $active_name in it. And it is set to the name of the connection from $db_url if it is an array.

My proposal is to make it global rather then static (and may be change it name since $active_name is too common, may be something like $db_active_name). Making its global will make it possible to use it in db_check_version() to select correct connection from $db_url array.

This should not affect any existing functionality and will solve this problem.

Let me know what do you think about this idea. If it sounds OK for you I'll prepare the patch.

f1mishutka commented 5 years ago

Sorry, forgot one thing: using $db_type here will not work. It is set just once during first connection attempt (if (!isset($db_conns[$name])) { ... }). If you use db_set_active() method again existing connection from $db_conns[$name] array will be used and $db_type will not be changed.

dsnopek commented 5 years ago

Ah, ok, I see about $db_type...

dsnopek commented 5 years ago

My proposal is to make it global rather then static (and may be change it name since $active_name is too common, may be something like $db_active_name).

I'm not crazy about the idea of adding a new global... But it'd be better than "fixing" $db_type which is another idea that went through my mind.

The most likely place a global would conflict on an existing site is in settings.php, but it would really only cause problems if there were depending on its value later in a custom module or something.

Ok, let's try this with a new $db_active_name global. @f1mishutka please start a PR or patch for this!

f1mishutka commented 5 years ago

started PR #40