That's some ugly code. We can do better.

CommentFileSizeAuthor
#6 323372-bootstrap-fix.patch1018 bytesdamien tournoud
#5 323372-bootstrap-fix.patch950 bytesdamien tournoud
#1 get_bootstrap_phase_collateral.patch1.85 KBchx
get_bootstrap_phase_no_global_oh_my.patch1.88 KBchx

Comments

chx’s picture

chx commented
StatusFileSize
new get_bootstrap_phase_collateral.patch1.85 KB

DamZ complained that I return the phase we are in and not the one we completed. But if we are to keep the completed_phase then we can refactor the whole function to something more simpler...

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Reviewed & tested by the community 
$ egrep -r _drupal_current_bootstrap_phase .
./includes/bootstrap.inc:    global $_drupal_current_bootstrap_phase;
./includes/bootstrap.inc:    $_drupal_current_bootstrap_phase = $current_phase;
./includes/bootstrap.inc:  global $_drupal_current_bootstrap_phase;
./includes/bootstrap.inc:  return $_drupal_current_bootstrap_phase;

$_drupal_current_bootstrap was not used anywhere outside those two functions.

I have nothing to add...

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks all.

damien tournoud’s picture

damien tournoud commented
Status: Fixed » Active

This broke the installer.

damien tournoud’s picture

damien tournoud commented
Priority: Normal » Critical
Status: Active » Needs review
StatusFileSize
new 323372-bootstrap-fix.patch950 bytes

Pfff.

Note to self: review chx' patches with a critical eye.

damien tournoud’s picture

damien tournoud commented
StatusFileSize
new 323372-bootstrap-fix.patch1018 bytes

Better yet.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Should we also remove the now-unnecessary drupal_get_bootstrap_phase function or separate patch?

damien tournoud’s picture

damien tournoud commented

No we don't need to remove it. It's a clean API, for once, please don't remove it :)

boombatower’s picture

boombatower commented
Status: Needs review » Reviewed & tested by the community

Last two patches fix issue and get all passes.

http://testing.drupal.org/pifr/node/1/323372

or even like:
http://testing.drupal.org/pifr/file/drupal.org/323372-bootstrap-fix.patch

When this gets in it would be nice if I was notified so I can update t.d.o. We seem to be getting a number of these that I have to hack t.d.o in the mean time.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks! :)

Anonymous’s picture

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

Automatically closed -- issue fixed for two weeks with no activity.