Updater Suggestions

[wrossmann]

Was just helping a friend through a failed update to 1.7.0 and have some notes for the updater logic.

1. Your database migrations are fragile and one-way.
   • The migrations are not completely idempotent [Eg: 112, which is what we fought with] and there is no rollback code at all, meaning that a single failed statement can leave the DB in a state that requires either manual intervention or a restore from backup.
   • My knee-jerk reaction was "use transactions" but you can't do DML modifications in a transaction.
   • I would suggest using a library that specifically handles DB migrations and regressions, eg: up() and down().
2. The updater does not update the app version until all migrations have run, meaning that re-running updater tries to apply all the migrations again.
   • After each migration is run either update the config.version to match, or use a separate version setting just for the migrations.
   • This would allow migrations to be resumed after fixing specific issues, without re-running potentially non-idempotent migrations.

3. sourcebans-pp/web/updater/Updater.php

   Line 37 in 358d31e

   $this->latestVersion = array_pop(array_keys($this->updateList));

   • To get rid of the notice 'Only variables should be passed by reference':
     array_pop(array_keys($this->updateList)) -> array_slice(array_keys($this->updateList), -1)[0]

Specifically what I think happened is $friend started at config.version == 1 and got up to 355 where a duplicate mod entry caused the whole thing to conk out, and then he got stuck at the noted migration for 112. Since the DB structure was already obviously changed we commented out the block. After seeing the duplicate key failure in 355 we reviewed the tables that appear to reference sb_mods, sb_servers and sb_submissions, and removed the duplicate entry that was not referenced.

[dhopeross]

And maybe don't put your PHP closing bracket before the secret code so IT'S NOT BEING BLASTED OUT TO THE ENTIRE INTERNET!!!!

[wrossmann]

Haha yeah. I think that the upgrade script might just append the define() statement to the settings file without checking if there's a closing ?> at the end.