Page MenuHomePhabricator

Running maintenance scripts without mwscript fails silently on MediaWiki-Vagrant
Closed, ResolvedPublic

Description

When php <some-maintenance-script> --wiki=<some-db> is used on MW-Vagrant, the DB parameter is silently ignored. This is not obvious, and it's hard to figure out what's happening and how it can be avoided.

The relevant change is 27253a2; the version before that was not user-friendly, but that can be fixed. (E.g. choose the main wiki if there are no others, show an error message with the correct usage otherwise. That's also a good place to educate users about foreachwiki.)

Details

Related Gerrit Patches:

Event Timeline

Tgr created this task.Jan 8 2016, 10:21 PM
Tgr raised the priority of this task from to Needs Triage.
Tgr updated the task description. (Show Details)
Tgr added a project: MediaWiki-Vagrant.
Tgr added a subscriber: Tgr.
Restricted Application added subscribers: StudiesWorld, Aklapper. · View Herald TranscriptJan 8 2016, 10:21 PM

Change was made for T71520, and this task is a follow on from T122863

On WMF wikis we have

if ( !class_exists( 'MWMultiVersion' ) ) {
        print "No MWMultiVersion instance initialized! MWScript.php wrapper not used?\n";
        exit(1);
}

Per discussion on IRC, if we only have one wiki, this silent fallback isn't the worst thing in the world. If there's more than one, falling back to the default wiki is unintuitive. The error message could/should be improved in those cases though

CC @bd808 for his input

bd808 added a comment.Jan 8 2016, 10:47 PM

The changes in https://gerrit.wikimedia.org/r/#/c/154472/ were made under the assumption that the primary MediaWiki-Vagrant use case is to run a single wiki and the implementation decision to always provision as a multiwiki-style a wikifarm introduced a layer of complexity that would be confusing and annoying to a first-time/casual MediaWiki-Vagrant developer.

As I see it there are 3 possibilities:

  1. Keep things as they are today (mwscript --wiki=wiki is assumed if mwscript isn't used)
  2. Go back to the original implementation (fail with an error if mwscript isn't used)
  3. Make a hybrid of #1 & #2 where mwscript --wiki=wiki is assumed until more than one wiki instance has been provisioned and from then on fail with an error if mwscript is not used.

I think I would personally lean towards trying option 3 since we have already tried 1 and 2 and found issues with them. Others may however disagree and feel that adding even more "magic" is worse rather than better.

Reedy added a comment.Jan 8 2016, 10:49 PM

count( $wgLocalDatabases ) > 1 etc, you use mwscript... If you don't, it will fail (with a better error message!)

I suspect just doing it silently is half of the problem... "mwscript wrapper not initialised,, running script as dbname foobarwiki"

Tgr added a comment.Jan 9 2016, 12:49 AM
  1. Make a hybrid of #1 & #2 where mwscript --wiki=wiki is assumed until more than one wiki instance has been provisioned and from then on fail with an error if mwscript is not used.

If someone explicitly refers to a non-existing DB, that should be an error though.

Unicornisaurous triaged this task as Low priority.
Unicornisaurous set Security to None.
Unicornisaurous added a subscriber: Unicornisaurous.

Change 264717 had a related patch set uploaded (by Unicornisaurous):
Add sanity check to ensure mwscript is used in multiwiki setups

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

Unicornisaurous added a comment.EditedJan 18 2016, 6:23 AM

@Tgr: Sorry I didn't have much time to work on this task today. I submitted a small patch for vagrant but I'm having some trouble adding a check to see if the requested database actually exists (ie. is in $wgLocalDatabases). I'm trying to add a check into Maintenance.php or doMaintenance.php, but I can't find a place to put the check after $wgLocalDatabases is actually populated. (At what point is $wgLocalDatabases expected to be populated? I assume they should be available after LocalSettings are loaded?)

I'm also confused as to the role of the MW_DB and MW_PREFIX constants which are defined in Maintenance.php using the value of the --wiki option. I don't see them referenced anywhere else.

Tgr added a comment.Jan 18 2016, 6:36 AM

Maintenance is used by both Vagrant and normal installs, so changing that is more scary. $wgLocalDatabases is populated (assuming you run a maintenance script with mwscript) from the /vagrant/settings.d/wikis/*/dbConf.php files which are loaded from /var/www/w/dblist.php which is required in various places (e.g. /var/www/w/index.php or MWMultiVersion::initializeForMaintenance()). I think you can just rely on a null check for MWMultiVersion::getInstance(), though - if mwscript is used with a valid database, it sets the instance. No idea if the MW_ constants are actually used; might be B/C stuff.

Change 264717 merged by jenkins-bot:
Add sanity check to ensure mwscript is used in multiwiki setups

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

Tgr closed this task as Resolved.Jan 20 2016, 6:56 AM

Thanks @Unicornisaurous!

Thu current logic aborts and shows the (presumably) correct command when there is ambiguity. There are some edge cases in which the command will be wrong (there is no escaping; if there is a DB parameter, it's duplicated) but there is no easy way to fix that and IMO improving it is not worth the effort so we can close this.

Change 276403 had a related patch set uploaded (by BryanDavis):
multiversion: fix wgConf global extraction

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

Change 276403 merged by jenkins-bot:
multiversion: fix wgConf global extraction

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