In T251588#6131324, @eprodromou wrote:

OK, we need to review this in tech planning.

Any updates? This will soon be blocking progress on T157658: Factor out a backend from EditPage

EditPage::attemptSave and EditPage::attemptSave::after are indeed only used by WikiEditor, and the only thing happening in there is EventLogging to EditAttemptStep schema. The perfect solution indeed would be to drop these hooks. @Ottomata is this schema still used? What is the plan (if any) for that schema and transition to new event platform? Adding Analytics as well if someone other then Andrew has an answer.

As for the two other hooks - given the new edit constraint system, it would be nice to replace them with hooks that can add more instances of IEditConstraint interface to a passed runner, or returns an array of additional constraints. However, the interface of the constrain is still not quite solidified to allow extensions implementing more of them. As it currently stands, the instance of the constraint has to get all the edit state it needs in the constructor, thus if we allow making new constraints in the hook, we need to pass significant amount of edit state into the hook... So perhaps it's better to wait on a decision regarding these 2 hooks until we have more of the constraints system migrated.

For now, the constraint system is completely internal, and until the backend is fully factored out of EditPage it should probably stay that way. I agree that the "perfect solution" is to eliminate these hooks if possible. The full migration to a backend cannot proceed until eventually the hooks passing instances of EditPage objects are removed, since the backend won't know about EditPage

Ping @nettrom_WMF and @nshahquinn-wmf as per talk:EditAttemptStep.

Pretty sure this is very active and used. We're migrating legacy schemas over to the new platform in a backwards compatible way; meaning we are doing so without changing instrumentation code. We don't have any plans to stop emitting this schema's events via that those hooks as part of that migration. Adding Product-Analytics

Adding @MNeisler to make sure Editing is aware.

Yes, confirming that EditAttemptStep is still active and used frequently by the Editing team and others to track editing activity for multiple ongoing projects. I've tagged the Editing team on this task so they are aware.

cc @Milimetric

Codesearch shows 22 extensions that use these hooks: https://codesearch.wmcloud.org/extensions/?q=EditFilter%7CEditPage%3A%3AattemptSave%7CEditFilterMergedContent%7CEditPage%3A%3AattemptSave%3Aafter&i=nope&files=extension%5C.json&excludeFiles=&repos=

For each usage, we'll have to check how the $editPage parameter is used.

I expect that we will resolve this as part of T275847. Moving to "watching" for now, for lack of a better place.

In T251588#6860385, @daniel wrote:

Codesearch shows 22 extensions that use these hooks: https://codesearch.wmcloud.org/extensions/?q=EditFilter%7CEditPage%3A%3AattemptSave%7CEditFilterMergedContent%7CEditPage%3A%3AattemptSave%3Aafter&i=nope&files=extension%5C.json&excludeFiles=&repos=

For each usage, we'll have to check how the $editPage parameter is used.

I've excluded EditFilterMergedContent from that search because it does not pass an EditPage object and there's also a separate task for it: T304238. Then, looking at the others:

• mediawiki-moderation's onEditFilter uses it with Reflection (!) to get the private (!!) watchthis property.
• Athena's onEditFilter uses it to get the page being edited and then the user
• DiscussionThreading's attemptSave uses it to get the section, the summary, and it also adds a bunch of dynamic properties that are read/written in this and other hook handlers passing an EditPage
• Draft's onEditFilter uses it to get the main WebRequest (via EditPage::getArticle()->getContext()->getRequest()) to read a query parameter set by the extension elsewhere
• JsonData's onEditFilter uses it to get the page
• MsCatSelect's attemptSave uses it to append text to EditPage::$textbox1
• SpamBlacklist's onEditFilter uses it to get the page being edited
• SpamRegex's onEditFilter uses it to get the page, the summary, EditPage::$textbox1, and calls EditPage::spamPageWithContent()
• TitleBlacklist's onEditFilter uses it to get the page being edited
• WikiEditor's attemptSave uses it to get the WebRequest from the Article
• WikiEditor's attemptSave::after uses it to get the WebRequest from the Article and also the page being edited

I think passing the page and the user would be a good starting point. Then perhaps also the summary and the new text.

In T251588#6580279, @DannyS712 wrote:

For now, the constraint system is completely internal, and until the backend is fully factored out of EditPage it should probably stay that way. I agree that the "perfect solution" is to eliminate these hooks if possible. The full migration to a backend cannot proceed until eventually the hooks passing instances of EditPage objects are removed, since the backend won't know about EditPage

This sounds like an egg-or-chicken problem: you say the constraints system should stay internal until the backend is fully factored out, but the backend cannot be fully factored out until the old hooks are removed. There are two solutions:

• Introduce a new hook, deprecate the old one, finish the factoring out and make constraints stable, deprecate the new hook as well. Migrate extensions twice.
• Mark the constraints stable before the backend is factored out (but late enough so that breaking changes will be very unlikely). Migrate extensions only once.

I think the latter causes less hassle.

In T251588#7863938, @Daimona wrote:

I think passing the page and the user would be a good starting point. Then perhaps also the summary and the new text.

“Then” sounds like you plan to have more data added to the hook later on? That would mean we need a value object holding all the data, since one cannot add new parameters to a hook, but can add new properties to an object.

Also, I think we should take the chance to use narrow interfaces (PageIdentity, UserIdentity etc.) instead of WikiPage, Article, User and the like.

Regarding the edit constraint system, I think something like this would be a good way forward:

