Changeset View
Standalone View
src/applications/differential/controller/DifferentialRevisionLandController.php
- This file was added.
<?php | |||||
final class DifferentialRevisionLandController extends DifferentialController { | |||||
private $revisionID; | |||||
private $strategyClass; | |||||
private $pushStrategy; | |||||
public function willProcessRequest(array $data) { | |||||
$this->revisionID = $data['id']; | |||||
$this->strategyClass = $data['strategy']; | |||||
} | |||||
epriestley: Double-quote? | |||||
public function processRequest() { | |||||
$request = $this->getRequest(); | |||||
$viewer = $request->getUser(); | |||||
Not Done Inline ActionsYou can use isFormPost() to verify the request is a POST with CSRF. To make it be POST, you can use workflow on the action link. The non-post case should pop a confirmation dialog ("Really land this?") for users with broken JS or right click -> open in new window. Roughly: if ($this->isFormPost()) { do_land(); return success_dialog(); } return confirm_dialog(); If you always want to show the confirm dialog, you can use isDialogFormPost() instead of isFormPost(). epriestley: You can use `isFormPost()` to verify the request is a POST with CSRF. To make it be POST, you… | |||||
Not Done Inline ActionsFor now I'd feel safer with having the confirmation dialog always show up, and maybe explain what it'd going to do (As in "This is going to squash this revision, rebase, and push it to master"). avivey: For now I'd feel safer with having the confirmation dialog always show up, and maybe explain… | |||||
$revision_id = $this->revisionID; | |||||
$revision = id(new DifferentialRevisionQuery()) | |||||
->withIDs(array($revision_id)) | |||||
->setViewer($viewer) | |||||
->executeOne(); | |||||
if (!$revision) { | |||||
return new Aphront404Response(); | |||||
} | |||||
if (is_subclass_of($this->strategyClass, 'DifferentialLandingStrategy')) { | |||||
$this->pushStrategy = newv($this->strategyClass, array()); | |||||
} else { | |||||
throw new Exception( | |||||
"Strategy type must be a valid class name and must subclass ". | |||||
"DifferentialLandingStrategy. ". | |||||
"'{$this->strategyClass}' is not a subclass of ". | |||||
Not Done Inline ActionsAll this return array(...) stuff should probably just let the exception escape, or wrap it in a ProxyException and rethrow. None of it ever returns more than one string. epriestley: All this `return array(...)` stuff should probably just let the exception escape, or wrap it in… | |||||
"DifferentialLandingStrategy."); | |||||
epriestleyUnsubmitted Not Done Inline Actions(This could probably just 404 if you want, I think it's unlikely anyone will ever hit this by accident.) epriestley: (This could probably just 404 if you want, I think it's unlikely anyone will ever hit this by… | |||||
} | |||||
if ($request->isDialogFormPost()) { | |||||
try { | |||||
$this->attemptLand($revision, $request); | |||||
$title = "Success!"; | |||||
$text = "Revision was successfully landed."; | |||||
epriestleyUnsubmitted Not Done Inline ActionsUI strings like this should be wrapped in pht(). epriestley: UI strings like this should be wrapped in `pht()`. | |||||
} catch (PhutilProxyException $ex) { | |||||
$title = "Failed to land revision"; | |||||
$text = hsprintf( | |||||
'%s:<br><pre>%s</pre>', | |||||
Not Done Inline Actionsbaaaa epriestley: baaaa | |||||
$ex->getMessage(), | |||||
$ex->getPreviousException()->getMessage()); | |||||
$text = id(new AphrontErrorView()) | |||||
->appendChild($text); | |||||
} catch (Exception $ex) { | |||||
$title = "Failed to land revision"; | |||||
$text = hsprintf('<pre>%s</pre>', $ex->getMessage()); | |||||
$text = id(new AphrontErrorView()) | |||||
->appendChild($text); | |||||
} | |||||
epriestleyUnsubmitted Not Done Inline ActionsI think if you just let this exception escape, we'll render something approximately as useful without needing any code. epriestley: I //think// if you just let this exception escape, we'll render something approximately as… | |||||
aviveyAuthorUnsubmitted Not Done Inline ActionsThe default handler doesn't special-case ProxyException, and it doesn't add the <pre>, which is important for git errors. I can reduce some code here though. avivey: The default handler doesn't special-case ProxyException, and it doesn't add the `<pre>`, which… | |||||
$dialog = id(new AphrontDialogView()) | |||||
->setUser($viewer) | |||||
->setTitle($title) | |||||
->appendChild(phutil_tag('p', array(), $text)) | |||||
->setSubmitURI('/D'.$revision_id) | |||||
->addSubmitButton('Back to revision'); | |||||
epriestleyUnsubmitted Not Done Inline ActionsMaybe just call this "Done"? It should also have pht(). epriestley: Maybe just call this `"Done"`? It should also have `pht()`. | |||||
return id(new AphrontDialogResponse())->setDialog($dialog); | |||||
} | |||||
$prompt = hsprintf( | |||||
'This will squash and rebase revision %s, and '. | |||||
'push it to origin/master. <br><br>'. | |||||
'It is an experimental feature and may not work.', | |||||
$revision_id); | |||||
Not Done Inline ActionsI expect these 3 calls will be abstracted to 3 "providers" or something, which the flow will have to pick and match based on repo type and features. I only avoided extracting interfaces/classes due to strict adherence to YAGNI, and I plan my own "push provider" for not-hosted git repos. Initial thoughts are to create /differential/landing dir which will hold all interfaces/implementations, and somehow config them in the repo control. avivey: I expect these 3 calls will be abstracted to 3 "providers" or something, which the flow will… | |||||
$dialog = id(new AphrontDialogView()) | |||||
->setUser($viewer) | |||||
->setTitle("Land Revision ${revision_id}?") | |||||
epriestleyUnsubmitted Not Done Inline ActionsFor consistency, prefer {$var} to ${var}. Both work because PHP is a magical language, but we use the former form (nearly?) exclusively. I should just add a lint rule for this, but the grammar is pretty messy and I just punted originally instead of actually writing it into the parser. epriestley: For consistency, prefer `{$var}` to `${var}`. Both work because PHP is a magical language, but… | |||||
->appendChild($prompt) | |||||
->setSubmitURI($request->getRequestURI()) | |||||
->addSubmitButton('Land it!') | |||||
->addCancelButton('/D'.$revision_id); | |||||
return id(new AphrontDialogResponse())->setDialog($dialog); | |||||
} | |||||
private function attemptLand($revision, $request) { | |||||
$status = $revision->getStatus(); | |||||
if ($status != ArcanistDifferentialRevisionStatus::ACCEPTED) { | |||||
throw new Exception("Only Accepted revisions can be landed."); | |||||
} | |||||
$repository = $revision->getRepository(); | |||||
if ($repository === null) { | |||||
throw new Exception("revision is not attached to a repository."); | |||||
} | |||||
$can_push = PhabricatorPolicyFilter::hasCapability( | |||||
$request->getUser(), | |||||
$repository, | |||||
DiffusionCapabilityPush::CAPABILITY); | |||||
if (!$can_push) { | |||||
throw new Exception( | |||||
pht('You do not have permission to push to this repository.')); | |||||
} | |||||
try { | |||||
$this->pushStrategy->assertSupportForRepository($repository); | |||||
} catch (Exception $e) { | |||||
throw new PhutilProxyException( | |||||
'This strategy does not support this repository', | |||||
$e); | |||||
} | |||||
$lock = $this->lockRepository($repository); | |||||
aviveyAuthorUnsubmitted Not Done Inline ActionsI'd moved the lock into the "main" controller, to minimize the risk of a strategy doing something wrong with the locks. It's also simpler here. avivey: I'd moved the lock into the "main" controller, to minimize the risk of a strategy doing… | |||||
try { | |||||
$this->pushStrategy->processLandRequest( | |||||
$request, | |||||
$revision, | |||||
$repository); | |||||
} catch (Exception $e) { | |||||
$lock->unlock(); | |||||
throw $e; | |||||
} | |||||
$lock->unlock(); | |||||
} | |||||
private static function lockRepository($repository) { | |||||
epriestleyUnsubmitted Not Done Inline ActionsThis is declared static, but called as an instance method on line 115. epriestley: This is declared `static`, but called as an instance method on line 115. | |||||
$lock_name = __CLASS__.':'.($repository->getCallsign()); | |||||
$lock = PhabricatorGlobalLock::newLock($lock_name); | |||||
$lock->lock(); | |||||
return $lock; | |||||
} | |||||
} | |||||
Double-quote?