Merge "ApiQueryInfo: fix query limits for testactions"
authorjenkins-bot <jenkins-bot@gerrit.wikimedia.org>
Mon, 17 Sep 2018 14:06:26 +0000 (14:06 +0000)
committerGerrit Code Review <gerrit@wikimedia.org>
Mon, 17 Sep 2018 14:06:26 +0000 (14:06 +0000)
15 files changed:
RELEASE-NOTES-1.32
autoload.php
docs/hooks.txt
includes/Revision/RenderedRevision.php
includes/Revision/SlotRenderingProvider.php [new file with mode: 0644]
includes/Storage/DerivedPageDataUpdater.php
includes/content/Content.php
includes/content/ContentHandler.php
includes/page/Article.php
includes/page/WikiPage.php
tests/phpunit/includes/Storage/DerivedPageDataUpdaterTest.php
tests/phpunit/includes/content/WikitextContentHandlerTest.php
tests/phpunit/includes/page/WikiPageDbTestBase.php
tests/phpunit/includes/page/WikiPageMcrReadNewDbTest.php
tests/phpunit/includes/page/WikiPageNoContentModelDbTest.php

index afafc23..5954b87 100644 (file)
@@ -433,6 +433,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
index b52dfb7..67285d0 100644 (file)
@@ -897,6 +897,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',
index 9a53ccf..8223814 100644 (file)
@@ -2546,6 +2546,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
@@ -2919,6 +2925,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.
 
@@ -2978,9 +2991,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
@@ -4038,10 +4051,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.
index ba40a81..fa16c61 100644 (file)
@@ -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 (file)
index 0000000..740f0f2
--- /dev/null
@@ -0,0 +1,32 @@
+<?php
+/**
+ * Created by PhpStorm.
+ * User: daki
+ * Date: 05.09.18
+ * Time: 16:08
+ */
+namespace MediaWiki\Revision;
+
+use MediaWiki\Storage\SuppressedDataException;
+use ParserOutput;
+
+/**
+ * A lazy provider of ParserOutput objects for a revision's individual slots.
+ *
+ * @since 1.32
+ */
+interface SlotRenderingProvider {
+
+       /**
+        * @param string $role
+        * @param array $hints Hints given as an associative array. Known keys:
+        *      - 'generate-html' => 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 = [] );
+
+}
index b44e0e9..e34e406 100644 (file)
@@ -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 );
index 000bff2..bb3fb10 100644 (file)
@@ -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.
index 7a378b3..fab043a 100644 (file)
@@ -28,6 +28,7 @@
 use Wikimedia\Assert\Assert;
 use MediaWiki\Logger\LoggerFactory;
 use MediaWiki\MediaWikiServices;
+use MediaWiki\Revision\SlotRenderingProvider;
 use MediaWiki\Search\ParserOutputSearchDataExtractor;
 
 /**
@@ -1410,4 +1411,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 [];
+       }
+
 }
index 128cefe..9d651f4 100644 (file)
@@ -2305,8 +2305,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 );
        }
 
        /**
index bf21a56..74e3179 100644 (file)
@@ -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;
        }
 
        /**
index 7c4be97..8b472d4 100644 (file)
@@ -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.
         *
index b4b2948..806038a 100644 (file)
@@ -1,6 +1,7 @@
 <?php
 
 use MediaWiki\MediaWikiServices;
+use MediaWiki\Revision\SlotRenderingProvider;
 
 /**
  * @group ContentHandler
@@ -363,4 +364,30 @@ class WikitextContentHandlerTest extends MediaWikiLangTestCase {
                $this->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 );
+       }
+
 }
index dc805df..4cb2b47 100644 (file)
@@ -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
         */
index 9e2ff09..458e415 100644 (file)
@@ -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 );
+       }
+
 }
index 7c9c657..d849124 100644 (file)
@@ -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 );
+       }
+
 }