From 9cf84278651d67ffb98336ffd65b2f5df11d776d Mon Sep 17 00:00:00 2001 From: Edward Chernenko Date: Mon, 15 Oct 2018 13:37:39 +0300 Subject: [PATCH] MCR: provide MultiContentSave hook to replace PageContentSave Old hook PageContentSave only received the main slot of the article. In order for extensions to intercept changes in all slots, they should receive a RevisionSlots object with new content. PageContentSave hook is still preserved for backward compatibility. Note: parameters $iswatch and $section (from PageContentSave) are not added into the new hook, because they are obsolete in the old hook. Note: new hook also doesn't unnecessarily pass-by-reference (T193950), which was an issue with the old hook. Bug: T205982 Change-Id: I55455639c5ff6ccbf3917d85c1ab0dac847ec853 --- docs/hooks.txt | 18 +++- includes/Storage/PageUpdater.php | 32 ++++--- .../includes/Storage/PageUpdaterTest.php | 96 +++++++++++++++++++ 3 files changed, 134 insertions(+), 12 deletions(-) diff --git a/docs/hooks.txt b/docs/hooks.txt index 621ec8ea920..2ee3b272152 100644 --- a/docs/hooks.txt +++ b/docs/hooks.txt @@ -2335,6 +2335,21 @@ $oldTitle: Title object of the current (old) location $newTitle: Title object of the new location $status: Status object to pass error messages to +'MultiContentSave': Before an article is saved. +$renderedRevision: RenderedRevision (object) representing the planned revision. + Provides access to: (1) ParserOutput of all slots, + (2) RevisionRecord, which contains + (2a) Title of the page that will be edited, + (2b) RevisionSlots - content of all slots, including inherited slots, + where content has already been modified by PreSaveTransform. + NOTE: because this revision is not yet saved, slots don't have content ID + or address, and revision itself doesn't have an ID. +$user: UserIdentity (object): author of this new revision. +$summary: CommentStoreComment (object): user-provided edit comment/summary +(not an autosummary: will be empty if the user hasn't provided a comment). +$flags: combination of EDIT_* flags, e.g. EDIT_MINOR. +$status: if the hook is aborted, error code can be placed into this Status. + 'NamespaceIsMovable': Called when determining if it is possible to pages in a namespace. $index: Integer; the index of the namespace being checked. @@ -2460,7 +2475,8 @@ $title: Title object strings are deprecated. $userLang: the user language (Language object) -'PageContentSave': Before an article is saved. +'PageContentSave': DEPRECATED since 1.35! Use MultiContentSave instead. +Before an article is saved. [&]$wikiPage: the WikiPage (object) being saved [&]$user: the user (object) saving the article [&]$content: the new article content, as a Content object diff --git a/includes/Storage/PageUpdater.php b/includes/Storage/PageUpdater.php index 098d3dcb2e8..a25e54a6b23 100644 --- a/includes/Storage/PageUpdater.php +++ b/includes/Storage/PageUpdater.php @@ -692,9 +692,6 @@ class PageUpdater { $useStashed = $this->ajaxEditStash; } - // TODO: use this only for the legacy hook, and only if something uses the legacy hook - $wikiPage = $this->getWikiPage(); - $user = $this->user; // Prepare the update. This performs PST and generates the canonical ParserOutput. @@ -734,16 +731,29 @@ class PageUpdater { */ $this->derivedDataUpdater->getCanonicalParserOutput(); - $mainContent = $this->derivedDataUpdater->getSlots()->getContent( SlotRecord::MAIN ); - // Trigger pre-save hook (using provided edit summary) + $renderedRevision = $this->derivedDataUpdater->getRenderedRevision(); $hookStatus = Status::newGood( [] ); - // TODO: replace legacy hook! - // TODO: avoid pass-by-reference, see T193950 - // Check if the hook rejected the attempted save - if ( !Hooks::run( 'PageContentSave', [ &$wikiPage, &$user, &$mainContent, &$summary, - $flags & EDIT_MINOR, null, null, &$flags, &$hookStatus ] ) - ) { + $allowedByHook = Hooks::run( 'MultiContentSave', [ + $renderedRevision, $user, $summary, $flags, $hookStatus + ] ); + if ( $allowedByHook && Hooks::isRegistered( 'PageContentSave' ) ) { + // Also run the legacy hook. + // TODO: avoid pass-by-reference, see T193950 + // NOTE: WikiPage should only be used for the legacy hook, + // and only if something uses the legacy hook. + $mainContent = $this->derivedDataUpdater->getSlots()->getContent( SlotRecord::MAIN ); + $wikiPage = $this->getWikiPage(); + + // Deprecated since 1.35. + $allowedByHook = Hooks::run( 'PageContentSave', [ + &$wikiPage, &$user, &$mainContent, &$summary, + $flags & EDIT_MINOR, null, null, &$flags, &$hookStatus + ] ); + } + + if ( !$allowedByHook ) { + // The hook has prevented this change from being saved. if ( $hookStatus->isOK() ) { // Hook returned false but didn't call fatal(); use generic message $hookStatus->fatal( 'edit-hook-aborted' ); diff --git a/tests/phpunit/includes/Storage/PageUpdaterTest.php b/tests/phpunit/includes/Storage/PageUpdaterTest.php index c2f7d8c700b..4ce6cd6a9c6 100644 --- a/tests/phpunit/includes/Storage/PageUpdaterTest.php +++ b/tests/phpunit/includes/Storage/PageUpdaterTest.php @@ -5,12 +5,14 @@ namespace MediaWiki\Tests\Storage; use CommentStoreComment; use Content; use MediaWiki\MediaWikiServices; +use MediaWiki\Revision\RenderedRevision; use MediaWiki\Revision\RevisionRecord; use MediaWiki\Revision\SlotRecord; use MediaWikiTestCase; use ParserOptions; use RecentChange; use Revision; +use Status; use TextContent; use Title; use User; @@ -260,6 +262,100 @@ class PageUpdaterTest extends MediaWikiTestCase { return $rev; } + /** + * Verify that MultiContentSave hook is called by saveRevision() with correct parameters. + * @covers \MediaWiki\Storage\PageUpdater::saveRevision() + */ + public function testMultiContentSaveHook() { + $user = $this->getTestUser()->getUser(); + $title = $this->getDummyTitle( __METHOD__ ); + + // TODO: MCR: test additional slots + $slots = [ + SlotRecord::MAIN => new TextContent( 'Lorem Ipsum' ) + ]; + + // start editing non-existing page + $page = WikiPage::factory( $title ); + $updater = $page->newPageUpdater( $user ); + foreach ( $slots as $slot => $content ) { + $updater->setContent( $slot, $content ); + } + + $summary = CommentStoreComment::newUnsavedComment( 'Just a test' ); + + $expected = [ + 'user' => $user, + 'title' => $title, + 'slots' => $slots, + 'summary' => $summary + ]; + $hookFired = false; + $this->setTemporaryHook( 'MultiContentSave', + function ( RenderedRevision $renderedRevision, User $user, + $summary, $flags, Status $hookStatus + ) use ( &$hookFired, $expected ) { + $hookFired = true; + + $this->assertSame( $expected['summary'], $summary ); + $this->assertSame( EDIT_NEW, $flags ); + + $title = $renderedRevision->getRevision()->getPageAsLinkTarget(); + $this->assertSame( $expected['title']->getFullText(), $title->getFullText() ); + + $slots = $renderedRevision->getRevision()->getSlots(); + foreach ( $expected['slots'] as $slot => $content ) { + $this->assertSame( $content, $slots->getSlot( $slot )->getContent() ); + } + + // Don't abort this edit. + return true; + } + ); + + $rev = $updater->saveRevision( $summary ); + $this->assertTrue( $hookFired, "MultiContentSave hook wasn't called." ); + $this->assertNotNull( $rev, + "MultiContentSave returned true, but revision wasn't created." ); + } + + /** + * Verify that MultiContentSave hook can abort saveRevision() by returning false. + * @covers \MediaWiki\Storage\PageUpdater::saveRevision() + */ + public function testMultiContentSaveHookAbort() { + $user = $this->getTestUser()->getUser(); + $title = $this->getDummyTitle( __METHOD__ ); + + // start editing non-existing page + $page = WikiPage::factory( $title ); + $updater = $page->newPageUpdater( $user ); + $updater->setContent( SlotRecord::MAIN, new TextContent( 'Lorem Ipsum' ) ); + + $summary = CommentStoreComment::newUnsavedComment( 'Just a test' ); + + $expectedError = 'aborted-by-test-hook'; + $this->setTemporaryHook( 'MultiContentSave', + function ( RenderedRevision $renderedRevision, User $user, + $summary, $flags, Status $hookStatus + ) use ( $expectedError ) { + $hookStatus->fatal( $expectedError ); + + // Returning false should disallow saveRevision() to continue saving this revision. + return false; + } + ); + + $rev = $updater->saveRevision( $summary ); + $this->assertNull( $rev, + "MultiContentSave returned false, but revision was still created." ); + + $status = $updater->getStatus(); + $this->assertFalse( $status->isOK(), + "MultiContentSave returned false, but Status is not fatal." ); + $this->assertSame( $expectedError, $status->getMessage()->getKey() ); + } + /** * @covers \MediaWiki\Storage\PageUpdater::grabParentRevision() * @covers \MediaWiki\Storage\PageUpdater::saveRevision()