I think at this point it’s worth pursuing the Status approach after all, which was briefly discussed in this change for T365452 – and we can probably even just reuse Wikibase’s TempUserStatus, IMHO. (And make more custom subclasses of it to type-safely carry the EntitySchemaId or other relevant information.)

Change #1056197 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/EntitySchema@master] Introduce EntitySchemaStatus, use in EntitySchemaInserter

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

Bleh, I wanted to do this in multiple commits that are nice and separate, but of course that means that CI will still be broken on the earlier commits…

How painful would it be to disable temp accounts in EntitySchema CI again?

In T356148#10007596, @Lucas_Werkmeister_WMDE wrote:

How painful would it be to disable temp accounts in EntitySchema CI again?

Well, it’s doable, but there are already other tests in the suite that start to fail if temp accounts are disabled 😩

1) MediaWiki\CheckUser\Tests\Integration\HookHandler\ToolLinksHandlerTest::testOnSpecialContributionsBeforeMainOutputArchive with data set "Can see archived contributions" (true, true)
Expectation failed for method name is "addSubtitle" when invoked 1 time(s).
Method was expected to be called 1 times, actually called 0 times.

2) MediaWiki\CheckUser\Tests\Integration\HookHandler\ToolLinksHandlerTest::testOnSpecialContributionsBeforeMainOutputForMobileView
Expectation failed for method name is "addSubtitle" when invoked 1 time(s).
Method was expected to be called 1 times, actually called 0 times.

They’ve only been enabled in CI for a week 😭

Change #1056200 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/EntitySchema@master] Temporarily disable IP Masking (temporary accounts) in CI

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

Change #1056479 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/EntitySchema@master] Use EntitySchemaStatus in EntitySchemaUpdater

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

End-of-day update: CI is green, and the status refactoring in EntitySchemaUpdater is also done. Now we need to actually make it create temp users… which I think will require a bit more refactoring, because we’re currently creating the PageUpdater relatively early, but the User is part of that – so we need to create the temp user before that point. (Or create the PageUpdater later? I’m not sure. We also use that for conflict resolution.)

In T356148#10011355, @Lucas_Werkmeister_WMDE wrote:

End-of-day update: CI is green, and the status refactoring in EntitySchemaUpdater is also done.

nice :)

Now we need to actually make it create temp users… which I think will require a bit more refactoring, because we’re currently creating the PageUpdater relatively early, but the User is part of that – so we need to create the temp user before that point. (Or create the PageUpdater later? I’m not sure. We also use that for conflict resolution.)

In case it helps, in T359405: Create temporary account early in edit cycle for all edit attempts we changed the creation of the temp account to happen early in the edit cycle (after some basic permission checks in core but before any hooks are invoked). Not sure about your code in particular but if there's a potential for any logs to be generated as part of the edit attempt (e.g. an AbuseFilter trip), the temporary account should be created before that happens, so it can be referenced in the logs.

In T356148#10014039, @kostajh wrote:

Now we need to actually make it create temp users… which I think will require a bit more refactoring, because we’re currently creating the PageUpdater relatively early, but the User is part of that – so we need to create the temp user before that point. (Or create the PageUpdater later? I’m not sure. We also use that for conflict resolution.)

In case it helps, in T359405: Create temporary account early in edit cycle for all edit attempts we changed the creation of the temp account to happen early in the edit cycle (after some basic permission checks in core but before any hooks are invoked). Not sure about your code in particular but if there's a potential for any logs to be generated as part of the edit attempt (e.g. an AbuseFilter trip), the temporary account should be created before that happens, so it can be referenced in the logs.

That’s actually good to know, thanks! I had been wondering yesterday why Wikibase’s MediaWikiEditEntity creates the temp account after checking edit filters while EditPage seemed to do it the other way around; I guess it was just written that way before T359405 changed it in core. (Or perhaps we were following Trust and Safety Product/Temporary Accounts/For developers § Creating a new temporary user, which currently still says to “create a temporary user […] as late as possible” – is that outdated now?) Let’s create the temp account earlier then, before checking EditFilterMergedContent.

(Also, does that mean we should change the order in Wikibase?)

In T356148#10014795, @Lucas_Werkmeister_WMDE wrote:

In T356148#10014039, @kostajh wrote:

Now we need to actually make it create temp users… which I think will require a bit more refactoring, because we’re currently creating the PageUpdater relatively early, but the User is part of that – so we need to create the temp user before that point. (Or create the PageUpdater later? I’m not sure. We also use that for conflict resolution.)

