Move query conditional helper functions to database.inc [#764418]

The complete error message as last theme install step

An AJAX HTTP error occurred. HTTP Result Code: 200 Debugging information follows. Path: http://head7.quiptime.lan/authorize.php?batch=1&id=14&op=do StatusText: OK ResponseText: Fatal error: Call to undefined function db_or() in /var/www/head7.quiptime.lan/modules/system/system.module on line 2105 Call Stack: 0.0005 170352 1. {main}() /var/www/head7.quiptime.lan/authorize.php:0 0.1471 13609168 2. _batch_page() /var/www/head7.quiptime.lan/authorize.php:153 0.1476 13613336 3. _batch_do() /var/www/head7.quiptime.lan/includes/batch.inc:81 0.1476 13613336 4. _batch_process() /var/www/head7.quiptime.lan/includes/batch.inc:163 0.1526 14200904 5. call_user_func_array() /var/www/head7.quiptime.lan/includes/batch.inc:286 0.1526 14200992 6. update_authorize_batch_copy_project() /var/www/head7.quiptime.lan/includes/batch.inc:0 0.1690 14506568 7. Updater->install() /var/www/head7.quiptime.lan/modules/update/update.authorize.inc:146 0.3580 14528520 8. ThemeUpdater->postInstall() /var/www/head7.quiptime.lan/includes/updater.inc:280 0.3581 14528304 9. system_rebuild_theme_data() /var/www/head7.quiptime.lan/modules/system/system.updater.inc:138 0.3831 14625328 10. system_update_files_database() /var/www/head7.quiptime.lan/modules/system/system.module:2427

The theme will be properly installed.

The main error is:

Call to undefined function db_or() in /var/www/head7.quiptime.lan/modules/system/system.module on line 2105

The attached patch fix the main error.

Comment	File	Size	Author
#14	764418-14.patch	2.44 KB	jpmckinney
#14
#8	764418-8.patch	452 bytes	jpmckinney
#8
	system.module.patch	553 bytes	quiptime

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

jpmckinney CreditAttribution: jpmckinney commented 7 April 2010 at 12:30

Status:

Needs work

» Needs review

CNR for test bot.

Comment #2

jpmckinney CreditAttribution: jpmckinney commented 7 April 2010 at 12:39

Although this patch may fix the main error, I don't think this is the best patch. Something tells me query.inc should have already been included by this point.

Comment #3

quiptime CreditAttribution: quiptime commented 7 April 2010 at 12:52

Something tells me query.inc should have already been included by this point.

That was exactly my first thought. But, the error exist.

Two functions call the system_update_files_database():

1. system_rebuild_module_data()
2. system_rebuild_theme_data()

Comment #4

jpmckinney CreditAttribution: jpmckinney commented 7 April 2010 at 12:54

Yes, I think query.inc should be included somewhere further up the call-stack, but I don't know enough about the updater system to know where.

Comment #5

quiptime CreditAttribution: quiptime commented 7 April 2010 at 15:35

Something tells me query.inc should have already been included by this point

That is a mistake.

To examine this, i've done the following test.

In the file authorize.php i have added this code: dsm(get_required_files());

// *** Real work of the script begins here. ***

require_once DRUPAL_ROOT . '/includes/bootstrap.inc';
require_once DRUPAL_ROOT . '/includes/session.inc';
require_once DRUPAL_ROOT . '/includes/common.inc';
require_once DRUPAL_ROOT . '/includes/file.inc';
require_once DRUPAL_ROOT . '/includes/module.inc';
require_once DRUPAL_ROOT . '/includes/ajax.inc';

// We prepare only a minimal bootstrap. This includes the database and
// variables, however, so we have access to the class autoloader registry.
drupal_bootstrap(DRUPAL_BOOTSTRAP_SESSION);

// This must go after drupal_bootstrap(), which unsets globals!
global $conf;

// We have to enable the user and system modules, even to check access and
// display errors via the maintainence theme.
$module_list['system']['filename'] = 'modules/system/system.module';
$module_list['user']['filename'] = 'modules/user/user.module';
module_list(TRUE, FALSE, FALSE, $module_list);
drupal_load('module', 'system');
drupal_load('module', 'user');

dsm(get_required_files()); // <<<

This indicates which files are includet - in the last installation step.

What is the result of the test?

1. If a module is installed then the file query.inc is included.
2. If a theme is installed then the file query.inc is NOT included.

For this reason, for me is clear: My patch is a solution of the error.

Comment #6

jpmckinney CreditAttribution: jpmckinney commented 7 April 2010 at 17:23

When I add the dsm and install a module through admin/modules/install inside the overlay, query.inc does not appear in the dsm.

Anyway, all I'm saying is that this bug is symptomatic of a deeper bug that should also be fixed.

Comment #7

quiptime CreditAttribution: quiptime commented 7 April 2010 at 17:58

For best test with dsm(get_required_files()), with and without overlay module, please put the dsm code line at the end of the file authorize.php.

Test without my patch:

- an module install relalize 4 "database-folder includes" - 2 x database.inc and 2 x query.inc
- an theme install relalize 2 "database-folder includes" - 2 x database.inc

The dsm output display an array. The number of keys of the array is different.

Test with my patch:

- an module install relalize 4 "database-folder includes" - 2 x database.inc and 2 x query.inc
- an theme install relalize 4 "database-folder includes" - 2 x database.inc and 2 x query.inc

The dsm output display an array. The number of keys of the array is the same.

Comment #8

jpmckinney CreditAttribution: jpmckinney commented 7 April 2010 at 19:31

File	Size
764418-8.patch	452 bytes

I wonder if this bug suggests that the registry ought to be updated when installing a theme.

Comment #9

jpmckinney CreditAttribution: jpmckinney commented 7 April 2010 at 20:45

I wrote a test for this issue in #693084-30: Regression: file_munge_filename() extension handling broken by move to File Field.

Comment #10

quiptime CreditAttribution: quiptime commented 7 April 2010 at 22:44

Patch #8 tested. Works.

Comment #11

sun

German

Karlsruhe

CreditAttribution: sun commented 9 April 2010 at 23:28

Component:	system.module	» install system
Status:	Needs review	» Reviewed & tested by the community

Patch looks good, can't hurt, if it solves the issue (didn't test myself). Re-assigning to install system to hopefully get some attention from the right folks.

