revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-23 05:50:55 +01:00
Code Issues Releases Wiki Activity

Improve the behavior of PhabricatorFileEditField for Macros

Browse source 
Summary:
See D17848. This improves things a little bit in two cases:

Case 1:

  - Create a macro.
  - Pick a valid file.
  - Pick an invalid name.
  - Submit form.
  - Before patch: your file is lost and you have to pick it again.
  - After patch: your file is "held" in the form, you just can't see it in the UI. If you submit again, it keeps the same file. If you pick a new file, it uses that one instead.

Case 2:

  - Apply D17848.
  - Delete the `if ($value) {` thing that I'm weirded out about (see inline).
  - Edit a macro.
  - Don't pick a new file.
  - Before patch: error, can't null the image PHID.
  - Afer patch: not picking a new file means "keep the same file", but you can't tell from the UI.

Basically, the behaviors are good now, they just aren't very clear from the UI since "the field has an existing/just-submitted value" and "the field is empty" look the same. I think this is still a net win and we can fix up the UI later.

Test Plan: See workflows above.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D17853
This commit is contained in:
epriestley 2017-05-08 16:12:27 -07:00
parent bf30c07230
commit a1f5d37357
2 changed files with 51 additions and 11 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

12
src/aphront/httpparametertype/AphrontFileHTTPParameterType.php
View file

@ -7,10 +7,17 @@ final class AphrontFileHTTPParameterType
return $key.'_raw';
}
private function getDefaultKey($key) {
return $key.'_default';
}
protected function getParameterExists(AphrontRequest $request, $key) {
$file_key = $this->getFileKey($key);
$default_key = $this->getDefaultKey($key);
return $request->getExists($key) ||
$request->getFileExists($file_key);
$request->getFileExists($file_key) ||
$request->getExists($default_key);
}
protected function getParameterValue(AphrontRequest $request, $key) {
@ -27,8 +34,9 @@ final class AphrontFileHTTPParameterType
// this code around as a fallback if the client-side JS goes awry.
$file_key = $this->getFileKey($key);
$default_key = $this->getDefaultKey($key);
if (!$request->getFileExists($file_key)) {
return null;
return $request->getStr($default_key);
}
$viewer = $this->getViewer();

50
src/view/form/control/PHUIFormFileControl.php
View file

@ -30,15 +30,47 @@ final class PHUIFormFileControl
'chunkThreshold' => PhabricatorFileStorageEngine::getChunkThreshold(),
));
return phutil_tag(
'input',
array(
'type' => 'file',
'multiple' => $this->getAllowMultiple() ? 'multiple' : null,
'name' => $this->getName().'.raw',
'id' => $file_id,
'disabled' => $this->getDisabled() ? 'disabled' : null,
));
// If the control has a value, add a hidden input which submits it as a
// default. This allows the file control to mean "don't change anything",
// instead of "remove the file", if the user submits the form without
// touching it.
// This also allows the input to "hold" the value of an uploaded file if
// there is another error in the form: when you submit the form but are
// stopped because of an unrelated error, submitting it again will keep
// the value around (if you don't upload a new file) instead of requiring
// you to pick the file again.
// TODO: This works alright, but is a bit of a hack, and the UI should
// provide the user better feedback about whether the state of the control
// is "keep the value the same" or "remove the value", and about whether
// or not the control is "holding" a value from a previous submission.
$default_input = null;
$default_value = $this->getValue();
if ($default_value !== null) {
$default_input = phutil_tag(
'input',
array(
'type' => 'hidden',
'name' => $this->getName().'_default',
'value' => $default_value,
));
}
return array(
phutil_tag(
'input',
array(
'type' => 'file',
'multiple' => $this->getAllowMultiple() ? 'multiple' : null,
'name' => $this->getName().'_raw',
'id' => $file_id,
'disabled' => $this->getDisabled() ? 'disabled' : null,
)),
$default_input,
);
}
}