Differential D7486 Diff 16872 src/applications/differential/controller/DifferentialRevisionLandController.php

Changeset View

Standalone View

src/applications/differential/controller/DifferentialRevisionLandController.php

This file was added.

				<?php

				final class DifferentialRevisionLandController extends DifferentialController {

				public function processRequest() {
				$request = $this->getRequest();
				$viewer = $request->getUser();

				$revision_id = $request->getInt('revisionID');

				if ($revision_id == null) {
				throw new Exception('"');
				epriestleyUnsubmitted Not Done Inline Actions Double-quote? epriestley: Double-quote?
				}

				list($error, $details) = $this->attemptLand($revision_id, $viewer);
				epriestleyUnsubmitted Not Done Inline Actions You 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…
				aviveyAuthorUnsubmitted Not Done Inline Actions For 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…

				if ($error === null) {
				$title = "Success!";
				$text = "Revision was successfully landed.";
				} else {
				$title = "Failed to land revision";
				$text = hsprintf('%s<br>Details:<br><pre>%s</pre>', $error, $details);
				}

				$dialog = id(new AphrontDialogView())
				->setUser($viewer)
				->setTitle($title)
				->appendChild(phutil_tag('p', array(), $text));

				return id(new AphrontDialogResponse())->setDialog($dialog);
				}

				private function attemptLand($revision_id, $viewer) {
				epriestleyUnsubmitted Not Done Inline Actions All 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…
				$revision = id(new DifferentialRevisionQuery())
				->withIDs(array($revision_id))
				->setViewer($viewer)
				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…
				->executeOne();
				if (!$revision) {
				return array("revision $revision_id not found");
				}

				$status = $revision->getStatus();
				if ($status != ArcanistDifferentialRevisionStatus::ACCEPTED) {
				epriestleyUnsubmitted Not Done Inline Actions UI strings like this should be wrapped in `pht()`. epriestley: UI strings like this should be wrapped in `pht()`.
				return array("Can only land Accepted revisions.");
				}

				epriestleyUnsubmitted Not Done Inline Actions baaaa epriestley: baaaa
				$repository = $revision->getRepository();

				if ($repository === null) {
				return array("revision is not attached to a repository.");
				}

				$can_push = PhabricatorPolicyFilter::hasCapability(
				$viewer,
				$repository,
				DiffusionCapabilityPush::CAPABILITY);

				epriestleyUnsubmitted Not Done Inline Actions I 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 Actions The 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…
				if (!$can_push) {
				return array(
				pht('You do not have permission to push to this repository.'));
				}

				$push_strategy = PhabricatorEnv::newObjectFromConfig(
				'differential.land-strategy');
				epriestleyUnsubmitted Not Done Inline Actions Maybe just call this `"Done"`? It should also have `pht()`. epriestley: Maybe just call this `"Done"`? It should also have `pht()`.

				try {
				$push_strategy->assertSupportForRepository($repository);
				} catch (Exception $e) {
				return array('Repository does not support landing:', $e->getMessage());
				}

				try {
				$workspace = $this->getWorkspace($repository);
				aviveyAuthorUnsubmitted Not Done Inline Actions I 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…
				} catch (Exception $e) {
				return array('Failed to allocate a workspace.', $e->getMessage());
				}

				epriestleyUnsubmitted Not Done Inline Actions For 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…
				try {
				$push_strategy->commitRevisionToWorkspace(
				$revision,
				$workspace,
				$viewer);
				} catch (Exception $e) {
				return array('Failed to commit patch.', $e->getMessage());
				}

				try {
				$push_strategy->pushWorkspaceRepository(
				$repository,
				$workspace,
				$viewer);
				} catch (Exception $e) {
				return array('Failed to push changes upstream.', $e->getMessage());
				}

				return array();
				}

				function getWorkspace($repo) {
				return DifferentialGetWorkingCopy::getCleanGitWorkspace($repo);
				}
				}

				aviveyAuthorUnsubmitted Not Done Inline Actions I'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…
				epriestleyUnsubmitted Not Done Inline Actions This 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.