From a1f5d37357d6fbf548062350603da033c7df271f Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 8 May 2017 16:12:27 -0700
Subject: [PATCH] Improve the behavior of PhabricatorFileEditField for Macros

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
---
 .../AphrontFileHTTPParameterType.php          | 12 ++++-
 src/view/form/control/PHUIFormFileControl.php | 50 +++++++++++++++----
 2 files changed, 51 insertions(+), 11 deletions(-)

diff --git a/src/aphront/httpparametertype/AphrontFileHTTPParameterType.php b/src/aphront/httpparametertype/AphrontFileHTTPParameterType.php
index ceff8c1ddb..c2e5f47f8f 100644
--- a/src/aphront/httpparametertype/AphrontFileHTTPParameterType.php
+++ b/src/aphront/httpparametertype/AphrontFileHTTPParameterType.php
@@ -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();
diff --git a/src/view/form/control/PHUIFormFileControl.php b/src/view/form/control/PHUIFormFileControl.php
index 57f5d30bf5..9dff7e21ca 100644
--- a/src/view/form/control/PHUIFormFileControl.php
+++ b/src/view/form/control/PHUIFormFileControl.php
@@ -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,
+    );
   }
 
 }