jenkins-bot has submitted this change and it was merged. ( 
https://gerrit.wikimedia.org/r/367045 )

Change subject: Streamline input validation for Special pages.
......................................................................


Streamline input validation for Special pages.

Change-Id: I0886cfe8ad5b490e2362175452459a1635160e70
---
M repo/includes/Specials/SpecialModifyEntity.php
M repo/includes/Specials/SpecialModifyTerm.php
M repo/includes/Specials/SpecialSetLabelDescriptionAliases.php
M repo/includes/Specials/SpecialSetSiteLink.php
M repo/tests/phpunit/includes/Specials/SpecialModifyTermTestCase.php
5 files changed, 112 insertions(+), 60 deletions(-)

Approvals:
  WMDE-leszek: Looks good to me, approved
  jenkins-bot: Verified



diff --git a/repo/includes/Specials/SpecialModifyEntity.php 
b/repo/includes/Specials/SpecialModifyEntity.php
index 00a30f6..c1131f4 100644
--- a/repo/includes/Specials/SpecialModifyEntity.php
+++ b/repo/includes/Specials/SpecialModifyEntity.php
@@ -5,6 +5,7 @@
 use HTMLForm;
 use Html;
 use MWException;
+use Status;
 use Wikibase\DataModel\Entity\EntityId;
 use Wikibase\Repo\ChangeOp\ChangeOp;
 use Wikibase\Repo\ChangeOp\ChangeOpException;
@@ -32,7 +33,7 @@
        /**
         * @var EntityDocument|null
         */
-       private $entity = null;
+       private $entityForModification = null;
 
        /**
         * @var EntityId|null
@@ -163,12 +164,30 @@
         * @return EntityDocument
         */
        protected function getEntityForModification() {
-               if ( !$this->entity ) {
+               if ( !$this->entityForModification ) {
                        $revision = $this->getBaseRevision();
-                       $this->entity = $revision->getEntity()->copy();
+                       $this->entityForModification = 
$revision->getEntity()->copy();
                }
 
-               return $this->entity;
+               return $this->entityForModification;
+       }
+
+       /**
+        * Returns the EntityDocument that is to be shown by code in this class 
(or subclasses).
+        * The returns null if no entity ID was specified in the request.
+        *
+        * @throws MessageException
+        * @throws UserInputException
+        *
+        * @return EntityDocument|null
+        */
+       protected function getEntityForDisplay() {
+               if ( $this->entityId ) {
+                       $revision = $this->getBaseRevision();
+                       return $revision->getEntity();
+               }
+
+               return null;
        }
 
        /**
@@ -186,36 +205,41 @@
                $this->setHeaders();
                $this->outputHeader();
 
-               $summary = false;
-               $entity = null;
-
                try {
-                       $this->prepareArguments( $subPage );
-
+                       $this->processArguments( $subPage );
                        $valid = $this->validateInput();
-                       $entity = $this->getEntityId() ? 
$this->getEntityForModification() : null;
+                       $status = null;
 
-                       if ( $valid && $entity ) {
-                               $summary = $this->modifyEntity( $entity );
+                       if ( $valid && $this->isModificationRequested() ) {
+                               $updatedEntity = 
$this->getEntityForModification();
+                               $summary = $this->modifyEntity( $updatedEntity 
);
+
+                               if ( $summary ) {
+                                       $token = $this->getRequest()->getVal( 
'wpEditToken' );
+                                       $status = $this->saveEntity( 
$updatedEntity, $summary, $token );
+
+                                       $this->handleStatus( $status, 
$updatedEntity );
+                               }
+                       }
+
+                       if ( !$status || !$status->isOK() ) {
+                               $entity = $this->getEntityForDisplay();
+                               $this->setForm( $entity );
                        }
                } catch ( UserInputException $ex ) {
                        $error = $this->msg( $ex->getKey(), $ex->getParams() 
)->parse();
                        $this->showErrorHTML( $error );
                }
+       }
 
-               if ( !$entity || !$summary ) {
-                       $this->setForm( $entity );
+       private function handleStatus( Status $status, EntityDocument $entity ) 
{
+               if ( $status->isOK() ) {
+                       $entityUrl = $this->getEntityTitle( 
$this->getEntityId() )->getFullURL();
+                       $this->getOutput()->redirect( $entityUrl );
                } else {
-                       $status = $this->saveEntity( $entity, $summary, 
$this->getRequest()->getVal( 'wpEditToken' ) );
-
-                       if ( !$status->isOK() && $status->getErrorsArray() ) {
-                               $errors = $status->getErrorsArray();
-                               $this->showErrorHTML( $this->msg( 
$errors[0][0], array_slice( $errors[0], 1 ) )->parse() );
-                               $this->setForm( $entity );
-                       } else {
-                               $entityUrl = $this->getEntityTitle( 
$entity->getId() )->getFullURL();
-                               $this->getOutput()->redirect( $entityUrl );
-                       }
+                       $errors = $status->getErrorsArray();
+                       $this->showErrorHTML( $this->msg( $errors[0][0], 
array_slice( $errors[0], 1 ) )->parse() );
+                       $this->setForm( $entity );
                }
        }
 
@@ -224,7 +248,7 @@
         *
         * @param string|null $subPage
         */
-       protected function prepareArguments( $subPage ) {
+       protected function processArguments( $subPage ) {
                $parts = $subPage === '' ? [] : explode( '/', $subPage, 2 );
 
                $idString = $this->getRequest()->getVal( 'id', isset( $parts[0] 
) ? $parts[0] : null );
@@ -305,14 +329,40 @@
        /**
         * Validates form input.
         *
-        * The default implementation just checks whether a target entity was 
specified via a POST request.
+        * The default implementation does nothing.
         * Subclasses should override this to detect otherwise incomplete or 
erroneous input.
         *
-        * @return bool true if the form input is ok and normal processing 
should
-        * continue by calling modifyEntity().
+        * If this method returns false, the entity should not be updated and 
the user should be
+        * presented with an input form. Only if it returns true, and 
isModificationRequested() also
+        * returns true, the entity should be updated in the storage backend.
+        *
+        * @throws UserInputException if any of the provided input is invalid. 
If the input is
+        *         merely incomplete, no exception should be raised.
+        *
+        * @return bool true if all input needed for modification has been 
supplied.
+        *         false if the input is valid but incomplete, or if the input 
is invalid and
+        *         showErrorHTML() has already been called to notify the user 
of the problem.
+        *         The preferred way of indicating invalid input is however to 
throw a
+        *         UserInputException.
         */
        protected function validateInput() {
-               return $this->getEntityId() !== null && 
$this->getRequest()->wasPosted();
+               return $this->getEntityId() !== null;
+       }
+
+       /**
+        * Whether the current request is a request for modification (as 
opposed to a
+        * request for showing the input form).
+        *
+        * If this method returns false, the entity should not be updated and 
the user should be
+        * presented with an input form. Only if it returns true, and 
validateInput() also
+        * returns true, the entity should be updated in the storage backend.
+        *
+        * Undefined before processArguments() was called.
+        *
+        * @return bool
+        */
+       protected function isModificationRequested() {
+               return $this->getRequest()->wasPosted();
        }
 
        /**
diff --git a/repo/includes/Specials/SpecialModifyTerm.php 
b/repo/includes/Specials/SpecialModifyTerm.php
index 7229b44..68d5561 100644
--- a/repo/includes/Specials/SpecialModifyTerm.php
+++ b/repo/includes/Specials/SpecialModifyTerm.php
@@ -7,6 +7,7 @@
 use Language;
 use Status;
 use Wikibase\DataModel\Entity\EntityId;
+use Wikibase\Lib\UserInputException;
 use Wikibase\Repo\ChangeOp\ChangeOpException;
 use Wikibase\Repo\ChangeOp\FingerprintChangeOpFactory;
 use Wikibase\DataModel\Entity\EntityDocument;
@@ -92,12 +93,12 @@
        }
 
        /**
-        * @see SpecialModifyEntity::prepareArguments()
+        * @see SpecialModifyEntity::processArguments()
         *
         * @param string|null $subPage
         */
-       protected function prepareArguments( $subPage ) {
-               parent::prepareArguments( $subPage );
+       protected function processArguments( $subPage ) {
+               parent::processArguments( $subPage );
 
                $request = $this->getRequest();
                $parts = ( $subPage === '' ) ? [] : explode( '/', $subPage, 2 );
@@ -115,6 +116,13 @@
                $this->value = $this->getPostedValue();
                if ( $this->value === null ) {
                        $this->value = $request->getVal( 'value' );
+               }
+
+               // If the user just enters an item id and a language, dont 
remove the term.
+               // The user can remove the term in the second form where it has 
to be
+               // actually removed. This prevents users from removing terms 
accidentally.
+               if ( !$request->getCheck( 'remove' ) && $this->value === '' ) {
+                       $this->value = null;
                }
        }
 
@@ -138,14 +146,15 @@
         * @return bool
         */
        protected function validateInput() {
-               $request = $this->getRequest();
-
                if ( !parent::validateInput() ) {
                        return false;
                }
 
-               $entityId = $this->getEntityId();
+               if ( $this->value === null ) {
+                       return false;
+               }
 
+               $entityId = $this->getEntityId();
                if ( $entityId ) {
                        $status = $this->checkTermChangePermissions( $entityId 
);
 
@@ -153,14 +162,6 @@
                                $this->showErrorHTML( $this->msg( 
'permissionserrors' ) );
                                return false;
                        }
-               }
-
-               // If the user just enters an item id and a language, dont 
remove the term.
-               // The user can remove the term in the second form where it has 
to be
-               // actually removed. This prevents users from removing terms 
accidentally.
-               if ( !$request->getCheck( 'remove' ) && $this->value === '' ) {
-                       $this->value = null;
-                       return false;
                }
 
                return true;
diff --git a/repo/includes/Specials/SpecialSetLabelDescriptionAliases.php 
b/repo/includes/Specials/SpecialSetLabelDescriptionAliases.php
index 12d4c67..c07f485 100644
--- a/repo/includes/Specials/SpecialSetLabelDescriptionAliases.php
+++ b/repo/includes/Specials/SpecialSetLabelDescriptionAliases.php
@@ -221,15 +221,15 @@
        }
 
        /**
-        * @see SpecialModifyEntity::prepareArguments
+        * @see SpecialModifyEntity::processArguments
         *
         * @param string|null $subPage
         */
-       protected function prepareArguments( $subPage ) {
+       protected function processArguments( $subPage ) {
                $this->extractInput( $subPage );
 
                // Parse the 'id' parameter and throw an exception if the 
entity cannot be loaded
-               parent::prepareArguments( $subPage );
+               parent::processArguments( $subPage );
 
                if ( $this->languageCode === '' ) {
                        $this->languageCode = $this->getLanguage()->getCode();
@@ -241,8 +241,8 @@
                        $this->languageCode = null;
                }
 
-               if ( $this->languageCode !== null && $this->getEntityId() ) {
-                       $entity = $this->getEntityForModification();
+               $entity = $this->getEntityForDisplay();
+               if ( $this->languageCode !== null && $entity ) {
                        if ( $entity instanceof FingerprintProvider ) {
                                $this->setFingerprintFields( 
$entity->getFingerprint() );
                        }
diff --git a/repo/includes/Specials/SpecialSetSiteLink.php 
b/repo/includes/Specials/SpecialSetSiteLink.php
index facb2a6..ddf3f81 100644
--- a/repo/includes/Specials/SpecialSetSiteLink.php
+++ b/repo/includes/Specials/SpecialSetSiteLink.php
@@ -125,12 +125,12 @@
        }
 
        /**
-        * @see SpecialModifyEntity::prepareArguments()
+        * @see SpecialModifyEntity::processArguments()
         *
         * @param string|null $subPage
         */
-       protected function prepareArguments( $subPage ) {
-               parent::prepareArguments( $subPage );
+       protected function processArguments( $subPage ) {
+               parent::processArguments( $subPage );
 
                $request = $this->getRequest();
                // explode the sub page from the format 
Special:SetSitelink/q123/enwiki
@@ -161,6 +161,13 @@
 
                $this->page = $request->getVal( 'page' );
 
+               // If the user just enters an item id and a site, dont remove 
the site link.
+               // The user can remove the site link in the second form where 
it has to be
+               // actually removed. This prevents users from removing site 
links accidentally.
+               if ( !$request->getCheck( 'remove' ) && $this->page === '' ) {
+                       $this->page = null;
+               }
+
                $this->badges = array_intersect(
                        array_keys( $this->badgeItems ),
                        $request->getArray( 'badges', [] )
@@ -173,21 +180,15 @@
         * @return bool
         */
        protected function validateInput() {
-               $request = $this->getRequest();
-
                if ( !$this->isValidSiteId( $this->site ) ) {
                        return false;
                }
 
-               if ( !parent::validateInput() ) {
+               if ( $this->page === null ) {
                        return false;
                }
 
-               // If the user just enters an item id and a site, dont remove 
the site link.
-               // The user can remove the site link in the second form where 
it has to be
-               // actually removed. This prevents users from removing site 
links accidentally.
-               if ( !$request->getCheck( 'remove' ) && $this->page === '' ) {
-                       $this->page = null;
+               if ( !parent::validateInput() ) {
                        return false;
                }
 
diff --git a/repo/tests/phpunit/includes/Specials/SpecialModifyTermTestCase.php 
b/repo/tests/phpunit/includes/Specials/SpecialModifyTermTestCase.php
index fe2b279..c4941ff 100644
--- a/repo/tests/phpunit/includes/Specials/SpecialModifyTermTestCase.php
+++ b/repo/tests/phpunit/includes/Specials/SpecialModifyTermTestCase.php
@@ -130,7 +130,7 @@
 
                $id = $this->createNewItemWithTerms( $language = 'de', 
$termValue = 'foo' );
 
-               $request = new FauxRequest( [ 'id' => $id, 'language' => 
$language, 'value' => '' ], true );
+               $request = new FauxRequest( [ 'id' => $id, 'language' => 
$language, 'value' => 'test' ], true );
 
                list( $output, ) = $this->executeSpecialPage( '', $request, 
self::USER_LANGUAGE );
 

-- 
To view, visit https://gerrit.wikimedia.org/r/367045
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I0886cfe8ad5b490e2362175452459a1635160e70
Gerrit-PatchSet: 10
Gerrit-Project: mediawiki/extensions/Wikibase
Gerrit-Branch: master
Gerrit-Owner: Daniel Kinzler <daniel.kinz...@wikimedia.de>
Gerrit-Reviewer: Addshore <addshorew...@gmail.com>
Gerrit-Reviewer: WMDE-leszek <leszek.mani...@wikimedia.de>
Gerrit-Reviewer: jenkins-bot <>

_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits

Reply via email to