Page MenuHomePhabricator

Figure out what to do with autoload entries for repo maintenance scripts
Closed, ResolvedPublic

Description

Consider the following:

  • If we leave autoload.php we will need to add a comment to stop people also dropping in non PSR-4 utility classes.
  • If we rename we'll have quite some fear about people who were bound to that name (but it would finish looking clean): Particularly existing cron-job.
  • If we rename then leave some deprecated "compat" files we have double the number of maintenance script files until we can phase them out.

Event Timeline

To recap: the problem is that the maintenance script file starts lowercase, but the file starts uppercase, therefore PSR-4 can’t be used to autoload e. g. \Wikibase\Repo\Maintenance\DumpRdf from repo/maintenance/dumpRdf.php, it would have to be repo/maintenance/DumpRdf.php.

I’m not sure if the maintenance scripts need to be autoloadable at all, actually. I think it’s possible that only DumpEntities (the base class of serveral “dump” maintenance scripts) needs to be autoloaded, and that’s why repo/maintenance/ was added to generateAutoload.php’s list of directories to traverse, which ended up adding all the individual maintenance scripts to the autoload file as well – but DumpEntities can be PSR-4 autoloaded properly, since it’s in DumpEntities.php (uppercase). We can probably check for uses of the maintenance scripts using MediaWiki code search.

I’m not sure if the maintenance scripts need to be autoloadable at all

Indeed they probably don't need to be, as they should be run directly.

And DumpEntities afaik is the only exception, although it in itself isn't really a maintenance script.
AFAIK the ain't scripts that use that base class should all load it directly anyway, which would still need to happen even if everything were using PSR4, as there is no autoloader at the start of maintenance script currently.

Ladsgroup added a comment.EditedJun 11 2020, 1:00 PM

I want to add even if we need to load them somewhere, they still don't need to be fully PSR-4 compliant, you can just put them in AutoLoadClasses in the extension.json file, that would get rid of autoload.php and makes loading the extension through extension registry doable.

AFAIK the ain't scripts that use that base class should all load it directly anyway, which would still need to happen even if everything were using PSR4, as there is no autoloader at the start of maintenance script currently.

Well, Maintenance::runChild() specifies a “child” maintenance script to run using a class name and file path, and the file path is optional if the class can be autoloaded. But out of the runChild uses I could find with codesearch, the Wikibase ones (in WikimediaMaintenance’s addWiki.php) all seem to specify the file path as well, so if I’m not mistaken, we don’t need those scripts (well, PopulateSitesTable seems to be the only one) to be autoloadable. (That said, currently we put that script into Lib’s AutoloadClasses. Maybe that wasn’t necessary after all.)

So I think, with DumpEntities already sorted out, we could just remove the maintenance scripts from the autoload.php, and that would be it?

Having just ripped them out spinning up a mediawiki-docker-dev environment throws

Cannot spawn child: Wikibase\Repo\Maintenance\PopulateTermFullEntityId
[d0c07525d42cb2059d632688] [no req]   Error from line 715 of /var/www/mediawiki/maintenance/includes/Maintenance.php: Class 'Wikibase\Repo\Maintenance\PopulateTermFullEntityId' not found
Backtrace:

While running the updater.

Looks like populateTermFullEntityId does need to be loaded by \Wikibase\Repo\Store\Sql\DatabaseSchemaUpdater

and I imagine the same thing is needed for RebuildTermsSearchKey

Hmph, yeah. Looking at code search, a handful of tests probably also need their test subject classes to be autoloadable (though I suppose they could require_once the files).

That code search didn’t even find the RebuildTermsSearchKey case because it’s not in the right namespace yet, by the way.

Change 604784 had a related patch set uploaded (by Tarrow; owner: Tarrow):
[mediawiki/extensions/Wikibase@master] Remove WikibaseRepo Maintenance Scripts Autoload

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

List of files that need to be moved to PSR-4 namespace (i.e. not in \Wikibase\Repo\Maintenance)

extensions/Wikibase/repo/maintenance/addUnitConversions.php
extensions/Wikibase/repo/maintenance/changePropertyDataType.php
extensions/Wikibase/repo/maintenance/clearTermSqlIndexSearchFields.php
extensions/Wikibase/repo/maintenance/dispatchChanges.php
extensions/Wikibase/repo/maintenance/dumpJson.php
extensions/Wikibase/repo/maintenance/dumpRdf.php
extensions/Wikibase/repo/maintenance/populateChangesSubscription.php
extensions/Wikibase/repo/maintenance/populateWithRandomEntitiesAndTerms.php
extensions/Wikibase/repo/maintenance/pruneChanges.php
extensions/Wikibase/repo/maintenance/rebuildItemTerms.php
extensions/Wikibase/repo/maintenance/rebuildPropertyInfo.php
extensions/Wikibase/repo/maintenance/rebuildPropertyTerms.php
extensions/Wikibase/repo/maintenance/rebuildTermSqlIndex.php
extensions/Wikibase/repo/maintenance/rebuildTermsSearchKey.php

Change 605184 had a related patch set uploaded (by Tarrow; owner: Tarrow):
[mediawiki/extensions/Wikibase@master] Move Maintenance to PSR-4 Style NS

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

Change 605184 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Move Maintenance to PSR-4 Style NS

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

a handful of tests probably also need their test subject classes to be autoloadable (though I suppose they could require_once the files).

I just realized that TestAutoloadClasses would probably be better for that.

Change 604784 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Remove the last autoload.php

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

Lucas_Werkmeister_WMDE closed this task as Resolved.Jun 16 2020, 9:16 AM