[3.0] Returns a bool from the substep that backs up a table

[albertlast]

Description

On PostgreSQL, an upgrade that was asked to back up the database stops at the first table it tries to copy:

Step 3: Backup
 +++ Backing up "smf_admin_info_files"... failed with error: "SMF\Maintenance\Tools\Upgrade::doBackupTable(): Return value must be of type bool, PgSql\Result returned"
stopped at step 3 of 7 (Backup)

doBackupTable() is declared : bool and returns the database layer's answer unchanged:

public function doBackupTable($table): bool
{
    return Db::$db->backup_table($table, 'backup_' . $table);
}

DatabaseApiInterface declares that answer as object|bool, and the two engines use both halves of it. PostgreSQL::backup_table() returns the result of its CREATE TABLE, which pg_query() answers with a PgSql\Result; MySQL::backup_table() returns the result of the same statement, which mysqli answers with true. The file is declare(strict_types=1), so one engine returns a bool and the other throws.

GenericSubStep::execute() treats a false return as a failed substep, and that is the signal worth keeping, so this compares against false rather than casting.

This has been the case since e963318b8, which moved the backup loop into substeps. The code before it discarded the return value and left a note about this exact thing:

// @@TODO: Check result? Should be a object, false if it failed.
Db::$db->backup_table($table, 'backup_' . $table);

It survives because the command line skips the backup step unless something asks for it, so an automated run never reaches this. In a browser the checkbox is ticked by default, which makes this the ordinary path for someone upgrading a PostgreSQL forum by hand.

How this was verified

Upgrading the 2.1.7 baseline from the 2.1 development environment, with the backup step turned on, on both engines:

	before	after
PostgreSQL, backups on	stops at step 3, first table	runs to completion
MySQL, backups on	runs to completion	runs to completion

The PostgreSQL run then reaches a second, unrelated fault in backup_table() itself, which is #9650. Both were applied together to confirm the upgrade finishes; this change alone moves the failure rather than removing it.

Also checked with the upgrade killed part way and started again, at 25% and 75% of the substeps, with backups on. Both recovered in one run, and every non-backup_ object matched an uninterrupted upgrade: all 323 tables, sequences, indexes, constraints, functions, aggregates and operators, 65 row counts and 228 settings.

Not reachable from the unit suite

doBackupTable() needs Db::$db, and telling the two engines apart is the whole of the behaviour.

Issues References (Fixes|Related|Closes)

1. Related: [3.0] Keeps a backup table from depending on the live table's sequence #9650, the fault this one uncovers.