There's been some discussion at mediawiki-api about what we should do with hooks in the API. There are currently three problems: 1. hooks that are supposed to be run aren't (such as ArticleDelete and ArticleDeleteComplete before yesterday 2. some hooks that are run can mess up (such as AlternateEdit) 3. the 'hookaborted' error the API returns doesn't include details as to who aborted the request and why
What I, personally, consider to be the best approach is to add new hooks to the API modules, which provide a possibility for extensions to abort the request and override the returned result. The only example thus far is the APIEditBeforeSave hook (includes/api/ApiEditPage.php:132). Other hooks should then be prevented from doing UI-specific things as much as possible. The AlternateEdit hook, for instance, has no place in an API request, as its very purpose is to override the edit form.
I've done an analysis of which hooks are run at which API request, and whether I think they belong there. A report of my findings (basically a list of hooks run at a certain request) can be found at the end of this message.
Most initialization hooks (most notably the SpecialPage and LogPage* hooks) are unnecessary for query requests (and also for many regular UI requests). Shouldn't we lazy-load special page names and LogPage data? The other lists look harmless, except for action=edit, which has the AlternateEdit hook, which doesn't belong there. AlternateEdit serves to replace the edit form. Extensions like AssertEdit that want to do something for every action=edit request will have to use the right hooks (APIEditPageBeforeSave or EditPage::attemptSave or whatever).
In general, I think most problems with action=edit are caused by the fact that ApiEditPage currently just fools EditPage with a FauxRequest object. There's a lot of mixture of UI and edit logic in EditPage, and I think the best solution would be to split off the edit logic into a new class. I'll be working on that in the ApiEdit_Vodafone branch for the next few days, and I hope I can finish it before I go away for a week on June 4th.
Roan Kattouw (Catrope)
Initialization hooks: (run for every API request, even action=query and action=help) - 'AuthPluginSetup' in Setup.php:258 - SpecialPage_initlist' in SpecialPage::initList() - 'UserLoadFromSession' in User::loadFromSession() - 'IsTrustedProxy' in wfIsTrustedProxy() - 'LogPageValidTypes' in Setup.php:295 - 'LogPageLogName' in Setup.php:296 - 'LogPageLogHeader' in Setup.php:297 - 'LogPageActionText' in Setup.php:298 - 'UserEffectiveGroups' in User::getEffectiveGroups() - 'UserGetRights' in User::getRights()
action=rollback runs the following hooks, in addition to the initialization hooks and parser-related hooks I omitted for clarity: - 'userCan' in Title::getUserPermissionsErrorInternal() - 'getUserPermissionsErrors' in the same function - 'getUserPermissionsErrorsExpensive' in the same function - 'GetBlockedStatus' in User::getBlockedStatus() - 'PingLimiter' in User::pingLimiter() - 'ArticleSave' in Article::doEdit() - 'ArticlePageDataBefore' in Article::pageData() - 'ArticlePageDataAfter' in the same function - 'ArticleAfterFetchContent' in Article::fetchContent() - 'RevisionInsertComplete' in Revision::insertOn() - 'NewRevisionFromEditComplete' in Article::doEdit() - 'UserArrayFromResult' in UserArray::newFromResult() - 'RecentChange_save' in RecentChange::save() - 'ArticleEditUpdatesDeleteFromRecentchanges' in Article::editUpdates() - 'SearchUpdate' in SearchUpdate::doUpdate() - 'ArticleSaveComplete' in Article::doEdit() - 'ArticleRollbackComplete' in Article::commitRollback()
action=delete: - 'userCan' in Title::getUserPermissionsErrorInternal() - 'getUserPermissionsErrors' in the same function - 'getUserPermissionsErrorsExpensive' in the same function - 'GetBlockedStatus' in User::getBlockedStatus() - 'ArticleDelete' in ApiDelete::delete() - 'ArticlePageDataBefore' in Article::pageData() - 'ArticlePageDataAfter' in the same function - 'ArticleAfterFetchContent' in Article::fetchContent() - 'UserArrayFromResult' in UserArray::newFromResult() - 'RecentChange_save' in RecentChange::save() - 'ArticleDeleteComplete' in ApiDelete::delete()
action=undelete: - 'GetBlockedStatus' in User::getBlockedStatus() - 'RevisionInsertComplete' in Revision::insertOn() (for every restored revision) - 'ArticleRevisionUndeleted' in PageArchive::undeleteRevisions() (for every restored revision) - 'userCan' in Title::getUserPermissionsErrorsInternal() - 'getUserPermissionsErrors' in Title::getUserPermissionsErrorsInternal() - 'ArticlePageDataBefore' in Article::pageData() - 'ArticlePageDataAfter' in the same function - 'ArticleAfterFetchContent' in Article::fetchContent() - 'ArticleEditUpdatesDeleteFromRecentchanges' in Article::editUpdates() - 'ArticleUndelete' in PageArchive::undeleteRevisions() - 'UserArrayFromResult' in UserArray::newFromResult() - 'RecentChange_save' in RecentChange::save() - 'SearchUpdate' in SearchUpdate::doUpdate()
action=protect: - 'GetBlockedStatus' in User::getBlockedStatus() - 'userCan' in Title::getUserPermissionsErrorInternal() - 'getUserPermissionsErrors' in the same function - 'getUserPermissionsErrorsExpensive' in the same function - 'ArticleProtect' in Article::updateRestrictions() - 'RevisionInsertComplete' in Revision::insertOn() - 'NewRevisionFromEditComplete' in Article::updateRestrictions() - 'ArticleProtectComplete' in Article::updateRestrictions() - 'UserArrayFromResult' in UserArray::newFromResult() - 'RecentChange_save' in RecentChange::save()
action=block: - 'BlockIp' in IPBlockForm::doBlock() - 'BlockIpComplete' in IPBlockForm::doBlock() - 'UserArrayFromResult' in UserArray::newFromResult() - 'RecentChange_save' in RecentChange::save()
action=unblock: - 'UserArrayFromResult' in UserArray::newFromResult - 'RecentChange_save' in RecentChange->save
action=move: - 'userCan' in Title->getUserPermissionsErrorsInternal - 'getUserPermissionsErrors' in Title->getUserPermissionsErrorsInternal - 'getUserPermissionsErrorsExpensive' in Title->getUserPermissionsErrorsInternal - 'GetBlockedStatus' in User->getBlockedStatus - 'AbortMove' in Title->isValidMoveOperation - 'RevisionInsertComplete' in Revision->insertOn (twice) - 'NewRevisionFromEditComplete' in Title->moveToNewTitle (twice) - 'UserArrayFromResult' in UserArray::newFromResult - 'RecentChange_save' in RecentChange->save - 'SearchUpdate' in SearchUpdate->doUpdate - 'TitleMoveComplete' in Title->moveTo Note: this used to run AbortMove twice, once in ApiMove::execute() (where it doesn't belong) and once in Title::isValidMoveOperation() (where it does belong)
action=edit: - 'userCan' in Title::getUserPermissionsErrorsInternal() - 'getUserPermissionsErrors' in Title::>getUserPermissionsErrorsInternal() - 'getUserPermissionsErrorsExpensive' in Title::getUserPermissionsErrorsInternal() - 'GetBlockedStatus' in User::getBlockedStatus() - 'AlternateEdit' in ApiEditPage::execute() - 'APIEditBeforeSave' in ApiEditPage::execute() - 'EditPage::attemptSave' in EditPage::internalAttemptSave() - 'EditFilter' in EditPage::internalAttemptSave() - 'PingLimiter' in User::pingLimiter() - 'ArticlePageDataBefore' in Article::pageData() - 'ArticlePageDataAfter' in Article::pageData() - 'ArticleAfterFetchContent' in Article::fetchContent() - 'EditFilterMerged' in EditPage::internalAttemptSave() - 'ArticleSave' in Article::doEdit() - 'RevisionInsertComplete' in Revision::insertOn() - 'NewRevisionFromEditComplete' in Article::doEdit() - 'UserArrayFromResult' in UserArray::newFromResult() - 'RecentChange_save' in RecentChange::save() - 'ArticleEditUpdatesDeleteFromRecentchanges' in Article::editUpdates() - 'ArticleSaveComplete' in Article::doEdit() - 'ArticleUpdateBeforeRedirect' in Article::updateArticle() - 'GetLocalURL' in Title::getLocalURL() - 'GetFullURL' in Title::getFullURL() - 'SearchUpdate' in SearchUpdate::doUpdate()
Roan Kattouw wrote:
In general, I think most problems with action=edit are caused by the fact that ApiEditPage currently just fools EditPage with a FauxRequest object. There's a lot of mixture of UI and edit logic in EditPage, and I think the best solution would be to split off the edit logic into a new class.
Yes, that seems pretty obvious. We should not have two redundant implementations of page editing, but simply two interfaces to the same underlying code.
Looking at it from a MVC viewpoint, most of the API code in the past has been pure View code, presenting data to the user but not modifying it. Duplication there has not been an issue, especially as the view provided by the API often differs significantly from that provided by the HTML interface. Editing, however, involves Controller and Model code, and you really do not want to have multiple redundant model implementations. The right thing to do is to factor out the model (including permissions checks etc.) into a separate module, for which both the API and EditPage can serve as controllers and views.
Ilmari Karonen schreef:
The right thing to do is to factor out the model (including permissions checks etc.) into a separate module, for which both the API and EditPage can serve as controllers and views.
Exactly. Right now, we just abuse EditPage::attemptInternalSave() as what you call the model, but the problem is it's so integrated with the UI part of EditPage that it causes all kinds of weird bugs. This would also be a nice opportunity to improve conflict resolution (especially the case when separate sections are edited).
Roan Kattouw (Catrope)
On Tue, May 27, 2008 at 6:13 PM, Roan Kattouw roan.kattouw@home.nl wrote:
There's been some discussion at mediawiki-api about what we should do with hooks in the API. There are currently three problems:
- hooks that are supposed to be run aren't (such as ArticleDelete and
ArticleDeleteComplete before yesterday 2. some hooks that are run can mess up (such as AlternateEdit) 3. the 'hookaborted' error the API returns doesn't include details as to who aborted the request and why
What I, personally, consider to be the best approach is to add new hooks to the API modules, which provide a possibility for extensions to abort the request and override the returned result. The only example thus far is the APIEditBeforeSave hook (includes/api/ApiEditPage.php:132). Other hooks should then be prevented from doing UI-specific things as much as possible. The AlternateEdit hook, for instance, has no place in an API request, as its very purpose is to override the edit form.
This should be done for hooks that are normally used to change the interface. We should try to trigger as many core hooks as possible for compatibility issues.
Most initialization hooks (most notably the SpecialPage and LogPage* hooks) are unnecessary for query requests (and also for many regular UI requests). Shouldn't we lazy-load special page names and LogPage data? The other lists look harmless, except for action=edit, which has the AlternateEdit hook, which doesn't belong there. AlternateEdit serves to replace the edit form. Extensions like AssertEdit that want to do something for every action=edit request will have to use the right hooks (APIEditPageBeforeSave or EditPage::attemptSave or whatever).
The problem arises that in some cases only interface hooks exist and not clean backend hooks. IIRC this is for example SpecialMovepageAftermove or something like that. There is no ArticleMoveComplete hook, which leads to the fact that the SpecialMovepage hook is used for both interface as backend hooks. That probably needs fixing. Instead of the AlternateEdit hooks there should be a generic BeforeArticleSave hook.
Bryan Tong Minh schreef:
The problem arises that in some cases only interface hooks exist and not clean backend hooks. IIRC this is for example SpecialMovepageAftermove or something like that. There is no ArticleMoveComplete hook, which leads to the fact that the SpecialMovepage hook is used for both interface as backend hooks. That probably needs fixing. Instead of the AlternateEdit hooks there should be a generic BeforeArticleSave hook.
There is the TitleMoveComplete hook, I see no reason why that couldn't be used. There's also ArticleSave and ArticleSaveComplete, so I think we have all those hooks already. We have backend hooks all over the place; the only thing that doesn't have any hooks (neither backend nor UI) is unblock.
Roan Kattouw (Catrope)
wikitech-l@lists.wikimedia.org