Requirement warning shouldn't block update.php from running [#2184049]

If running update.php (in-browser) and the 'user has the same email as the site email address' requirement is flagged, it prevents the update scripts from running (because any requirement warning or error does). I propose to hide this warning if running update.php.

Comment	File	Size	Author
#3	2184049-requirement-runtime-phase.patch	513 bytes	Dave Reid
#1	2184049-requirement-runtime-phase.patch	513 bytes	Dave Reid

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Dave Reid

he/him

English

Nebraska USA

CreditAttribution: Dave Reid commented 29 January 2014 at 00:44

Status:

Active

» Needs review

File	Size
2184049-requirement-runtime-phase.patch	513 bytes

Looks like it's as simple as checking the $phase variable passed into hook_requirements(). Stuff like this should only be checked if $phase == 'runtime' as a best practice. Note that update_check_requirements() passes in 'update' so this would be skipped in that case.

Comment #2

q0rban CreditAttribution: q0rban commented 29 January 2014 at 02:18

Shouldn't this be if $phase != 'runtime'?

Comment #3

Dave Reid

he/him

English

Nebraska USA

CreditAttribution: Dave Reid commented 29 January 2014 at 02:25

File	Size
2184049-requirement-runtime-phase.patch	513 bytes

I would say I was testing you but I just made this too quickly. My bad.

Comment #4

q0rban CreditAttribution: q0rban commented 29 January 2014 at 02:30

It seems odd that Drupal would be blocking an update from a REQUIREMENT_WARNING, since the docs specifically say that *shouldn't* be happening: https://api.drupal.org/api/drupal/includes!install.inc/constant/REQUIREM...

Requirement severity -- Warning condition; proceed but flag warning.

Comment #5

q0rban CreditAttribution: q0rban commented 29 January 2014 at 02:32

Either way, I think this is a safe enough change.

Comment #6

Dave Reid

he/him

English

Nebraska USA

CreditAttribution: Dave Reid commented 29 January 2014 at 02:35

It doesn't reference what should be proceeding though. I think it's also meant to reference the installer only, looking at drupal_check_module() (which is used during install process).

Now that I take a hard look at the update.php warning, it's deceptive as well. It shows the warnings at first and only has the following text at the bottom:

Check the error messages and try again.

To mean this means I need to resolve the warnings and re-attempt update.php, but in reality the 'try again' link actually just lets me continue the update.php script ignoring the warnings. I still think it's proper to hide this warning except when $phase == 'runtime'.