Do not throw Aphront400Response when editing a task
AcceptedPublic
Actions

Authored by aklapper on Jul 15 2024, 16:13.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15866: Aphront400Response when editing a task

Summary

HTTP 400 is supposed to be a Bad Request: The server cannot or will not process the request due to an apparent client error.
That is not the case when creating a preview - that request is valid. Thus do not throw a HTTP 400 in the background while the rest of the user activity passes anyway.

Closes T15866

Test Plan

Check the web browser's network console when

editing a Maniphest task's project tags and priority via the "Add Action" field
adding a comment to a Maniphest task and signing it with 2FA
editing/adding in other applications relying on __preview__

Diff Detail

Repository

rP Phorge

Branch

T15886doNotHttp400

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 1439
Build 1439: arc lint + arc unit

Event Timeline

aklapper created this revision.Jul 15 2024, 16:13

Owners added a reviewer: O1: Blessed Committers.Jul 15 2024, 16:13

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptJul 15 2024, 16:13

aklapper requested review of this revision.Jul 15 2024, 16:13

Harbormaster completed remote builds in B1439: Diff 2247.Jul 15 2024, 16:13

I guess I just do not enjoy a good number of HTTP 400 errors in our error logs when literally nothing went wrong at all.

valerio.bozzolan added inline comments.Aug 9 2024, 07:15

src/aphront/AphrontRequest.php
708	I don't understand potential consequences of removing `isFormPost()` so maybe we can keep it, and "just" add the missing `isAjax()` return ($this->isFormPost() \|\| $this->isAjax()) && $this->getStr('__preview__');

I forgot to accept but please consider the little cute small comment to add the corner case for the background ajax request, that I suspect it may work too and I hope with less unexpected situations. There was probably a reason for that isFormPost(). For example see isContinueRequest() that is even stricter in a way. Thanks :)

This revision is now accepted and ready to land.Wed, Dec 11, 07:31