Do not override content format in EditPage when loading rev.

getCurrentContent() previously would set $this->contentModel and
$this->contentFormat to the values from the current revision. I
do not believe this makes sense given how the method is called.

The method is used to load content to do a diff against in case of
the show diff button or edit conflict (and a couple other places).
In that case, one should clearly use the format the user is currently
editing in. Arguably if the content model is different the most
correct thing would be to convert the content model, except we already
error out in that case before reaching this point, so no point. The
only place where it could possibly make sense to override these variables
is in the getContentObject() method, however the majority of code paths
in that method do not alter $this->contentModel/format. Thus its more
consistent to not alter the contentModel/format state.

The previous code caused very confusing behaviour, where if you have a
content model that supports multiple formats, and the user selects a
non-default one (via &format=foo url parameter), everything would work
fine until hitting show diff.

Bug: T139249
Change-Id: I0b89e3d07290121b02eb6fc8483f68c2b44c878b

diff --git a/includes/EditPage.php b/includes/EditPage.php
index 9c7ccdf..ffd0b38 100644 (file)
--- a/includes/EditPage.php
+++ b/includes/EditPage.php
@@ -20,6 +20,8 @@
  * @file
  */
 
+use MediaWiki\Logger\LoggerFactory;
+
 /**
  * The edit page/HTML interface (split from Article)
  * The actual database and text munging is still in Article,
@@ -1249,9 +1251,31 @@ class EditPage {
 
                        return $handler->makeEmptyContent();
                } else {
-                       # nasty side-effect, but needed for consistency
-                       $this->contentModel = $rev->getContentModel();
-                       $this->contentFormat = $rev->getContentFormat();
+                       // Content models should always be the same since we error
+                       // out if they are different before this point.
+                       $logger = LoggerFactory::getInstance( 'editpage' );
+                       if ( $this->contentModel !== $rev->getContentModel() ) {
+                               $logger->warning( "Overriding content model from current edit {prev} to {new}", [
+                                       'prev' => $this->contentModel,
+                                       'new' => $rev->getContentModel(),
+                                       'title' => $this->getTitle()->getPrefixedDBkey(),
+                                       'method' => __METHOD__
+                               ] );
+                               $this->contentModel = $rev->getContentModel();
+                       }
+
+                       // Given that the content models should match, the current selected
+                       // format should be supported.
+                       if ( !$content->isSupportedFormat( $this->contentFormat ) ) {
+                               $logger->warning( "Current revision content format unsupported. Overriding {prev} to {new}", [
+
+                                       'prev' => $this->contentFormat,
+                                       'new' => $rev->getContentFormat(),
+                                       'title' => $this->getTitle()->getPrefixedDBkey(),
+                                       'method' => __METHOD__
+                               ] );
+                               $this->contentFormat = $rev->getContentFormat();
+                       }
 
                        return $content;
                }