Change 979991 had a related patch set uploaded (by Tchanders; author: Tchanders):

[mediawiki/core@master] WIP: Always allow local IP actors to be created when importing

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

Extension:FileImporter uses UploadRevisionImporter for importing changes to files (FileRevisionFromRemoteUrl) and text changes (TextRevisionFromTextRevision).

I tried importing some files from commons that had revisions by IP actors (locally, with temp accounts enabled, with https://gerrit.wikimedia.org/r/979991 applied). All were imported successfully - the new IP actors were created successfully.

I noticed that ActorStore was in the "disallow IP actor creation" mode for the changes to files, and "allow IP actor creation" mode for the text changes. This is because the text changes use ImportableOldRevisionImporter, which is updated in the patch, whereas file changes use ImportableUploadRevisionImporter which is not.

Change 979991 merged by jenkins-bot:

[mediawiki/core@master] Always allow local IP actors to be created when importing

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

• Allow an IP actor to be created when importing an upload

Scheduled for next sprint - will add this once the board is created

Change 1006070 had a related patch set uploaded (by Tchanders; author: Tchanders):

[mediawiki/core@master] WIP Demo of allowing IP actor creation for file import

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

• Allow an IP actor to be created when importing an upload

This is a bit trickier than importing a revision, because ImportableUploadRevisionImporter, OldLocalFile and LocalFile haven't been modernized. The patch at https://gerrit.wikimedia.org/r/1006070 shows how it could be done.

I tested this patch by importing files between two local wikis, and encountered another IP actor creation error from the FileImporter extension.

There are a few approaches we could take here:

1. Make the core changes only, in case we get other usages of ImportableUploadRevisionImporter in the future
2. Fix the IP actor creation error(s) in FileImporter
3. Do nothing yet, and only fix this if and when it is needed in the future.

My preference is (3). The case where a logged out user can upload is unlikely on WMF sites or third party sites, so I don't think it's worth taking the time to fix this now (thinking of testing, QA, maintenance). Just fixing core adds more mess to the mess that is already there for no feature improvement. If this is needed in the future, hopefully it could be part of a more significant refactor and be done more cleanly, as in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/979991.

Moving to the code review column for feedback on the approach (despite the patch being WIP).

In T354207#9573166, @Tchanders wrote:

There are a few approaches we could take here:

1. Make the core changes only, in case we get other usages of ImportableUploadRevisionImporter in the future

I agree that changing this adds complication for little to no improvement. Especially how adding to ::recordUpload3 with another boolean value makes the arguments to that method even more confusing.

1. Fix the IP actor creation error(s) in FileImporter

This seems possible on a quick look to me, but I'm not sure entirely how it would be done without some modification to core (based on the understanding that ImportableUploadRevisionImporter is called by the extension).

1. Do nothing yet, and only fix this if and when it is needed in the future.

I also agree with this being the preferred solution for the time being. The complication of the File related code (as found by MediaModeration) is such that there will be a lot of time spent on making this definitely work. Weighing this against how this (should) never be encountered on production WMF wikis and for third-party wikis too, we probably don't need to spend time on this and can revisit this if our assumption failed.

To ensure that my thinking wasn't based on incorrect assumptions, I ran the following SQL queries on commonswiki which all returned zero (which shows no revisions of files created by IPs):

select count(*) from image join actor on img_actor = actor_id where actor_user is NULL AND act
or_name != 'Conversion script' AND actor_name not like '%>%';
select count(*) from oldimage join actor on oi_actor = actor_id where actor_user is NULL AND a
ctor_name != 'Conversion script' AND actor_name not like '%>%';
select count(*) from filearchive join actor on fa_actor = actor_id where actor_user is NULL AN
D actor_name != 'Conversion script' AND actor_name not like '%>%';

In T354207#9583501, @Dreamy_Jazz wrote:

In T354207#9573166, @Tchanders wrote:

There are a few approaches we could take here:

1. Make the core changes only, in case we get other usages of ImportableUploadRevisionImporter in the future

I agree that changing this adds complication for little to no improvement. Especially how adding to ::recordUpload3 with another boolean value makes the arguments to that method even more confusing.

1. Fix the IP actor creation error(s) in FileImporter

This seems possible on a quick look to me, but I'm not sure entirely how it would be done without some modification to core (based on the understanding that ImportableUploadRevisionImporter is called by the extension).

1. Do nothing yet, and only fix this if and when it is needed in the future.

I also agree with this being the preferred solution for the time being. The complication of the File related code (as found by MediaModeration) is such that there will be a lot of time spent on making this definitely work. Weighing this against how this (should) never be encountered on production WMF wikis and for third-party wikis too, we probably don't need to spend time on this and can revisit this if our assumption failed.

To ensure that my thinking wasn't based on incorrect assumptions, I ran the following SQL queries on commonswiki which all returned zero (which shows no revisions of files created by IPs):

select count(*) from image join actor on img_actor = actor_id where actor_user is NULL AND act
or_name != 'Conversion script' AND actor_name not like '%>%';
select count(*) from oldimage join actor on oi_actor = actor_id where actor_user is NULL AND a
ctor_name != 'Conversion script' AND actor_name not like '%>%';
select count(*) from filearchive join actor on fa_actor = actor_id where actor_user is NULL AN
D actor_name != 'Conversion script' AND actor_name not like '%>%';

I also agree with option 3 ("do nothing until/unless needed"). Thanks for the investigation & options!

Thanks for the feedback.

Moving the for QA now. The patch we made fixed this case:

Allow an IP actor to be created when importing a revision

Testing steps

Example of how to test this on a wikifarm with two wikis:

• On wiki 1:
  • Disable temp accounts
  • Edit an article as an IP actor
  • Visit Special:Export
    • Check "Save as file"
    • Enter the article you just edited and export it
• At wiki 2:
  • Enable temp accounts
  • Visit Special:Import (requires import right)
  • Upload the file that was downloaded in the previous step
  • Import should be successful and the history page should show edits by the IP actor

I have imported revisions made by IPs via Special:Import and API:Import.

I have checked in the actor database that new rows for IPs are created if they are not already there.

For some imports I performed an API:Query action to check the contributors and revisions properties.

I also imported a couple of files from commons.

I also tested on a wiki without temporary accounts in case of any regressions.

Locally, I tried doing an import as an anonymous/IP user (I had to change my permissions to allow this). On a wiki with temporary users, the import does not succeed and I get the below error in the logs. I assume this is OK as it probably isn't sensible to allow imports for anonymous users.

2024-03-11 08:28:40 fd76acf90528 enwiki: [ba7f5ccaa3e17b246d4d89e9] /w/api.php   Wikimedia\Rdbms\DBTransactionStateError: Cannot execute query from MediaWiki\MediaWikiEntryPoint::commitMainTransaction while transaction status is ERROR
#0 /var/www/html/w/includes/libs/rdbms/database/Database.php(992): Wikimedia\Rdbms\TransactionManager->assertTransactionStatus()
#1 /var/www/html/w/includes/libs/rdbms/database/Database.php(640): Wikimedia\Rdbms\Database->assertQueryIsCurrentlyAllowed()
#2 /var/www/html/w/includes/libs/rdbms/database/Database.php(2376): Wikimedia\Rdbms\Database->query()
#3 /var/www/html/w/includes/libs/rdbms/loadbalancer/LoadBalancer.php(1458): Wikimedia\Rdbms\Database->commit()
#4 /var/www/html/w/includes/libs/rdbms/lbfactory/LBFactory.php(331): Wikimedia\Rdbms\LoadBalancer->commitPrimaryChanges()
#5 /var/www/html/w/includes/MediaWikiEntryPoint.php(296): Wikimedia\Rdbms\LBFactory->commitPrimaryChanges()
#6 /var/www/html/w/includes/MediaWikiEntryPoint.php(188): MediaWiki\MediaWikiEntryPoint->commitMainTransaction()
#7 /var/www/html/w/includes/MediaWikiEntryPoint.php(171): MediaWiki\MediaWikiEntryPoint->doPrepareForOutput()
#8 /var/www/html/w/includes/MediaWiki.php(90): MediaWiki\MediaWikiEntryPoint->prepareForOutput()
#9 /var/www/html/w/includes/api/ApiMain.php(950): MediaWiki::preOutputCommit()
#10 /var/www/html/w/includes/api/ApiMain.php(893): ApiMain->executeActionWithErrorHandling()
#11 /var/www/html/w/includes/api/ApiEntryPoint.php(158): ApiMain->execute()
#12 /var/www/html/w/includes/MediaWikiEntryPoint.php(199): MediaWiki\Api\ApiEntryPoint->execute()
#13 /var/www/html/w/api.php(44): MediaWiki\MediaWikiEntryPoint->run()
#14 {main}
2024-03-11 08:28:40 fd76acf90528 enwiki: [ba7f5ccaa3e17b246d4d89e9] /w/api.php   Wikimedia\Rdbms\DBTransactionError: Commit failed on server(s) mariadb-main: Cannot execute query from MediaWiki\MediaWikiEntryPoint::commitMainTransaction while transaction status is ERROR
#0 /var/www/html/w/includes/libs/rdbms/lbfactory/LBFactory.php(331): Wikimedia\Rdbms\LoadBalancer->commitPrimaryChanges()
#1 /var/www/html/w/includes/MediaWikiEntryPoint.php(296): Wikimedia\Rdbms\LBFactory->commitPrimaryChanges()
#2 /var/www/html/w/includes/MediaWikiEntryPoint.php(188): MediaWiki\MediaWikiEntryPoint->commitMainTransaction()
#3 /var/www/html/w/includes/MediaWikiEntryPoint.php(171): MediaWiki\MediaWikiEntryPoint->doPrepareForOutput()
#4 /var/www/html/w/includes/MediaWiki.php(90): MediaWiki\MediaWikiEntryPoint->prepareForOutput()
#5 /var/www/html/w/includes/api/ApiMain.php(950): MediaWiki::preOutputCommit()
#6 /var/www/html/w/includes/api/ApiMain.php(893): ApiMain->executeActionWithErrorHandling()
#7 /var/www/html/w/includes/api/ApiEntryPoint.php(158): ApiMain->execute()
#8 /var/www/html/w/includes/MediaWikiEntryPoint.php(199): MediaWiki\Api\ApiEntryPoint->execute()
#9 /var/www/html/w/api.php(44): MediaWiki\MediaWikiEntryPoint->run()
#10 {main}

Test environments:

• https://de.wikipedia.beta.wmflabs.org
• https://en.wikipedia.beta.wmflabs.org

Change 1006070 abandoned by Dreamy Jazz:

[mediawiki/core@master] WIP Demo of allowing IP actor creation for file import

Reason:

Task closed with (for now) not allowing IP actor creation for file imports.

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