From 7960d5385f1146459671148e2e34d3ba50e17aeb Mon Sep 17 00:00:00 2001 From: daniel Date: Fri, 9 Mar 2018 23:05:47 +0100 Subject: [PATCH] Introduce ContentHandler::getSecondaryDataUpdates. This adds getSecondaryDataUpdates and getDeletionUpdates to ContentHandler, and updates WikiPage and DerivedPageDataUpdates to handle DataUpdates from all slots. Bug: T194038 Bug: T194037 Change-Id: I75c96318f58a5cdda48484f7040ae41e6f42392a --- RELEASE-NOTES-1.32 | 12 ++ autoload.php | 1 + docs/hooks.txt | 26 ++- includes/Revision/RenderedRevision.php | 2 +- includes/Revision/SlotRenderingProvider.php | 32 ++++ includes/Storage/DerivedPageDataUpdater.php | 178 +++++++++++++----- includes/content/Content.php | 13 +- includes/content/ContentHandler.php | 72 +++++++ includes/page/Article.php | 9 +- includes/page/WikiPage.php | 75 ++++++-- .../Storage/DerivedPageDataUpdaterTest.php | 109 ++++++++++- .../content/WikitextContentHandlerTest.php | 27 +++ .../includes/page/WikiPageDbTestBase.php | 110 ++++++++++- .../page/WikiPageMcrReadNewDbTest.php | 25 +++ .../page/WikiPageNoContentModelDbTest.php | 20 ++ 15 files changed, 625 insertions(+), 86 deletions(-) create mode 100644 includes/Revision/SlotRenderingProvider.php diff --git a/RELEASE-NOTES-1.32 b/RELEASE-NOTES-1.32 index ff59d433ea..fd670a7e56 100644 --- a/RELEASE-NOTES-1.32 +++ b/RELEASE-NOTES-1.32 @@ -412,6 +412,18 @@ because of Phabricator reports. 'help', 'help-message', 'help-messages' instead. * (T197179) HTMLFormField::getNotices() is now deprecated. * The jquery.localize module is now deprecated. Use jquery.i18n instead. +* The SecondaryDataUpdates hook was deprecated in favor of RevisionDataUpdates, + or overriding ContentHandler::getSecondaryDataUpdates (T194038). +* The WikiPageDeletionUpdates hook was deprecated in favor of + PageDeletionDataUpdates, or overriding ContentHandler::getDeletionDataUpdates + (T194038). +* Content::getSecondaryDataUpdates has been deprecated in favor of + ContentHandler::getSecondaryDataUpdates() for overriding by extensions + (T194038). + Application logic should call WikiPage::doSecondaryDataUpdates() (T194037). +* Content::getDeletionUpdates has been deprecated in favor of + ContentHandler::getDeletionUpdates() for overriding by extensions (T194038). + Application logic should call WikiPage::doSecondaryDataUpdates() (T194037). === Other changes in 1.32 === * (T198811) The following tables have had their UNIQUE indexes turned into diff --git a/autoload.php b/autoload.php index dc9461edc7..45e2364b45 100644 --- a/autoload.php +++ b/autoload.php @@ -898,6 +898,7 @@ $wgAutoloadLocalClasses = [ 'MediaWiki\\ProcOpenError' => __DIR__ . '/includes/exception/ProcOpenError.php', 'MediaWiki\\Revision\\RenderedRevision' => __DIR__ . '/includes/Revision/RenderedRevision.php', 'MediaWiki\\Revision\\RevisionRenderer' => __DIR__ . '/includes/Revision/RevisionRenderer.php', + 'MediaWiki\\Revision\\SlotRenderingProvider' => __DIR__ . '/includes/Revision/SlotRenderingProvider.php', 'MediaWiki\\Search\\ParserOutputSearchDataExtractor' => __DIR__ . '/includes/search/ParserOutputSearchDataExtractor.php', 'MediaWiki\\ShellDisabledError' => __DIR__ . '/includes/exception/ShellDisabledError.php', 'MediaWiki\\Site\\MediaWikiPageNameNormalizer' => __DIR__ . '/includes/site/MediaWikiPageNameNormalizer.php', diff --git a/docs/hooks.txt b/docs/hooks.txt index 436131cdd8..cfe696fe94 100644 --- a/docs/hooks.txt +++ b/docs/hooks.txt @@ -2537,6 +2537,12 @@ $originalRevId: if the edit restores or repeats an earlier revision (such as a (Used to be called $baseRevId.) $undidRevId: the rev ID (or 0) this edit undid +'PageDeletionDataUpdates': Called when constructing a list of DeferrableUpdate to be +executed when a page is deleted. +$title The Title of the page being deleted. +$revision A RevisionRecord representing the page's current revision at the time of deletion. +&$updates A list of DeferrableUpdate that can be manipulated by the hook handler. + 'PageHistoryBeforeList': When a history page list is about to be constructed. &$article: the article that the history is loading for $context: RequestContext object @@ -2910,6 +2916,13 @@ called after the addition of 'qunit' and MediaWiki testing resources. added to any module. &$ResourceLoader: object +'RevisionDataUpdates': Called when constructing a list of DeferrableUpdate to be +executed to record secondary data about a revision. +$title The Title of the page the revision belongs to +$renderedRevision a RenderedRevision object representing the new revision and providing access + to the RevisionRecord as well as ParserOutput of that revision. +&$updates A list of DeferrableUpdate that can be manipulated by the hook handler. + 'RevisionRecordInserted': Called after a revision is inserted into the database. $revisionRecord: the RevisionRecord that has just been inserted. @@ -2969,9 +2982,9 @@ result augmentors. Note that lists should be in the format name => object and the names in both lists should be distinct. -'SecondaryDataUpdates': Allows modification of the list of DataUpdates to -perform when page content is modified. Currently called by -AbstractContent::getSecondaryDataUpdates. +'SecondaryDataUpdates': DEPRECATED! Use RevisionDataUpdates or override +ContentHandler::getSecondaryDataUpdates instead. +Allows modification of the list of DataUpdates to perform when page content is modified. $title: Title of the page that is being edited. $oldContent: Content object representing the page's content before the edit. $recursive: bool indicating whether DataUpdates should trigger recursive @@ -4045,10 +4058,9 @@ dumps. One, and only one hook should set this, and return false. &$opts: Options to use for the query &$join: Join conditions -'WikiPageDeletionUpdates': manipulate the list of DeferrableUpdates to be -applied when a page is deleted. Called in WikiPage::getDeletionUpdates(). Note -that updates specific to a content model should be provided by the respective -Content's getDeletionUpdates() method. +'WikiPageDeletionUpdates': DEPRECATED! Use PageDeletionDataUpdates or +override ContentHandler::getDeletionDataUpdates instead. +Manipulates the list of DeferrableUpdates to be applied when a page is deleted. $page: the WikiPage $content: the Content to generate updates for, or null in case the page revision could not be loaded. The delete will succeed despite this. diff --git a/includes/Revision/RenderedRevision.php b/includes/Revision/RenderedRevision.php index 0c052d1b46..e6e9259bcb 100644 --- a/includes/Revision/RenderedRevision.php +++ b/includes/Revision/RenderedRevision.php @@ -42,7 +42,7 @@ use Wikimedia\Assert\Assert; * * @since 1.32 */ -class RenderedRevision { +class RenderedRevision implements SlotRenderingProvider { /** * @var Title diff --git a/includes/Revision/SlotRenderingProvider.php b/includes/Revision/SlotRenderingProvider.php new file mode 100644 index 0000000000..740f0f2c9f --- /dev/null +++ b/includes/Revision/SlotRenderingProvider.php @@ -0,0 +1,32 @@ + bool: Whether the caller is interested in output HTML (as opposed + * to just meta-data). Default is to generate HTML. + * + * @throws SuppressedDataException if the content is not accessible for the audience + * specified in the constructor. + * @return ParserOutput + */ + public function getSlotParserOutput( $role, array $hints = [] ); + +} diff --git a/includes/Storage/DerivedPageDataUpdater.php b/includes/Storage/DerivedPageDataUpdater.php index 99c31b2d69..4252ba828c 100644 --- a/includes/Storage/DerivedPageDataUpdater.php +++ b/includes/Storage/DerivedPageDataUpdater.php @@ -27,12 +27,14 @@ use CategoryMembershipChangeJob; use Content; use ContentHandler; use DataUpdate; +use DeferrableUpdate; use DeferredUpdates; use Hooks; use IDBAccessObject; use InvalidArgumentException; use JobQueueGroup; use Language; +use LinksDeletionUpdate; use LinksUpdate; use LogicException; use MediaWiki\Edit\PreparedEdit; @@ -165,8 +167,7 @@ class DerivedPageDataUpdater implements IDBAccessObject { * * Contains the following fields: * - oldRevision (RevisionRecord|null): the revision that was current before the change - * associated with this update. Might not be set, use getOldRevision() instead of direct - * access. + * associated with this update. Might not be set, use getParentRevision(). * - oldId (int|null): the id of the above revision. 0 if there is no such revision (the change * was about creating a new page); null if not known (that should not happen). * - oldIsRedirect (bool|null): whether the page was a redirect before the change. Lazy-loaded, @@ -183,6 +184,11 @@ class DerivedPageDataUpdater implements IDBAccessObject { */ private $slotsUpdate = null; + /** + * @var RevisionRecord|null + */ + private $parentRevision = null; + /** * @var RevisionRecord|null */ @@ -456,29 +462,34 @@ class DerivedPageDataUpdater implements IDBAccessObject { } /** - * Returns the revision that was current before the edit. This would be null if the edit - * created the page, or the revision's parent for a regular edit, or the revision itself - * for a null-edit. - * Only defined after calling grabCurrentRevision() or prepareContent() or prepareUpdate()! + * Returns the parent revision of the new revision wrapped by this update. + * If the update is a null-edit, this will return the parent of the current (and new) revision. + * This will return null if the revision wrapped by this update created the page. + * Only defined after calling prepareContent() or prepareUpdate()! * - * @return RevisionRecord|null the revision that was current before the edit, or null if - * the edit created the page. + * @return RevisionRecord|null the parent revision of the new revision, or null if + * the update created the page. */ - private function getOldRevision() { - $this->assertHasPageState( __METHOD__ ); + private function getParentRevision() { + $this->assertPrepared( __METHOD__ ); - // If 'oldRevision' is not set, load it! - // Useful if $this->oldPageState is initialized by prepareUpdate. - if ( !array_key_exists( 'oldRevision', $this->pageState ) ) { - /** @var int $oldId */ - $oldId = $this->pageState['oldId']; - $flags = $this->useMaster() ? RevisionStore::READ_LATEST : 0; - $this->pageState['oldRevision'] = $oldId - ? $this->revisionStore->getRevisionById( $oldId, $flags ) - : null; + if ( $this->parentRevision ) { + return $this->parentRevision; } - return $this->pageState['oldRevision']; + if ( !$this->pageState['oldId'] ) { + // If there was no current revision, there is no parent revision, + // since the page didn't exist. + return null; + } + + $oldId = $this->revision->getParentId(); + $flags = $this->useMaster() ? RevisionStore::READ_LATEST : 0; + $this->parentRevision = $oldId + ? $this->revisionStore->getRevisionById( $oldId, $flags ) + : null; + + return $this->parentRevision; } /** @@ -495,8 +506,8 @@ class DerivedPageDataUpdater implements IDBAccessObject { * @note After prepareUpdate() was called, grabCurrentRevision() will throw an exception * to avoid confusion, since the page's current revision is then the new revision after * the edit, which was presumably passed to prepareUpdate() as the $revision parameter. - * Use getOldRevision() instead to access the revision that used to be current before the - * edit. + * Use getParentRevision() instead to access the revision that is the parent of the + * new revision. * * @return RevisionRecord|null the page's current revision, or null if the page does not * yet exist. @@ -834,6 +845,8 @@ class DerivedPageDataUpdater implements IDBAccessObject { // prepareUpdate() is redundant for null-edits $this->doTransition( 'has-revision' ); + } else { + $this->parentRevision = $parentRevision; } } @@ -969,7 +982,7 @@ class DerivedPageDataUpdater implements IDBAccessObject { $this->assertPrepared( __METHOD__ ); if ( !$this->slotsUpdate ) { - $old = $this->getOldRevision(); + $old = $this->getParentRevision(); $this->slotsUpdate = RevisionSlotsUpdate::newFromRevisionSlots( $this->revision->getSlots(), $old ? $old->getSlots() : null @@ -1252,34 +1265,103 @@ class DerivedPageDataUpdater implements IDBAccessObject { /** * @param bool $recursive * - * @return DataUpdate[] + * @return DeferrableUpdate[] */ public function getSecondaryDataUpdates( $recursive = false ) { - // TODO: MCR: getSecondaryDataUpdates() needs a complete overhaul to avoid DataUpdates - // from different slots overwriting each other in the database. Plan: - // * replace direct calls to Content::getSecondaryDataUpdates() with calls to this method - // * Construct LinksUpdate here, on the combined ParserOutput, instead of in AbstractContent - // for each slot. - // * Pass $slot into getSecondaryDataUpdates() - probably be introducing a new duplicate - // version of this function in ContentHandler. - // * The new method gets the PreparedEdit, but no $recursive flag (that's for LinksUpdate) - // * Hack: call both the old and the new getSecondaryDataUpdates method here; Pass - // the per-slot ParserOutput to the old method, for B/C. - // * Hack: If there is more than one slot, filter LinksUpdate from the DataUpdates - // returned by getSecondaryDataUpdates, and use a LinksUpdated for the combined output - // instead. - // * Call the SecondaryDataUpdates hook here (or kill it - its signature doesn't make sense) - - $content = $this->getSlots()->getContent( 'main' ); - - // NOTE: $output is the combined output, to be shown in the default view. + if ( $this->isContentDeleted() ) { + // This shouldn't happen, since the current content is always public, + // and DataUpates are only needed for current content. + return []; + } + $output = $this->getCanonicalParserOutput(); - $updates = $content->getSecondaryDataUpdates( - $this->getTitle(), null, $recursive, $output + // Construct a LinksUpdate for the combined canonical output. + $linksUpdate = new LinksUpdate( + $this->getTitle(), + $output, + $recursive ); - return $updates; + $allUpdates = [ $linksUpdate ]; + + // NOTE: Run updates for all slots, not just the modified slots! Otherwise, + // info for an inherited slot may end up being removed. This is also needed + // to ensure that purges are effective. + $renderedRevision = $this->getRenderedRevision(); + foreach ( $this->getSlots()->getSlotRoles() as $role ) { + $slot = $this->getRawSlot( $role ); + $content = $slot->getContent(); + $handler = $content->getContentHandler(); + + $updates = $handler->getSecondaryDataUpdates( + $this->getTitle(), + $content, + $role, + $renderedRevision + ); + $allUpdates = array_merge( $allUpdates, $updates ); + + // TODO: remove B/C hack in 1.32! + // NOTE: we assume that the combined output contains all relevant meta-data for + // all slots! + $legacyUpdates = $content->getSecondaryDataUpdates( + $this->getTitle(), + null, + $recursive, + $output + ); + + // HACK: filter out redundant and incomplete LinksUpdates + $legacyUpdates = array_filter( $legacyUpdates, function ( $update ) { + return !( $update instanceof LinksUpdate ); + } ); + + $allUpdates = array_merge( $allUpdates, $legacyUpdates ); + } + + // XXX: if a slot was removed by an earlier edit, but deletion updates failed to run at + // that time, we don't know for which slots to run deletion updates when purging a page. + // We'd have to examine the entire history of the page to determine that. Perhaps there + // could be a "try extra hard" mode for that case that would run a DB query to find all + // roles/models ever used on the page. On the other hand, removing slots should be quite + // rare, so perhaps this isn't worth the trouble. + + // TODO: consolidate with similar logic in WikiPage::getDeletionUpdates() + $wikiPage = $this->getWikiPage(); + $parentRevision = $this->getParentRevision(); + foreach ( $this->getRemovedSlotRoles() as $role ) { + // HACK: we should get the content model of the removed slot from a SlotRoleHandler! + // For now, find the slot in the parent revision - if the slot was removed, it should + // always exist in the parent revision. + $parentSlot = $parentRevision->getSlot( $role, RevisionRecord::RAW ); + $content = $parentSlot->getContent(); + $handler = $content->getContentHandler(); + + $updates = $handler->getDeletionUpdates( + $this->getTitle(), + $role + ); + $allUpdates = array_merge( $allUpdates, $updates ); + + // TODO: remove B/C hack in 1.32! + $legacyUpdates = $content->getDeletionUpdates( $wikiPage ); + + // HACK: filter out redundant and incomplete LinksDeletionUpdate + $legacyUpdates = array_filter( $legacyUpdates, function ( $update ) { + return !( $update instanceof LinksDeletionUpdate ); + } ); + + $allUpdates = array_merge( $allUpdates, $legacyUpdates ); + } + + // TODO: hard deprecate SecondaryDataUpdates in favor of RevisionDataUpdates in 1.33! + Hooks::run( + 'RevisionDataUpdates', + [ $this->getTitle(), $renderedRevision, &$allUpdates ] + ); + + return $allUpdates; } /** @@ -1425,7 +1507,7 @@ class DerivedPageDataUpdater implements IDBAccessObject { WikiPage::onArticleEdit( $title, $legacyRevision, $this->getTouchedSlotRoles() ); } - $oldRevision = $this->getOldRevision(); + $oldRevision = $this->getParentRevision(); $oldLegacyRevision = $oldRevision ? new Revision( $oldRevision ) : null; // TODO: In the wiring, register a listener for this on the new PageEventEmitter @@ -1484,7 +1566,9 @@ class DerivedPageDataUpdater implements IDBAccessObject { } foreach ( $updates as $update ) { - $update->setCause( $causeAction, $causeAgent ); + if ( $update instanceof DataUpdate ) { + $update->setCause( $causeAction, $causeAgent ); + } if ( $update instanceof LinksUpdate ) { $update->setRevision( $legacyRevision ); $update->setTriggeringUser( $triggeringUser ); diff --git a/includes/content/Content.php b/includes/content/Content.php index 000bff2d64..bb3fb107d7 100644 --- a/includes/content/Content.php +++ b/includes/content/Content.php @@ -284,13 +284,8 @@ interface Content { * made to replace information about the old content with information about * the new content. * - * This default implementation calls - * $this->getParserOutput( $content, $title, null, null, false ), - * and then calls getSecondaryDataUpdates( $title, $recursive ) on the - * resulting ParserOutput object. - * - * Subclasses may implement this to determine the necessary updates more - * efficiently, or make use of information about the old content. + * @deprecated since 1.32, call and override + * ContentHandler::getSecondaryDataUpdates instead. * * @note Implementations should call the SecondaryDataUpdates hook, like * AbstractContent does. @@ -481,8 +476,10 @@ interface Content { * the current state of the database. * * @since 1.21 + * @deprecated since 1.32, call and override + * ContentHandler::getDeletionUpdates instead. * - * @param WikiPage $page The deleted page + * @param WikiPage $page The page the content was deleted from. * @param ParserOutput|null $parserOutput Optional parser output object * for efficient access to meta-information about the content object. * Provide if you have one handy. diff --git a/includes/content/ContentHandler.php b/includes/content/ContentHandler.php index 344d0406bd..b50fe237b7 100644 --- a/includes/content/ContentHandler.php +++ b/includes/content/ContentHandler.php @@ -28,6 +28,7 @@ use Wikimedia\Assert\Assert; use MediaWiki\Logger\LoggerFactory; use MediaWiki\MediaWikiServices; +use MediaWiki\Revision\SlotRenderingProvider; use MediaWiki\Search\ParserOutputSearchDataExtractor; /** @@ -1404,4 +1405,75 @@ abstract class ContentHandler { return $parserOutput; } + /** + * Returns a list of DeferrableUpdate objects for recording information about the + * given Content in some secondary data store. + * + * Application logic should not call this method directly. Instead, it should call + * DerivedPageDataUpdater::getSecondaryDataUpdates(). + * + * @note Implementations must not return a LinksUpdate instance. Instead, a LinksUpdate + * is created by the calling code in DerivedPageDataUpdater, on the combined ParserOutput + * of all slots, not for each slot individually. This is in contrast to the old + * getSecondaryDataUpdates method defined by AbstractContent, which returned a LinksUpdate. + * + * @note Implementations should not call $content->getParserOutput, they should call + * $slotOutput->getSlotRendering( $role, false ) instead if they need to access a ParserOutput + * of $content. This allows existing ParserOutput objects to be re-used, while avoiding + * creating a ParserOutput when none is needed. + * + * @param Title $title The title of the page to supply the updates for + * @param Content $content The content to generate data updates for. + * @param string $role The role (slot) in which the content is being used. Which updates + * are performed should generally not depend on the role the content has, but the + * DeferrableUpdates themselves may need to know the role, to track to which slot the + * data refers, and to avoid overwriting data of the same kind from another slot. + * @param SlotRenderingProvider $slotOutput A provider that can be used to gain access to + * a ParserOutput of $content by calling $slotOutput->getSlotParserOutput( $role, false ). + * @return DeferrableUpdate[] A list of DeferrableUpdate objects for putting information + * about this content object somewhere. The default implementation returns an empty + * array. + * @since 1.32 + */ + public function getSecondaryDataUpdates( + Title $title, + Content $content, + $role, + SlotRenderingProvider $slotOutput + ) { + return []; + } + + /** + * Returns a list of DeferrableUpdate objects for removing information about content + * in some secondary data store. This is used when a page is deleted, and also when + * a slot is removed from a page. + * + * Application logic should not call this method directly. Instead, it should call + * WikiPage::getSecondaryDataUpdates(). + * + * @note Implementations must not return a LinksDeletionUpdate instance. Instead, a + * LinksDeletionUpdate is created by the calling code in WikiPage. + * This is in contrast to the old getDeletionUpdates method defined by AbstractContent, + * which returned a LinksUpdate. + * + * @note Implementations should not rely on the page's current content, but rather the current + * state of the secondary data store. + * + * @param Title $title The title of the page to supply the updates for + * @param string $role The role (slot) in which the content is being used. Which updates + * are performed should generally not depend on the role the content has, but the + * DeferrableUpdates themselves may need to know the role, to track to which slot the + * data refers, and to avoid overwriting data of the same kind from another slot. + * + * @return DeferrableUpdate[] A list of DeferrableUpdate objects for putting information + * about this content object somewhere. The default implementation returns an empty + * array. + * + * @since 1.32 + */ + public function getDeletionUpdates( Title $title, $role ) { + return []; + } + } diff --git a/includes/page/Article.php b/includes/page/Article.php index e90334fce6..9d3e36f42f 100644 --- a/includes/page/Article.php +++ b/includes/page/Article.php @@ -2123,8 +2123,13 @@ class Article implements Page { * Call to WikiPage function for backwards compatibility. * @see WikiPage::doDeleteUpdates */ - public function doDeleteUpdates( $id, Content $content = null ) { - return $this->mPage->doDeleteUpdates( $id, $content ); + public function doDeleteUpdates( + $id, + Content $content = null, + $revision = null, + User $user = null + ) { + $this->mPage->doDeleteUpdates( $id, $content, $revision, $user ); } /** diff --git a/includes/page/WikiPage.php b/includes/page/WikiPage.php index bf21a56708..74e31791e9 100644 --- a/includes/page/WikiPage.php +++ b/includes/page/WikiPage.php @@ -29,6 +29,7 @@ use MediaWiki\Storage\PageUpdater; use MediaWiki\Storage\RevisionRecord; use MediaWiki\Storage\RevisionSlotsUpdate; use MediaWiki\Storage\RevisionStore; +use MediaWiki\Storage\SlotRecord; use Wikimedia\Assert\Assert; use Wikimedia\Rdbms\FakeResultWrapper; use Wikimedia\Rdbms\IDatabase; @@ -2820,15 +2821,21 @@ class WikiPage implements Page, IDBAccessObject { * Do some database updates after deletion * * @param int $id The page_id value of the page being deleted - * @param Content|null $content Optional page content to be used when determining + * @param Content|null $content Page content to be used when determining * the required updates. This may be needed because $this->getContent() * may already return null when the page proper was deleted. - * @param Revision|null $revision The latest page revision + * @param RevisionRecord|Revision|null $revision The current page revision at the time of + * deletion, used when determining the required updates. This may be needed because + * $this->getRevision() may already return null when the page proper was deleted. * @param User|null $user The user that caused the deletion */ public function doDeleteUpdates( $id, Content $content = null, Revision $revision = null, User $user = null ) { + if ( $id !== $this->getId() ) { + throw new InvalidArgumentException( 'Mismatching page ID' ); + } + try { $countable = $this->isCountable(); } catch ( Exception $ex ) { @@ -2843,7 +2850,9 @@ class WikiPage implements Page, IDBAccessObject { ) ); // Delete pagelinks, update secondary indexes, etc - $updates = $this->getDeletionUpdates( $content ); + $updates = $this->getDeletionUpdates( + $revision ? $revision->getRevisionRecord() : $content + ); foreach ( $updates as $update ) { DeferredUpdates::addUpdate( $update ); } @@ -3545,32 +3554,68 @@ class WikiPage implements Page, IDBAccessObject { * updates should remove any information about this page from secondary data * stores such as links tables. * - * @param Content|null $content Optional Content object for determining the - * necessary updates. + * @param RevisionRecord|Content|null $rev The revision being deleted. Also accepts a Content + * object for backwards compatibility. * @return DeferrableUpdate[] */ - public function getDeletionUpdates( Content $content = null ) { - if ( !$content ) { - // load content object, which may be used to determine the necessary updates. - // XXX: the content may not be needed to determine the updates. + public function getDeletionUpdates( $rev = null ) { + if ( !$rev ) { + wfDeprecated( __METHOD__ . ' without a RevisionRecord', '1.32' ); + try { - $content = $this->getContent( Revision::RAW ); + $rev = $this->getRevisionRecord(); } catch ( Exception $ex ) { // If we can't load the content, something is wrong. Perhaps that's why // the user is trying to delete the page, so let's not fail in that case. // Note that doDeleteArticleReal() will already have logged an issue with // loading the content. + wfDebug( __METHOD__ . ' failed to load current revision of page ' . $this->getId() ); } } - if ( !$content ) { - $updates = []; + if ( !$rev ) { + $slotContent = []; + } elseif ( $rev instanceof Content ) { + wfDeprecated( __METHOD__ . ' with a Content object instead of a RevisionRecord', '1.32' ); + + $slotContent = [ 'main' => $rev ]; } else { - $updates = $content->getDeletionUpdates( $this ); + $slotContent = array_map( function ( SlotRecord $slot ) { + return $slot->getContent( Revision::RAW ); + }, $rev->getSlots()->getSlots() ); } - Hooks::run( 'WikiPageDeletionUpdates', [ $this, $content, &$updates ] ); - return $updates; + $allUpdates = [ new LinksDeletionUpdate( $this ) ]; + + // NOTE: once Content::getDeletionUpdates() is removed, we only need to content + // model here, not the content object! + // TODO: consolidate with similar logic in DerivedPageDataUpdater::getSecondaryDataUpdates() + /** @var Content $content */ + foreach ( $slotContent as $role => $content ) { + $handler = $content->getContentHandler(); + + $updates = $handler->getDeletionUpdates( + $this->getTitle(), + $role + ); + $allUpdates = array_merge( $allUpdates, $updates ); + + // TODO: remove B/C hack in 1.32! + $legacyUpdates = $content->getDeletionUpdates( $this ); + + // HACK: filter out redundant and incomplete LinksDeletionUpdate + $legacyUpdates = array_filter( $legacyUpdates, function ( $update ) { + return !( $update instanceof LinksDeletionUpdate ); + } ); + + $allUpdates = array_merge( $allUpdates, $legacyUpdates ); + } + + Hooks::run( 'PageDeletionDataUpdates', [ $this->getTitle(), $rev, &$allUpdates ] ); + + // TODO: hard deprecate old hook in 1.33 + Hooks::run( 'WikiPageDeletionUpdates', [ $this, $content, &$allUpdates ] ); + return $allUpdates; } /** diff --git a/tests/phpunit/includes/Storage/DerivedPageDataUpdaterTest.php b/tests/phpunit/includes/Storage/DerivedPageDataUpdaterTest.php index 7c4be9771e..8b472d4836 100644 --- a/tests/phpunit/includes/Storage/DerivedPageDataUpdaterTest.php +++ b/tests/phpunit/includes/Storage/DerivedPageDataUpdaterTest.php @@ -4,6 +4,7 @@ namespace MediaWiki\Tests\Storage; use CommentStoreComment; use Content; +use ContentHandler; use LinksUpdate; use MediaWiki\MediaWikiServices; use MediaWiki\Storage\DerivedPageDataUpdater; @@ -13,6 +14,10 @@ use MediaWiki\Storage\RevisionRecord; use MediaWiki\Storage\RevisionSlotsUpdate; use MediaWiki\Storage\SlotRecord; use MediaWikiTestCase; +use MWCallableUpdate; +use PHPUnit\Framework\MockObject\MockObject; +use TextContent; +use TextContentHandler; use Title; use User; use Wikimedia\TestingAccessWrapper; @@ -500,7 +505,6 @@ class DerivedPageDataUpdaterTest extends MediaWikiTestCase { $dataUpdates = $updater->getSecondaryDataUpdates(); - // TODO: MCR: assert updates from all slots! $this->assertNotEmpty( $dataUpdates ); $linksUpdates = array_filter( $dataUpdates, function ( $du ) { @@ -509,6 +513,109 @@ class DerivedPageDataUpdaterTest extends MediaWikiTestCase { $this->assertCount( 1, $linksUpdates ); } + /** + * @param string $name + * + * @return ContentHandler + */ + private function defineMockContentModelForUpdateTesting( $name ) { + /** @var ContentHandler|MockObject $handler */ + $handler = $this->getMockBuilder( TextContentHandler::class ) + ->setConstructorArgs( [ $name ] ) + ->setMethods( + [ 'getSecondaryDataUpdates', 'getDeletionUpdates', 'unserializeContent' ] + ) + ->getMock(); + + $dataUpdate = new MWCallableUpdate( 'time' ); + $dataUpdate->_name = "$name data update"; + + $deletionUpdate = new MWCallableUpdate( 'time' ); + $deletionUpdate->_name = "$name deletion update"; + + $handler->method( 'getSecondaryDataUpdates' )->willReturn( [ $dataUpdate ] ); + $handler->method( 'getDeletionUpdates' )->willReturn( [ $deletionUpdate ] ); + $handler->method( 'unserializeContent' )->willReturnCallback( + function ( $text ) use ( $handler ) { + return $this->createMockContent( $handler, $text ); + } + ); + + $this->mergeMwGlobalArrayValue( + 'wgContentHandlers', [ + $name => function () use ( $handler ){ + return $handler; + } + ] + ); + + return $handler; + } + + /** + * @param ContentHandler $handler + * @param string $text + * + * @return Content + */ + private function createMockContent( ContentHandler $handler, $text ) { + /** @var Content|MockObject $content */ + $content = $this->getMockBuilder( TextContent::class ) + ->setConstructorArgs( [ $text ] ) + ->setMethods( [ 'getModel', 'getContentHandler' ] ) + ->getMock(); + + $content->method( 'getModel' )->willReturn( $handler->getModelID() ); + $content->method( 'getContentHandler' )->willReturn( $handler ); + + return $content; + } + + public function testGetSecondaryDataUpdatesWithSlotRemoval() { + global $wgMultiContentRevisionSchemaMigrationStage; + + if ( ! ( $wgMultiContentRevisionSchemaMigrationStage & SCHEMA_COMPAT_READ_NEW ) ) { + $this->markTestSkipped( 'Slot removal cannot happen with MCR being enabled' ); + } + + $m1 = $this->defineMockContentModelForUpdateTesting( 'M1' ); + $a1 = $this->defineMockContentModelForUpdateTesting( 'A1' ); + $m2 = $this->defineMockContentModelForUpdateTesting( 'M2' ); + + $mainContent1 = $this->createMockContent( $m1, 'main 1' ); + $auxContent1 = $this->createMockContent( $a1, 'aux 1' ); + $mainContent2 = $this->createMockContent( $m2, 'main 2' ); + + $user = $this->getTestUser()->getUser(); + $page = $this->getPage( __METHOD__ ); + $this->createRevision( + $page, + __METHOD__, + [ 'main' => $mainContent1, 'aux' => $auxContent1 ] + ); + + $update = new RevisionSlotsUpdate(); + $update->modifyContent( 'main', $mainContent2 ); + $update->removeSlot( 'aux' ); + + $page = $this->getPage( __METHOD__ ); + $updater = $this->getDerivedPageDataUpdater( $page ); + $updater->prepareContent( $user, $update, false ); + + $dataUpdates = $updater->getSecondaryDataUpdates(); + + $this->assertNotEmpty( $dataUpdates ); + + $updateNames = array_map( function ( $du ) { + return isset( $du->_name ) ? $du->_name : get_class( $du ); + }, $dataUpdates ); + + $this->assertContains( LinksUpdate::class, $updateNames ); + $this->assertContains( 'A1 deletion update', $updateNames ); + $this->assertContains( 'M2 data update', $updateNames ); + $this->assertNotContains( 'M1 data update', $updateNames ); + } + /** * Creates a dummy revision object without touching the database. * diff --git a/tests/phpunit/includes/content/WikitextContentHandlerTest.php b/tests/phpunit/includes/content/WikitextContentHandlerTest.php index b4b2948ba8..806038a06b 100644 --- a/tests/phpunit/includes/content/WikitextContentHandlerTest.php +++ b/tests/phpunit/includes/content/WikitextContentHandlerTest.php @@ -1,6 +1,7 @@ assertArrayHasKey( 'file_text', $data ); $this->assertEquals( 'This is file content', $data['file_text'] ); } + + public function testGetSecondaryDataUpdates() { + $title = Title::newFromText( 'Somefile.jpg', NS_FILE ); + $content = new WikitextContent( '' ); + + /** @var SlotRenderingProvider $srp */ + $srp = $this->getMock( SlotRenderingProvider::class ); + + $handler = new WikitextContentHandler(); + $updates = $handler->getSecondaryDataUpdates( $title, $content, 'main', $srp ); + + $this->assertEquals( [], $updates ); + } + + public function testGetDeletionUpdates() { + $title = Title::newFromText( 'Somefile.jpg', NS_FILE ); + $content = new WikitextContent( '' ); + + $srp = $this->getMock( SlotRenderingProvider::class ); + + $handler = new WikitextContentHandler(); + $updates = $handler->getDeletionUpdates( $title, 'main' ); + + $this->assertEquals( [], $updates ); + } + } diff --git a/tests/phpunit/includes/page/WikiPageDbTestBase.php b/tests/phpunit/includes/page/WikiPageDbTestBase.php index dc805dfeec..4cb2b47a6f 100644 --- a/tests/phpunit/includes/page/WikiPageDbTestBase.php +++ b/tests/phpunit/includes/page/WikiPageDbTestBase.php @@ -3,6 +3,7 @@ use MediaWiki\Edit\PreparedEdit; use MediaWiki\MediaWikiServices; use MediaWiki\Storage\RevisionSlotsUpdate; +use PHPUnit\Framework\MockObject\MockObject; use Wikimedia\TestingAccessWrapper; /** @@ -104,18 +105,35 @@ abstract class WikiPageDbTestBase extends MediaWikiLangTestCase { /** * @param string|Title|WikiPage $page - * @param string $text + * @param string|Content|Content[] $content * @param int|null $model * * @return WikiPage */ - protected function createPage( $page, $text, $model = null, $user = null ) { + protected function createPage( $page, $content, $model = null, $user = null ) { if ( is_string( $page ) || $page instanceof Title ) { $page = $this->newPage( $page, $model ); } - $content = ContentHandler::makeContent( $text, $page->getTitle(), $model ); - $page->doEditContent( $content, "testing", EDIT_NEW, false, $user ); + if ( !$user ) { + $user = $this->getTestUser()->getUser(); + } + + if ( is_string( $content ) ) { + $content = ContentHandler::makeContent( $content, $page->getTitle(), $model ); + } + + if ( !is_array( $content ) ) { + $content = [ 'main' => $content ]; + } + + $updater = $page->newPageUpdater( $user ); + + foreach ( $content as $role => $cnt ) { + $updater->setContent( $role, $cnt ); + } + + $updater->saveRevision( CommentStoreComment::newUnsavedComment( "testing" ) ); return $page; } @@ -589,16 +607,18 @@ abstract class WikiPageDbTestBase extends MediaWikiLangTestCase { * @covers WikiPage::doDeleteUpdates */ public function testDoDeleteUpdates() { + $user = $this->getTestUser()->getUser(); $page = $this->createPage( __METHOD__, "[[original text]] foo", CONTENT_MODEL_WIKITEXT ); $id = $page->getId(); + $page->loadPageData(); // make sure the current revision is cached. // Similar to MovePage logic wfGetDB( DB_MASTER )->delete( 'page', [ 'page_id' => $id ], __METHOD__ ); - $page->doDeleteUpdates( $id ); + $page->doDeleteUpdates( $page->getId(), $page->getContent(), $page->getRevision(), $user ); // Run the job queue JobQueueGroup::destroySingletons(); @@ -615,6 +635,86 @@ abstract class WikiPageDbTestBase extends MediaWikiLangTestCase { $this->assertEquals( 0, $n, 'pagelinks should contain no more links from the page' ); } + /** + * @param string $name + * + * @return ContentHandler + */ + protected function defineMockContentModelForUpdateTesting( $name ) { + /** @var ContentHandler|MockObject $handler */ + $handler = $this->getMockBuilder( TextContentHandler::class ) + ->setConstructorArgs( [ $name ] ) + ->setMethods( + [ 'getSecondaryDataUpdates', 'getDeletionUpdates', 'unserializeContent' ] + ) + ->getMock(); + + $dataUpdate = new MWCallableUpdate( 'time' ); + $dataUpdate->_name = "$name data update"; + + $deletionUpdate = new MWCallableUpdate( 'time' ); + $deletionUpdate->_name = "$name deletion update"; + + $handler->method( 'getSecondaryDataUpdates' )->willReturn( [ $dataUpdate ] ); + $handler->method( 'getDeletionUpdates' )->willReturn( [ $deletionUpdate ] ); + $handler->method( 'unserializeContent' )->willReturnCallback( + function ( $text ) use ( $handler ) { + return $this->createMockContent( $handler, $text ); + } + ); + + $this->mergeMwGlobalArrayValue( + 'wgContentHandlers', [ + $name => function () use ( $handler ){ + return $handler; + } + ] + ); + + return $handler; + } + + /** + * @param ContentHandler $handler + * @param string $text + * + * @return Content + */ + protected function createMockContent( ContentHandler $handler, $text ) { + /** @var Content|MockObject $content */ + $content = $this->getMockBuilder( TextContent::class ) + ->setConstructorArgs( [ $text ] ) + ->setMethods( [ 'getModel', 'getContentHandler' ] ) + ->getMock(); + + $content->method( 'getModel' )->willReturn( $handler->getModelID() ); + $content->method( 'getContentHandler' )->willReturn( $handler ); + + return $content; + } + + public function testGetDeletionUpdates() { + $m1 = $this->defineMockContentModelForUpdateTesting( 'M1' ); + + $mainContent1 = $this->createMockContent( $m1, 'main 1' ); + + $page = new WikiPage( Title::newFromText( __METHOD__ ) ); + $page = $this->createPage( + $page, + [ 'main' => $mainContent1 ] + ); + + $dataUpdates = $page->getDeletionUpdates( $page->getRevisionRecord() ); + $this->assertNotEmpty( $dataUpdates ); + + $updateNames = array_map( function ( $du ) { + return isset( $du->_name ) ? $du->_name : get_class( $du ); + }, $dataUpdates ); + + $this->assertContains( LinksDeletionUpdate::class, $updateNames ); + $this->assertContains( 'M1 deletion update', $updateNames ); + } + /** * @covers WikiPage::getRevision */ diff --git a/tests/phpunit/includes/page/WikiPageMcrReadNewDbTest.php b/tests/phpunit/includes/page/WikiPageMcrReadNewDbTest.php index 9e2ff09e6f..458e415dbc 100644 --- a/tests/phpunit/includes/page/WikiPageMcrReadNewDbTest.php +++ b/tests/phpunit/includes/page/WikiPageMcrReadNewDbTest.php @@ -20,4 +20,29 @@ class WikiPageMcrReadNewDbTest extends WikiPageDbTestBase { return true; } + public function testGetDeletionUpdates() { + $m1 = $this->defineMockContentModelForUpdateTesting( 'M1' ); + $a1 = $this->defineMockContentModelForUpdateTesting( 'A1' ); + + $mainContent1 = $this->createMockContent( $m1, 'main 1' ); + $auxContent1 = $this->createMockContent( $a1, 'aux 1' ); + + $page = new WikiPage( Title::newFromText( __METHOD__ ) ); + $page = $this->createPage( + $page, + [ 'main' => $mainContent1, 'aux' => $auxContent1 ] + ); + + $dataUpdates = $page->getDeletionUpdates( $page->getRevisionRecord() ); + $this->assertNotEmpty( $dataUpdates ); + + $updateNames = array_map( function ( $du ) { + return isset( $du->_name ) ? $du->_name : get_class( $du ); + }, $dataUpdates ); + + $this->assertContains( LinksDeletionUpdate::class, $updateNames ); + $this->assertContains( 'M1 deletion update', $updateNames ); + $this->assertContains( 'A1 deletion update', $updateNames ); + } + } diff --git a/tests/phpunit/includes/page/WikiPageNoContentModelDbTest.php b/tests/phpunit/includes/page/WikiPageNoContentModelDbTest.php index 7c9c657225..d849124926 100644 --- a/tests/phpunit/includes/page/WikiPageNoContentModelDbTest.php +++ b/tests/phpunit/includes/page/WikiPageNoContentModelDbTest.php @@ -20,4 +20,24 @@ class WikiPageNoContentModelDbTest extends WikiPageDbTestBase { return false; } + public function testGetDeletionUpdates() { + $mainContent1 = new WikitextContent( '' ); + + $title = Title::makeTitle( $this->getDefaultWikitextNS(), __METHOD__ ); + $page = new WikiPage( $title ); + $page = $this->createPage( + $page, + [ 'main' => $mainContent1 ] + ); + + $dataUpdates = $page->getDeletionUpdates( $page->getRevisionRecord() ); + $this->assertNotEmpty( $dataUpdates ); + + $updateNames = array_map( function ( $du ) { + return isset( $du->_name ) ? $du->_name : get_class( $du ); + }, $dataUpdates ); + + $this->assertContains( LinksDeletionUpdate::class, $updateNames ); + } + } -- 2.20.1