1. Move the page saving logic + related data into separate classes (https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1249476)
2. Simplify the edit inputs class as far as possible (provide defaults, use narrow interfaces as mentioned above, etc...). PageEdit and the related classes will need a lot of refactoring after the initial patch, since right now most parts are directly copied from EditPage to reduce the amount of changes in the patch.
3. Pass the edit inputs to EditConstraint::checkConstraint and remove all constructor parameters that are not dependencies. We could also potentially use an interface that's implemented by the edit inputs class, so we only expose the data that's really necessary for constraints.
4. Use ObjectFactory to allow edit constraints to use DI directly (since IMO this is cleaner than injecting everything into EditConstraintFactory and also into extension hook handlers after step 6, and we avoid autoloading classes / instantiating services we may not even need)
5. Move the creation of the edit constraint runners from PageEdit (e.g. ::getPreliminaryChecksRunner, ::getNewPageChecksRunner etc.) into EditConstraintFactory.
6. The constraint runners are already split up into stages, so we could create constants to identify those stages and then create a hook with $stage + &$constraints (or the constraint runner object) as parameters that allows extensions to add custom constraints to certain stages.
7. Rename EditPageStatus to PageEditStatus.
8. Mark EditConstraint, PageEditStatus + the hook as stable and replace EditFilter/EditFilterMergedContent with it.

The main issue with this is that each stage needs access to different values. However, I don't think it's a good solution to have multiple abstract edit constraint classes that all take different value objects (which would also require separate classes...). We also check some edit constraints for previews (RedirectConstraint; T394016) or when displaying the header (AccidentalRecreationConstraint, RevisionDeletedConstraint). Maybe it's possible to find a set of values that is sufficient for all usages and available in all stages.

Another question is whether the edit inputs are sufficient for all current uses of the hooks, or if they depend on EditPage for any other reason. I haven't looked into this yet in detail but I plan to do that soon.

Change #1281868 had a related patch set uploaded (by SomeRandomDeveloper; author: SomeRandomDeveloper):

[mediawiki/extensions/TitleBlacklist@master] Use MultiContentSave hook instead of EditFilter

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

Change #1281872 had a related patch set uploaded (by SomeRandomDeveloper; author: SomeRandomDeveloper):

[mediawiki/extensions/SpamBlacklist@master] Use MultiContentSave hook instead of EditFilter

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

Change #1281874 had a related patch set uploaded (by SomeRandomDeveloper; author: SomeRandomDeveloper):

[mediawiki/core@master] Deprecate EditFilter hook

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

Change #1281876 had a related patch set uploaded (by SomeRandomDeveloper; author: SomeRandomDeveloper):

[mediawiki/core@master] Deprecate EditPageBeforeConflictDiff hook

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

Change #1281868 merged by jenkins-bot:

[mediawiki/extensions/TitleBlacklist@master] Use MultiContentSave hook instead of EditFilter

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

Change #1281872 merged by jenkins-bot:

[mediawiki/extensions/SpamBlacklist@master] Use MultiContentSave hook instead of EditFilter

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

Change #1281874 merged by jenkins-bot:

[mediawiki/core@master] Deprecate EditFilter hook

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

Change #1281941 had a related patch set uploaded (by SomeRandomDeveloper; author: SomeRandomDeveloper):

[mediawiki/extensions/MassMessage@master] Avoid use of EditFilter hook in unit test

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

Change #1281941 merged by jenkins-bot:

[mediawiki/extensions/MassMessage@master] Avoid use of EditFilter hook in unit test

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

Change #1283071 had a related patch set uploaded (by SomeRandomDeveloper; author: SomeRandomDeveloper):

[mediawiki/core@master] Move EditFilter hook deprecation to 1.46

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

Change #1283072 had a related patch set uploaded (by SomeRandomDeveloper; author: SomeRandomDeveloper):

[mediawiki/core@REL1_46] Deprecate EditFilter hook

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

Change #1283075 had a related patch set uploaded (by SomeRandomDeveloper; author: SomeRandomDeveloper):

[mediawiki/extensions/TitleBlacklist@REL1_46] Use MultiContentSave hook instead of EditFilter

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

Change #1283076 had a related patch set uploaded (by SomeRandomDeveloper; author: SomeRandomDeveloper):

[mediawiki/extensions/SpamBlacklist@REL1_46] Use MultiContentSave hook instead of EditFilter

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

Change #1283077 had a related patch set uploaded (by SomeRandomDeveloper; author: SomeRandomDeveloper):

[mediawiki/extensions/MassMessage@REL1_46] Avoid use of EditFilter hook in unit test

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

Change #1283071 merged by jenkins-bot:

[mediawiki/core@master] Move EditFilter hook deprecation to 1.46

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

Change #1283072 merged by jenkins-bot:

[mediawiki/core@REL1_46] Deprecate EditFilter hook

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

Change #1283077 merged by jenkins-bot:

[mediawiki/extensions/MassMessage@REL1_46] Avoid use of EditFilter hook in unit test

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

Change #1283076 merged by jenkins-bot:

[mediawiki/extensions/SpamBlacklist@REL1_46] Use MultiContentSave hook instead of EditFilter

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

Change #1283075 merged by jenkins-bot:

[mediawiki/extensions/TitleBlacklist@REL1_46] Use MultiContentSave hook instead of EditFilter

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

Change #1283088 had a related patch set uploaded (by SomeRandomDeveloper; author: SomeRandomDeveloper):

[mediawiki/core@master] Remove EditFilter hook

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

Change #1283088 merged by jenkins-bot:

[mediawiki/core@master] Remove EditFilter hook

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