diff --git a/src/applications/phriction/application/PhabricatorPhrictionApplication.php b/src/applications/phriction/application/PhabricatorPhrictionApplication.php index b2cc472341..d8a7923b04 100644 --- a/src/applications/phriction/application/PhabricatorPhrictionApplication.php +++ b/src/applications/phriction/application/PhabricatorPhrictionApplication.php @@ -52,7 +52,7 @@ final class PhabricatorPhrictionApplication extends PhabricatorApplication { 'edit/(?:(?P[1-9]\d*)/)?' => 'PhrictionEditController', 'delete/(?P[1-9]\d*)/' => 'PhrictionDeleteController', 'new/' => 'PhrictionNewController', - 'move/(?:(?P[1-9]\d*)/)?' => 'PhrictionMoveController', + 'move/(?P[1-9]\d*)/' => 'PhrictionMoveController', 'preview/' => 'PhabricatorMarkupPreviewController', 'diff/(?P[1-9]\d*)/' => 'PhrictionDiffController', diff --git a/src/applications/phriction/controller/PhrictionMoveController.php b/src/applications/phriction/controller/PhrictionMoveController.php index ce4dbe240c..21c65f0495 100644 --- a/src/applications/phriction/controller/PhrictionMoveController.php +++ b/src/applications/phriction/controller/PhrictionMoveController.php @@ -2,79 +2,72 @@ final class PhrictionMoveController extends PhrictionController { - private $id; - - public function willProcessRequest(array $data) { - $this->id = idx($data, 'id'); - } - - public function processRequest() { - $request = $this->getRequest(); - $user = $request->getUser(); - - if ($this->id) { - $document = id(new PhrictionDocumentQuery()) - ->setViewer($user) - ->withIDs(array($this->id)) - ->needContent(true) - ->requireCapabilities( - array( - PhabricatorPolicyCapability::CAN_VIEW, - PhabricatorPolicyCapability::CAN_EDIT, - )) - ->executeOne(); - } else { - $slug = PhabricatorSlug::normalize( - $request->getStr('slug')); - if (!$slug) { - return new Aphront404Response(); - } - - $document = id(new PhrictionDocumentQuery()) - ->setViewer($user) - ->withSlugs(array($slug)) - ->needContent(true) - ->requireCapabilities( - array( - PhabricatorPolicyCapability::CAN_VIEW, - PhabricatorPolicyCapability::CAN_EDIT, - )) - ->executeOne(); - } + public function handleRequest(AphrontRequest $request) { + $viewer = $this->getViewer(); + $document = id(new PhrictionDocumentQuery()) + ->setViewer($viewer) + ->withIDs(array($request->getURIData('id'))) + ->needContent(true) + ->requireCapabilities( + array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + )) + ->executeOne(); if (!$document) { return new Aphront404Response(); } - if (!isset($slug)) { - $slug = $document->getSlug(); - } - - $target_slug = PhabricatorSlug::normalize( - $request->getStr('new-slug', $slug)); - - $submit_uri = $request->getRequestURI()->getPath(); + $slug = $document->getSlug(); $cancel_uri = PhrictionDocument::getSlugURI($slug); - $e_url = true; - $validation_exception = null; - $content = $document->getContent(); + $v_slug = $slug; + $e_slug = null; + $v_note = ''; + + $validation_exception = null; if ($request->isFormPost()) { + $v_note = $request->getStr('description'); + $v_slug = $request->getStr('slug'); + + // If what the user typed isn't what we're actually using, warn them + // about it. + if (strlen($v_slug)) { + $normal_slug = PhabricatorSlug::normalize($v_slug); + if ($normal_slug !== $v_slug) { + return $this->newDialog() + ->setTitle(pht('Adjust Path')) + ->appendParagraph( + pht( + 'The path you entered (%s) is not a valid wiki document '. + 'path. Paths may not contain special characters.', + phutil_tag('strong', array(), $v_slug))) + ->appendParagraph( + pht( + 'Would you like to use the path %s instead?', + phutil_tag('strong', array(), $normal_slug))) + ->addHiddenInput('slug', $normal_slug) + ->addHiddenInput('description', $v_note) + ->addCancelButton($cancel_uri) + ->addSubmitButton(pht('Accept Path')); + } + } $editor = id(new PhrictionTransactionEditor()) - ->setActor($user) + ->setActor($viewer) ->setContentSourceFromRequest($request) ->setContinueOnNoEffect(true) - ->setDescription($request->getStr('description')); + ->setDescription($v_note); $xactions = array(); $xactions[] = id(new PhrictionTransaction()) ->setTransactionType(PhrictionTransaction::TYPE_MOVE_TO) ->setNewValue($document); $target_document = PhrictionDocument::initializeNewDocument( - $user, - $target_slug); + $viewer, + $v_slug); try { $editor->applyTransactions($target_document, $xactions); $redir_uri = PhrictionDocument::getSlugURI( @@ -82,40 +75,40 @@ final class PhrictionMoveController extends PhrictionController { return id(new AphrontRedirectResponse())->setURI($redir_uri); } catch (PhabricatorApplicationTransactionValidationException $ex) { $validation_exception = $ex; - $e_url = $ex->getShortMessage(PhrictionTransaction::TYPE_MOVE_TO); + $e_slug = $ex->getShortMessage(PhrictionTransaction::TYPE_MOVE_TO); } } - $form = id(new PHUIFormLayoutView()) - ->setUser($user) + + $form = id(new AphrontFormView()) + ->setUser($viewer) ->appendChild( id(new AphrontFormStaticControl()) - ->setLabel(pht('Title')) - ->setValue($content->getTitle())) + ->setLabel(pht('Title')) + ->setValue($document->getContent()->getTitle())) ->appendChild( id(new AphrontFormTextControl()) - ->setLabel(pht('New URI')) - ->setValue($target_slug) - ->setError($e_url) - ->setName('new-slug') - ->setCaption(pht('The new location of the document.'))) + ->setLabel(pht('Current Path')) + ->setDisabled(true) + ->setValue($slug)) ->appendChild( id(new AphrontFormTextControl()) - ->setLabel(pht('Edit Notes')) - ->setValue(pht('Moving document to a new location.')) - ->setError(null) - ->setName('description')); + ->setLabel(pht('New Path')) + ->setValue($v_slug) + ->setError($e_slug) + ->setName('slug')) + ->appendChild( + id(new AphrontFormTextControl()) + ->setLabel(pht('Edit Notes')) + ->setValue($v_note) + ->setName('description')); - $dialog = id(new AphrontDialogView()) - ->setUser($user) - ->setValidationException($validation_exception) + return $this->newDialog() ->setTitle(pht('Move Document')) - ->appendChild($form) - ->setSubmitURI($submit_uri) + ->setValidationException($validation_exception) + ->appendForm($form) ->addSubmitButton(pht('Move Document')) ->addCancelButton($cancel_uri); - - return id(new AphrontDialogResponse())->setDialog($dialog); } } diff --git a/src/applications/phriction/editor/PhrictionTransactionEditor.php b/src/applications/phriction/editor/PhrictionTransactionEditor.php index eb85ef0169..d8c8bc1730 100644 --- a/src/applications/phriction/editor/PhrictionTransactionEditor.php +++ b/src/applications/phriction/editor/PhrictionTransactionEditor.php @@ -537,13 +537,24 @@ final class PhrictionTransactionEditor ->needContent(true) ->executeOne(); - // Considering to overwrite existing docs? Nuke this! + // Prevent overwrites and no-op moves. $exists = PhrictionDocumentStatus::STATUS_EXISTS; - if ($target_document && $target_document->getStatus() == $exists) { + if ($target_document) { + if ($target_document->getSlug() == $source_document->getSlug()) { + $message = pht( + 'You can not move a document to its existing location. '. + 'Choose a different location to move the document to.'); + } else if ($target_document->getStatus() == $exists) { + $message = pht( + 'You can not move this document there, because it would '. + 'overwrite an existing document which is already at that '. + 'location. Move or delete the existing document first.'); + } + $error = new PhabricatorApplicationTransactionValidationError( $type, - pht('Can not move document.'), - pht('Can not overwrite existing target document.'), + pht('Invalid'), + $message, $xaction); $errors[] = $error; } diff --git a/src/infrastructure/util/PhabricatorSlug.php b/src/infrastructure/util/PhabricatorSlug.php index 17e961ddbd..4e6d761dc2 100644 --- a/src/infrastructure/util/PhabricatorSlug.php +++ b/src/infrastructure/util/PhabricatorSlug.php @@ -8,7 +8,17 @@ final class PhabricatorSlug { $slug = phutil_utf8_strtolower($slug); $slug = preg_replace("@[\\x00-\\x19#%&+=\\\\?<> ]+@", '_', $slug); $slug = preg_replace('@_+@', '_', $slug); - $slug = trim($slug, '_'); + + // Remove leading and trailing underscores from each component, if the + // component has not been reduced to a single underscore. For example, "a?" + // converts to "a", but "??" converts to "_". + $parts = explode('/', $slug); + foreach ($parts as $key => $part) { + if ($part != '_') { + $parts[$key] = trim($part, '_'); + } + } + $slug = implode('/', $parts); // Specifically rewrite these slugs. It's OK to have a slug like "a..b", // but not a slug which is only "..". diff --git a/src/infrastructure/util/__tests__/PhabricatorSlugTestCase.php b/src/infrastructure/util/__tests__/PhabricatorSlugTestCase.php index faefb51382..3c820a085e 100644 --- a/src/infrastructure/util/__tests__/PhabricatorSlugTestCase.php +++ b/src/infrastructure/util/__tests__/PhabricatorSlugTestCase.php @@ -7,7 +7,7 @@ final class PhabricatorSlugTestCase extends PhabricatorTestCase { '' => '/', '/' => '/', '//' => '/', - '&&&' => '/', + '&&&' => '_/', '/derp/' => 'derp/', 'derp' => 'derp/', 'derp//derp' => 'derp/derp/', @@ -27,6 +27,13 @@ final class PhabricatorSlugTestCase extends PhabricatorTestCase { '../a' => 'dotdot/a/', 'a/..' => 'a/dotdot/', 'a/../' => 'a/dotdot/', + 'a?' => 'a/', + '??' => '_/', + 'a/?' => 'a/_/', + '??/a/??' => '_/a/_/', + 'a/??/c' => 'a/_/c/', + 'a/?b/c' => 'a/b/c/', + 'a/b?/c' => 'a/b/c/', ); foreach ($slugs as $slug => $normal) {