WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 6 years ago

#5860 closed defect (bug) (fixed)

Activating plugin that uses upgrade functions (dbDelta) fails

Reported by: jhodgdon Owned by: ryan
Milestone: 2.5 Priority: normal
Severity: critical Version: 2.5
Component: Administration Keywords: has-patch
Focuses: Cc:

Description

Some plugins need to have their own database tables, and it's convenient for them to use the "dbDelta" function that is defined (currently) in wp-admin/includes/upgrade.php to maintain the tables (i.e. using the same method that the core of WordPress does to upgrade to new DB versions).

So, a plugin might include the following line in its activate function, in order to include the file that contains that function:

require_once(ABSPATH . 'wp-admin/includes/upgrade.php');

This worked fine through WordPress 2.3.x, but in 2.5 bleeding (tested in [6851]), including that file causes the following error to appear when you activate the plugin (I'll attach my very short test plugin as an attachment to the ticket):

Plugin could not be activated because it triggered a fatal error.

Fatal error: Cannot redeclare bugtest_test_activate() (previously declared in /opt/www/eclipsework/WPDev/wp-content/plugins/test.php:16) in /opt/www/eclipsework/WPDev/wp-content/plugins/test.php on line 17

Contrary to the error message, the plugin is activated, however, and functions correctly after that point. The problem is apparently that it is being included twice, and the second time, of course, the functions are already there and cannot be redeclared.

There needs to be a way for plugins to include the dbDelta function, without these activation errors. I'll investigate and propose a patch.

Attachments (2)

test.php (415 bytes) - added by jhodgdon 8 years ago.
Test plugin that causes this error to occur
schema-php.diff (493 bytes) - added by jhodgdon 8 years ago.
Patch that makes it possible to load dbDelta functions during plugin activation

Download all attachments as: .zip

Change History (18)

@jhodgdon8 years ago

Test plugin that causes this error to occur

comment:1 @jhodgdon8 years ago

Apparently, the problem is that the wp-admin/includes/schema.php file has a line near the top, that in 2.5 bleeding, calls the $wpdb->supports_collation() function.

Just putting that function in the activate function causes this problem to occur. I am not sure why, because when I copied the body of the function and did the same thing, that was OK. ????

  // this line is OK, so it isn't the $wpdb object itself...
  $tmp = $wpdb->prefix;

  // this line is OK -- and this is the function body
  $tmp = version_compare(mysql_get_server_info($wpdb->dbh), '4.1.0', '>=');

  // this line is OK
  $tmp = $wpdb->dbh;

  // but this line causes plugin activation to have an error
  $tmp = $wpdb->supports_collation();

In fact, when I put that last line at the top of my plugin file (outside the activate function), it also caused a fatal error (though I didn't get an error message). Any idea what might be happening, or why that would cause a fatal error, when the function body of the supports_collation function doesn't? It would be helpful if I had an error message, but it doesn't give me anything useful.

comment:2 @jhodgdon8 years ago

  • Cc jhodgdon added
  • Severity changed from normal to critical
  • Summary changed from In 2.5 bleeding, activating plugin that uses upgrade functions gives spurious warning to Activating plugin that uses upgrade functions (dbDelta) fails

I changed the title of this bug -- actually, the activation process is not working correctly (db functions are not being created).

I also changed this to a higher severity, because I think a lot of plugins that use separate DB tables use this method to keep them updated. And this used to work fine in 2.3; worked fine in 2.2 also, though you had to include a different file:

   if( file_exists( ABSPATH . 'wp-admin/includes/upgrade.php' ))
     require_once(ABSPATH . 'wp-admin/includes/upgrade.php');
   else
     require_once(ABSPATH . 'wp-admin/upgrade-functions.php');

comment:3 follow-up: @ryan8 years ago

The plugin is now being loaded within activate_plugin(). When upgrade.php is included, it is not in global context. We might need to sprinkle some "global" statements around.

comment:4 @jhodgdon8 years ago

Ah, that explains it!

I am now able to fix the issue with my plugin (the real one, which creates tables) loading. To do this, I had to add two global declarations in the plugin file (for variables defined at global scope and used within the activation function), and also one line to the top of schema.php saying:

global $wpdb;

I'll put in a patch for that, as it should be innocuous for normal use.

Thanks for the clue! I never would have figured out it was non-global scope causing the problems... this seems like a major change, but I think I understand why.

By the way, if there *is* an error during the calling of the plugin activation function, the plugin does get loaded twice. Do you want me to file a separate bug report on that? It means the error message is bogus (as in my report, above).

@jhodgdon8 years ago

Patch that makes it possible to load dbDelta functions during plugin activation

comment:5 @jhodgdon8 years ago

  • Keywords has-patch added

Added a small patch that lets the dbDelta function be loaded

comment:6 @darkdragon8 years ago

I feel like I'm back in C/C++.

I'm not seeing how the patch does anything that shouldn't already be available. I'm not sure that the patch fixes anything. If $wpdb isn't available then global $wpdb doesn't do anything except declare an variable that was nonexistent. If it did exist, then global $wpdb; should just acknowledge that the variable existed in global scope anyway.

That doesn't seem to be a solution and if it is, then you just turned everything I think I know upside down and I should stop programming completely because I don't know anything.

It seems unrelated, what is the execution flow? You said it includes your plugin twice, which it should not do and even if it does, you only execute the init once, so it should blow up on your function and not on the require_once().

In the case that it did execute the function twice, then it should still only include the file in require_once() one time, as would be the case with require_once(). If not, then you have a PHP bug with require_once().

Both the solution and the problem seem completely mind boggling and my brain is about to implode. Can you further explain, so that down is not up and reality is real again?

comment:7 in reply to: ↑ 3 @darkdragon8 years ago

Replying to ryan:

The plugin is now being loaded within activate_plugin(). When upgrade.php is included, it is not in global context. We might need to sprinkle some "global" statements around.


This is interesting. So a plugin including another file will also be within that functions scope, unless it was inside a function, then it would be within its own function scope.

That is quite fascinating. However, the scope of it would be instead inside the plugin function, so the global should be declared there and not in the core file.

comment:8 @ryan8 years ago

schema.php relies on the globals so they should be declared in schema.php. Declaring $wpdb and $wp_queries global at the top of schema.php seems like it would prevent these surprises. We've been bitten by $wp_queries a few times already.

comment:9 @ryan8 years ago

  • Owner changed from anonymous to ryan

comment:10 @darkdragon8 years ago

The solution is not found with adding global declarations to scheme.php, but in the functions that include scheme.php from within that function's scope. The only reason you would have to declare $wpdb global is if wpdb.php hasn't been included yet and $wpdb doesn't exist, or it is within function or method scope.

The short term solution of adding global definition (when the scope is already global) is hackish at best and points the source of the problem away from the root of the problem.

I would suggest that if it is a problem, then search for scheme.php in the files and write a patch which solves the problem (the better long term solution).

I think the best solution would be to just move that function out of scheme.php into functions.php since it is useful enough to plugin developers that it should be available without having to require_once() the file. I would love to have that function, but I usually build a custom solution myself.

comment:11 @ryan8 years ago

Fixing it for all callers by adding the global to schema.php is more defensive. We can fix this once and for all instead of every time it pops up. But, hey, I don't much care either way.

comment:12 @darkdragon8 years ago

I'm basically just playing devil's advocate here, so whatever decision is not going to affect me whatsoever.

Why not just put the rest of the code in a function? You would have to make $wpdb a global and it isn't going to call the code until it is needed. The overhead is slight. You would have to make those variables globals in the functions that call them.

My argument basically breaks down, because no one is going to be looking at scheme.php anyway. I would be more comfortable in which case, when a developer did look at that file that they knew the reason why $wpdb was being declared a global, so that they don't adopt the practice as their own.

I really hate it when the function body is split up into separate files (and isn't HTML), because it makes it difficult to (initially) follow the flow.

To be honest, I could basically care less, but the people that criticize will just have one more (minor) example to throw at the WordPress world. It is in my opinion, that the only ones that will be calling wp-admin/includes/scheme.php is WordPress, which means that that 1) there shouldn't be that many files and functions including the file and 2) that the better, or best fix will be worth implementing in WordPress.

Since well, I've bought this up and argued the issue, I might as well provide the patch.

comment:13 @ryan8 years ago

(In [6896]) Declare wpdb and wp_queries as global. Props jhodgdon. see #5860

comment:14 @ryan8 years ago

I did the least effort fix for now. We can clean it up later.

comment:15 @jhodgdon8 years ago

  • Resolution set to fixed
  • Status changed from new to closed

That fixes the bug described here, so I'm closing.

comment:16 @darkdragon8 years ago

  • Cc darkdragon added

Sorry, I'll provide a patch after I graduate. I don't have the time at the moment.

Note: See TracTickets for help on using tickets.