Running DatabaseUpdater::purgeCache via WebInstaller should handle exceptions more gracefully
Closed, ResolvedPublic

Description

Split from T209285

In includes/installer/DatabaseInstaller.php method doUpgrade(), the call to $up->purgeCache() is not done inside the try/catch block. This means that if an error occurs in this step, the error is not output correctly, and furthermore the code to stop the ajax spinner is never run.

GCI instructions

  • Verify that you can trigger the issue locally, so you know if you successfully fixed it:
    • Download and setup a local copy of mediawiki
    • Artificially add a syntax error to trigger the issue. Go to includes/installer/DatabaseUpdater.php method purgeCache(). on around line 1068 change $this->db->delete( 'objectcache', '*', __METHOD__ ); to $this->db->delete( 'objAFafds', '*', __METHOD__ ); (Don't forget to change this back after you are done testing)
    • Once mediawiki is installed, go in your webbrowser to http://127.0.0.1/path/to/my/wiki/mw-config (If using vagrant, that will likely be http://localhost:8080/w/mw-config )
    • Put in your upgrade key (It should be in LocalSettings.php) and follow steps to do a db upgrade.
    • Check that you can reproduce the bad behaviour (i.e. The spinner never stops, and the error message has a bunch of html in it that is shown directly)
  • Now, fix the issue:
    • in includes/installer/DatabaseInstaller.php in the doUpgrade() method, ensure that the call to $up->purgeCache() is inside the try{} block.
  • Retest the issue again. Make sure that now you still get an error, but the error doesn't have any weird formatting and that the spinner stops spinning once the error is encountered.
  • Submit your change as a patch to gerrit.

GCI task: https://codein.withgoogle.com/dashboard/tasks/6205653698740224/

Related Objects

Bawolff created this task.Nov 13 2018, 9:24 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 13 2018, 9:24 AM
Bawolff updated the task description. (Show Details)Nov 13 2018, 9:24 AM
Bawolff renamed this task from Running update.php via WebInstaller should handle exceptions more gracefully to Running DatabaseUpdater::purgeCache via WebInstaller should handle exceptions more gracefully.Nov 13 2018, 9:31 AM
Bawolff updated the task description. (Show Details)

Change 474377 had a related patch set uploaded (by LukBukkit; owner: LukBukkit):
[mediawiki/core@master] Improve display of an SQL error during the installation

https://gerrit.wikimedia.org/r/474377

When this error during a upgrade happens, it looks like this after the patch:

Bawolff closed this task as Resolved.Nov 16 2018, 11:26 PM

Good work :)

Change 474377 merged by jenkins-bot:
[mediawiki/core@master] Improve display of an SQL error during the installation

https://gerrit.wikimedia.org/r/474377