Page MenuHomePhorge

Aphront400Response when editing a task
Open, Needs TriagePublic

Description

Phorge at ed9d212013cd0d8747f92bca4e7cb94900bccfd5. PHP version irrelevant as we've also seen this in downstream for a while.

When editing a task via the "Add Action..." dropdown and e.g. changing its priority and status (adding a text comment is not needed),

  • In PhabricatorEditEngine::buildResponse(), $action = 'comment' so the code does return $this->buildCommentResponse($object).
  • PhabricatorEditEngine::buildCommentResponse($object) calls/checks if (!$request->isFormOrHisecPost()) { return new Aphront400Response(); }.
  • AphrontRequest defines const TYPE_FORM = '__form__'.
  • AphrontRequest::isFormOrHisecPost() checks that $post = $this->getExists(self::TYPE_FORM) && $this->isHTTPPost(). The first condition fails as $this->getExists(self::TYPE_FORM) returns false because the payload (which can be inspected via json_encode($request->getRequestData())) in this single call does NOT include "__form__":"1": '{"__preview__":"1","editengine.actions":"[{\"type\":\"priority\",\"value\":\"high\",\"initialValue\":null},{\"type\":\"status\",\"value\":\"invalid\",\"initialValue\":null}]","viewData":"[]","__ajax__":"true","__metablock__":"14","__path__":"\/maniphest\/task\/edit\/3801\/comment\/"}'
  • As AphrontRequest::isFormOrHisecPost() returns false to PhabricatorEditEngine::buildCommentResponse(), it triggers return new Aphront400Response().

Thus I believe that the logic in rP543f2b6bf156be8eed7764927663d4366a8e1561 is slightly wrong.

Screenshot from 2024-06-24 14-03-07.png (599×1 px, 155 KB)

Revisions and Commits

Event Timeline

The diff below suppresses the error (and checks like trying to "Sign With MFA" without another action should fail etc still work) but no idea about potential side effects as isPreviewRequest() is called in numerous places.

Does anyone know/understand this code a bit better?

The other option would be including "__form__":"1" in that call to avoid the 400.

1diff --git a/src/aphront/AphrontRequest.php b/src/aphront/AphrontRequest.php
2index 2561e397b6..4337577c3e 100644
3--- a/src/aphront/AphrontRequest.php
4+++ b/src/aphront/AphrontRequest.php
5@@ -705,7 +705,7 @@ final class AphrontRequest extends Phobject {
6 }
7
8 public function isPreviewRequest() {
9- return $this->isFormPost() && $this->getStr('__preview__');
10+ return $this->getStr('__preview__');
11 }
12
13 /**
14diff --git a/src/applications/transactions/editengine/PhabricatorEditEngine.php b/src/applications/transactions/editengine/PhabricatorEditEngine.php
15index a696fb6cf7..2021da3615 100644
16--- a/src/applications/transactions/editengine/PhabricatorEditEngine.php
17+++ b/src/applications/transactions/editengine/PhabricatorEditEngine.php
18@@ -1892,10 +1892,11 @@ abstract class PhabricatorEditEngine
19
20 $controller = $this->getController();
21 $request = $controller->getRequest();
22+ $is_preview = $request->isPreviewRequest();
23
24 // NOTE: We handle hisec inside the transaction editor with "Sign With MFA"
25 // comment actions.
26- if (!$request->isFormOrHisecPost()) {
27+ if (!$request->isFormOrHisecPost() && !$is_preview) {
28 return new Aphront400Response();
29 }
30
31@@ -1911,7 +1912,6 @@ abstract class PhabricatorEditEngine
32
33 $fields = $this->buildEditFields($object);
34
35- $is_preview = $request->isPreviewRequest();
36 $view_uri = $this->getEffectiveObjectViewURI($object);
37
38 $template = $object->getApplicationTransactionTemplate();

The other option would be including "__form__":"1" in that call to avoid the 400.

I appreciate when method names describe exactly what they do. isPreviewRequest() currently does not, it seems...

Some notes to myself:

[acko@fedora phorge]$ grep -r isPreviewRequest .
./phorge/src/applications/badges/controller/PhabricatorBadgesCommentController.php:    $is_preview = $request->isPreviewRequest();
./phorge/src/applications/ponder/controller/PonderQuestionCommentController.php:    $is_preview = $request->isPreviewRequest();
./phorge/src/applications/ponder/controller/PonderAnswerCommentController.php:    $is_preview = $request->isPreviewRequest();
./phorge/src/applications/draft/storage/PhabricatorDraft.php:    if ($request->isPreviewRequest()) {
./phorge/src/applications/pholio/controller/PholioMockCommentController.php:    $is_preview = $request->isPreviewRequest();
./phorge/src/applications/transactions/editengine/PhabricatorEditEngine.php:    $is_preview = $request->isPreviewRequest();
./phorge/src/applications/slowvote/controller/PhabricatorSlowvoteCommentController.php:    $is_preview = $request->isPreviewRequest();
./phorge/src/aphront/AphrontRequest.php:  public function isPreviewRequest() {
[acko@fedora phorge]$ grep -r TYPE_PREVIEW .
./phorge/src/aphront/AphrontRequest.php:  const TYPE_PREVIEW = '__preview__';
^ NEVER USED.
[acko@fedora phorge]$ grep -r "__preview__" .
./phorge/src/aphront/AphrontRequest.php:  const TYPE_PREVIEW = '__preview__';
./phorge/src/aphront/AphrontRequest.php:    return $this->isFormPost() && $this->getStr('__preview__');
https://we.phorge.it/source/phorge/browse/master/webroot/rsrc/js/application/transactions/behavior-transaction-comment-form.js$16 : obj.__preview__ = 1;
https://we.phorge.it/source/phorge/browse/master/webroot/rsrc/js/application/transactions/behavior-comment-actions.js$90 :   data.__preview__ = 1;
^ none of these last two files set `__form__` explicitly either; see also `__form__` in https://we.phorge.it/source/phorge/browse/master/src/infrastructure/javelin/markup.php$136

So this is "just" something visible from server logs or browser console, right? It is not an error ever visible to user interface.

Thanks for digging into this in any case. Wow.