In case it helps, in T359405: Create temporary account early in edit cycle for all edit attempts we changed the creation of the temp account to happen early in the edit cycle (after some basic permission checks in core but before any hooks are invoked). Not sure about your code in particular but if there's a potential for any logs to be generated as part of the edit attempt (e.g. an AbuseFilter trip), the temporary account should be created before that happens, so it can be referenced in the logs.

That’s actually good to know, thanks! I had been wondering yesterday why Wikibase’s MediaWikiEditEntity creates the temp account after checking edit filters while EditPage seemed to do it the other way around; I guess it was just written that way before T359405 changed it in core. (Or perhaps we were following Trust and Safety Product/Temporary Accounts/For developers § Creating a new temporary user, which currently still says to “create a temporary user […] as late as possible” – is that outdated now?) Let’s create the temp account earlier then, before checking EditFilterMergedContent.

It is outdated, sorry about that. I've just updated the docs there https://www.mediawiki.org/w/index.php?title=Trust_and_Safety_Product%2FTemporary_Accounts%2FFor_developers&diff=6666952&oldid=6636682.

In T356148#10014800, @Lucas_Werkmeister_WMDE wrote:

(Also, does that mean we should change the order in Wikibase?)

I think so, yes. If there's a possibility that a log message would be generated, that log message will need to reference a temp account and not an IP actor, so you should create the temp user acocunt before that log message could be generated.

Alright, done in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikibase/+/1056949.

Change #1057243 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/EntitySchema@master] Make WatchlistUpdater a service

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

Change #1057244 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/EntitySchema@master] Support temporary accounts

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

Change #1057836 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/EntitySchema@master] Clean up MediaWikiRevisionEntitySchemaUpdaterTest

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

Change #1057850 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/EntitySchema@master] Make MediaWikiPageUpdaterFactory a service

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

Change #1058195 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/EntitySchema@master] Use TempUserTestTrait instead of mocking TempUserConfig

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

Change #1058628 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/EntitySchema@master] Revert "Temporarily disable IP Masking (temporary accounts) in CI"

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

There's some feedback on Icfab8e5e5aeb1b057cf874f078d2f1b9febc59b2 and Id29f4fe01c92aaa187620264f279624bf706fc7f that should be addressed before this is merged. Besides those minor changes, the patches seem to work well in my local testing.

I don't want to +2 the earlier patches in the chain yet, because the first patch disables the tests in CI. I'll wait until @Lucas_Werkmeister_WMDE is back next week and available to address the feedback.

Change #1056200 merged by jenkins-bot:

[mediawiki/extensions/EntitySchema@master] Temporarily disable IP Masking (temporary accounts) in CI

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

In T356148#10041561, @ArthurTaylor wrote:

I don't want to +2 the earlier patches in the chain yet, because the first patch disables the tests in CI. I'll wait until @Lucas_Werkmeister_WMDE is back next week and available to address the feedback.

But you still +2ed that first patch itself? So now the tests have been disabled for almost a week…

In T356148#10041561, @ArthurTaylor wrote:

There's some feedback on Icfab8e5e5aeb1b057cf874f078d2f1b9febc59b2 and Id29f4fe01c92aaa187620264f279624bf706fc7f that should be addressed before this is merged.

I have seen the feedback and I don’t agree that it should block those changes. (I’ve marked both comments as resolved now.)

Yeah - I changed my mind and +2'd it in the end because there were other changes that needed to go into EntitySchema and I didn't want to have the component blocked for a week.

Thanks for resolving the comments on the patches. From my side it's good to go now.

Change #1056197 merged by jenkins-bot:

[mediawiki/extensions/EntitySchema@master] Introduce EntitySchemaStatus, use in EntitySchemaInserter

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

Change #1056479 merged by jenkins-bot:

[mediawiki/extensions/EntitySchema@master] Use EntitySchemaStatus in EntitySchemaUpdater

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

Change #1057243 merged by jenkins-bot:

[mediawiki/extensions/EntitySchema@master] Make WatchlistUpdater a service

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

Change #1057850 merged by jenkins-bot:

[mediawiki/extensions/EntitySchema@master] Make MediaWikiPageUpdaterFactory a service

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

Change #1058195 merged by jenkins-bot:

[mediawiki/extensions/EntitySchema@master] Use TempUserTestTrait instead of mocking TempUserConfig

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

Change #1057244 merged by jenkins-bot:

[mediawiki/extensions/EntitySchema@master] Support temporary accounts

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

Change #1057836 merged by jenkins-bot:

[mediawiki/extensions/EntitySchema@master] Clean up MediaWikiRevisionEntitySchemaUpdaterTest

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

Change #1058628 merged by jenkins-bot:

[mediawiki/extensions/EntitySchema@master] Revert "Temporarily disable IP Masking (temporary accounts) in CI"

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