Comment #12

David_Rothstein CreditAttribution: David_Rothstein commented 10 April 2010 at 05:15

Component:	install system	» database system
Status:	Reviewed & tested by the community	» Needs review

Hm, I think this actually might be a subtle bug in the database system that just happens to be revealed by the somewhat unusual authorize.php workflow...

In authorize.php we have already bootstrapped past the database stage by the time any of this code is reached. And if you look at how the database system is initialized in the bootstrap:

http://api.drupal.org/api/function/_drupal_bootstrap_database/7

the only file that is actually explicitly loaded is includes/database/database.inc. All others are intended to be loaded by the class and interface autoloaders (i.e. using data stored in the registry and letting PHP load the files automatically). That works fine as long as those other files contain only classes and interfaces - no procedural code. And for the most part they do. However, at the bottom of query.inc are these four procedural functions:

function db_or() {}
function db_and() {}
function db_xor() {}
function db_condition() {}

So if you happen to call one of these functions before any of the classes/interfaces in query.inc are used, I don't think they'll ever be available.

So if this is right, then the correct fix here is to simply move those four functions out of query.inc and into database.inc, and to document somewhere (if it's not already) that all procedural functions in the database API can only live in database.inc.

Comment #13

Crell CreditAttribution: Crell commented 10 April 2010 at 08:19

Title:	Bug fix: Install new theme: An AJAX HTTP error occurred ... Fatal error: Call to undefined function db_or()	» Move query conditional helper functions to database.inc
Status:	Needs review	» Needs work

David is correct. Originally those functions were put in query.inc on the assumption that if you needed them, you would already have called db_insert(), db_update(), etc, which in turn create an object of a class defined in query.inc, so they'd always be available. I guess that assumption is wrong. :-)

They're small enough that we can just move them over to the main database.inc file and be done with it. Since it looks like every page in Drupal ends up loading all of the database files anyway, there's really no performance gain to having those functions lazy load.

Changing title accordingly. Should be an easy patch.

Comment #14

jpmckinney CreditAttribution: jpmckinney commented 10 April 2010 at 12:55

Status:

Needs work

» Needs review

File	Size
764418-14.patch	2.44 KB

Moved functions into database.inc. Not sure where to put the comment that procedural functions in the database API can only live in database.inc.

Comment #15

Crell CreditAttribution: Crell commented 10 April 2010 at 19:04

Status:

Needs review

» Reviewed & tested by the community

Short, sweet, done.

I don't think we need to document that sort of thing in code comments. At least, there's nowhere else that we do so let's not get into that question here.

Comment #16

David_Rothstein CreditAttribution: David_Rothstein commented 10 April 2010 at 22:20

Patch looks good, and I tested that it works!

The documentation for this restriction should live in the @file declarations (although granted no one probably ever reads them) - however, that's a separate issue: #767690: @file declarations in database system do not indicate what belongs in the file