Page MenuHomePhabricator

Installer aborts on outdated sqlite module though "--dbtype mysql" was passed
Closed, ResolvedPublic

Description

Author: ans+wikimedia

Description:
The CLI installer aborts installation during envChecks with:

Warning: you have SQLite 3.3.6, which is lower than minimum required version 3.3.7. SQLite will be unavailable.

Since this check fails, the installer terminates.
This is silly, since I want to install mediawiki with a mysql database and even ran the installer with --dbtype mysql.


Version: 1.19.3
Severity: normal
OS: Linux

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 1:23 AM
bzimport set Reference to bz44511.
bzimport added a subscriber: Unknown Object (MLST).

Confirmed with git master 1.27alpha:

`
+ git -C . log --oneline -1
5c63cce Merge "ApiQueryAllRevisions: Actually use 'start' and 'end'"
+ git -C vendor log --oneline -1
5efd7d7 Update OOjs UI to v0.12.12
+ git -C skins/Vector log --oneline -1
9f5f333 Localisation updates from https://translatewiki.net.
`

Change 248585 had a related patch set uploaded (by saper):
Installer: check for available databases

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

@saper Are you still working for this ticket? I encountered this problem on PHP 7.4.

If you don't want to follow up, I can do it :)

The influence of this task:
Since PHP 7.4, the bundled libsqlite has been removed. This means that PDO_sqlite depends on libsqlite in the system. As of now, MW requires that sqlite version cannot be less than 3.8.0. This may not be a problem on Debian or Ubuntu because they will accept newer version for packages, but it's not the same on Centos.

The packages on Centos are very old. The latest libsqlite that can be obtained on my machine is 3.7.17. So this bug will affect the user more easily in this situation.

Because the author of the patch has not been active for a long time, I will take over this task.

Change 518352 had a related patch set uploaded (by 星耀晨曦; owner: 星耀晨曦):
[mediawiki/core@master] Returns warning Status instead of fatal Status if no meets the requirements

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

When upgrading an existing install, or when specifying an explicit type through parameters for a new install, we should only run the check for that database type, and abort if that db type is unknown or unsupported.

The other checks could either be skipped, or be warning-only.

The current version of change 518352 (patch set 1) appears to make the check always warning-only. If that is indeed the case, it might need to be changed to still abort for the relevant db type.

I checked the Status returned by SqliteInstaller::checkPrerequisites() in includes/installer/Installer.php.

From the logic of the code, no-good Status are handled by two implementations (CliInstaller/WebInstaller) of Installer. In CliInstaller, if Status is no OK, the script will exit, that is why the installation will abort. In WebInstaller, just show the messages.

In my patch, will check the type of db will to install. If not pass, change the Status to not-OK.

The weird thing here is that CliInstaller::showStatusMessage() exits on error, whereas WebInstaller::showStatusMessage() does not, it just writes the error out. It's been that way since 2010 cd28ef5cbe894bf9f , apparently just as a quick hack to avoid the need to check return values in install.php. Installer::envCheckDB() only returns false if there are no usable database drivers, but it writes out warnings immediately using showStatusMessage(), which exits in CLI mode, so envCheckDB() never gets a chance to return a value. This is apparently the meaning of the comment "// @todo FIXME: This only works for the web installer!"

I think the correct thing to do is to remove the exit from CliInstaller::showStatusMessage(). The currently-unreachable return at install.php line 126 should become "return false", since that is now implemented as a way to set the exit status. CliInstaller::execute() should return a Status on line 180 instead of relying on showStatusMessage() to abort the install, and the return value of performInstallation() should be checked.

Ideally the bulk of CliInstaller::__construct() would be moved to another method, so that it can return a value, but that would break b/c for scripts that subclass it. Note that getCliInstaller() encourages subclassing. So as an alternative, the current showStatusMessage() calls there could be replaced with exceptions, to be caught by CommandLineInstaller::execute() and converted to an exit status.

I don't think it's tolerable to just work around the brokenness of showStatusMessage() by never using warnings or errors as they were intended.

Change 525970 had a related patch set uploaded (by 星耀晨曦; owner: 星耀晨曦):
[mediawiki/core@master] Make CliInstaller control the processing logic of the error

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

I submitted the new patch as suggested, please review.

Change 518352 abandoned by 星耀晨曦:
Relax for non-critical db type checking

Reason:
See https://gerrit.wikimedia.org/r/525970

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

Change 525970 merged by jenkins-bot:
[mediawiki/core@master] Make CliInstaller control the processing logic of the error

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

Change 248585 abandoned by saper:
Installer: check for available databases

Reason:
fixed by another change

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