DRUPAL_MINIMUM_MYSQL and a number of similar variables are defined in system.install where they aren't available after installation. I propose to move them to system.module below the Version string.

Currently, functions like db_status_report that use these defines won't work outside the installer's scope.

CommentFileSizeAuthor
#5 move_defines2.patch.txt1.39 KBasimmonds
#3 move_defines.patch.txt2.8 KBkilles@www.drop.org
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dww’s picture

dww
we/he/they
CreditAttribution: dww commented

seems reasonable. currently, nothing in core calls db_status_report() except system.install, but i don't see why we'd want to end up breaking those functions outside the scope of update.php. the only *tiny* downside is 4 more lines of php to parse on every page load, but that's definitely not a good enough reason to avoid this change...

+1 from me.

ChrisKennedy’s picture

ChrisKennedy CreditAttribution: ChrisKennedy commented

Sounds good to me. I did some quick searching where system.install is loaded and system.module isn't (i.e. part of installation) but didn't find anything that used those variables.

killes@www.drop.org’s picture

killes@www.drop.org CreditAttribution: killes@www.drop.org commented
Status: Active » Needs review
FileSize
move_defines.patch.txt2.8 KB

ok, here's the patch. it needs to be tested during an install.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Version: 5.x-dev » 6.x-dev
asimmonds’s picture

asimmonds CreditAttribution: asimmonds commented
FileSize
move_defines2.patch.txt1.39 KB

Rerolled for HEAD.

Have also tested with a fresh install, which works OK.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

looks good, has been tested. RTBC.

i even made sure that killes didn't try to bump the minimum requirements :)

Dries’s picture

Dries CreditAttribution: Dries commented
Version: 6.x-dev » 5.x-dev

Committed to CVS HEAD. Thanks.

asimmonds’s picture

asimmonds CreditAttribution: asimmonds commented

Commit http://drupal.org/cvs?commit=62004 doesn't show the defines being removed from system.install

killes@www.drop.org’s picture

killes@www.drop.org CreditAttribution: killes@www.drop.org commented
Status: Reviewed & tested by the community » Fixed

http://drupal.org/cvs?commit=62009

Anonymous’s picture

(not verified) CreditAttribution: commented
Status: Fixed » Closed (fixed)