See T3626 for an example of this misfiring.
The Editor in the bulk edit workflow currently does setContinueOnMissingFields(true). The intent of this is to let you edit a task title even if some custom "Required" field doesn't have a value: you shouldn't need to set "Gluten Free?" on an old task in order to bulk-edit the title.
In most cases where we set this flag, we're making some sort of narrowly scoped edit (like enable/disable) and we want to let you enable or disable objects even if they're missing a title or a custom field or have some other technically-invalid constraint. In these cases, we know that we're only applying valid transactions.
However, in the case of bulk edits, you can construct invalid transactions which only raise missing field errors, e.g. setting the title to nothing. Specifically:
- Bulk edit a task.
- Choose "Change Title to: <empty field>".
- Apply the edit.
One maybe-reasonable approach is to change how transaction validation works like this:
diff --git a/src/applications/maniphest/xaction/ManiphestTaskTitleTransaction.php b/src/applications/maniphest/xaction/ManiphestTaskTitleTransaction.php index e4ec2a132..1b38dc5e6 100644 --- a/src/applications/maniphest/xaction/ManiphestTaskTitleTransaction.php +++ b/src/applications/maniphest/xaction/ManiphestTaskTitleTransaction.php @@ -69,6 +69,15 @@ final class ManiphestTaskTitleTransaction pht('Tasks must have a title.')); } + foreach ($xactions as $xaction) { + if (!strlen($xaction->getNewValue())) { + $errors[] = $this->newInvalidError( + pht('You must choose a title.'), + $xaction); + continue; + } + } + return $errors; }
But I think that will give you two errors in the UI. Another is to do this:
diff --git a/src/applications/maniphest/xaction/ManiphestTaskTitleTransaction.php b/src/applications/maniphest/xaction/ManiphestTaskTitleTransaction.php index e4ec2a132..0cfa42bed 100644 --- a/src/applications/maniphest/xaction/ManiphestTaskTitleTransaction.php +++ b/src/applications/maniphest/xaction/ManiphestTaskTitleTransaction.php @@ -65,8 +65,14 @@ final class ManiphestTaskTitleTransaction $errors = array(); if ($this->isEmptyTextTransaction($object->getTitle(), $xactions)) { - $errors[] = $this->newRequiredError( - pht('Tasks must have a title.')); + if ($xactions) { + $errors[] = $this->newInvalidError( + pht('Tasks must have a title.'), + last($xactions)); + } else { + $errors[] = $this->newRequiredError( + pht('Tasks must have a title.')); + } } return $errors;
I think that gets all the behaviors right.
Both are kind of messy/unintuitive and might require a lot of updates to existing transactions.
We could also remove setContinueOnMissingFields(true) from the bulk editor but that creates the issues with editing older stuff.
Maybe the newRequiredError() API could just be streamlined a bit so it tends to get this right by default (required error if $xactions is empty, invalid error if $xactions is nonempty).