Page MenuHomePhabricator

"RemarkupField" returns "RemarkupValue" when editing defaults, which fails to survive serialization
Closed, ResolvedPublic

Description

See PHI2203. After recent changes for file handling, editing the form defaults for a form which includes a Remarkup text field causes it to emit a "RemarkupValue" as a default value. This value can't be serialized to the database and ends up stored as an empty array, which fatals when loaded.

The immediate issue is easy to fix, but ideally this sort of thing should fail loudly at serialization time, rather than once the database has already been poisoned.

Event Timeline

epriestley triaged this task as Normal priority.Jun 14 2022, 1:09 PM
epriestley created this task.

...ideally this sort of thing should fail loudly at serialization time...

This is because json_encode() is extremely forgiving of inputs. Behavior can be improved significantly like this:

diff --git a/src/object/Phobject.php b/src/object/Phobject.php
index 4d799306..f7df4798 100644
--- a/src/object/Phobject.php
+++ b/src/object/Phobject.php
@@ -17,7 +17,10 @@
  * (Legitimately iterable subclasses can provide a working implementation of
  * Iterator instead.)
  */
-abstract class Phobject implements Iterator {
+abstract class Phobject
+  implements
+    Iterator,
+    JsonSerializable {
 
   public function __get($name) {
     throw new DomainException(
@@ -33,6 +36,13 @@ abstract class Phobject implements Iterator {
         get_class($this).'::'.$name));
   }
 
+  public function jsonSerialize() {
+    throw new DomainException(
+      pht(
+        'Attempt to serialize unserializable object (of class "%s") to JSON.',
+        get_class($this)));
+  }
+
   #[\ReturnTypeWillChange]
   public function current() {
     $this->throwOnAttemptedIteration();

...but this is a bit risky, so I'm not going to press my luck for now.

I believe we may be hitting either the problem one of the above commit fixes, or suffering from a similar caused as side-effect from it.

https://phabricator.wikimedia.org/T312614

Basically, any maniphest task form we edit from today onwards makes that form inaccessible, seemingly in an irreversible manner. Even a no-op edit form submission will break it. This smells like a roundtrip issue where something gets corrupted and then sticks that way.

Our downstream trace:

PHP message: [2022-07-08 03:13:12] EXCEPTION: (RuntimeException) Array to string conversion at [<arcanist>/src/error/PhutilErrorHandler.php:261]
PHP message: arcanist(), ava(), phabricator(), translations(), wmf-ext-misc()
PHP message:   #0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer, array) called at [<phabricator>/src/view/form/control/AphrontFormTextAreaControl.php:76]
PHP message:   #1 <#2> AphrontFormTextAreaControl::renderInput() called at [<phabricator>/src/view/form/control/PhabricatorRemarkupControl.php:390]
PHP message:   #2 <#2> PhabricatorRemarkupControl::renderInput() called at [<phabricator>/src/view/form/control/AphrontFormControl.php:172]
PHP message:   #3 <#2> phutil_tag(string, array, array) called at [<phabricator>/src/view/form/PHUIFormLayoutView.php:54]
PHP message:   #4 <#2> PHUIFormLayoutView::render() called at [<phabricator>/src/view/form/AphrontFormView.php:160]
PHP message:   #5 <#2> phutil_tag(string, array, array) called at [<phabricator>/src/infrastructure/javelin/markup.php:70]
PHP message:   #6 <#2> phutil_escape_html(array) called at [<phabricator>/src/infrastructure/markup/render.php:139]
PHP message:   #7 <#2> phutil_escape_html(array) called at [<phabricator>/src/infrastructure/markup/render.php:97]
PHP message:   #8 <#2> phutil_tag(string, array, array) called at [<phabricator>/src/view/phui/PHUITwoColumnView.php:203]
PHP message:   #9 <#2> PHUITwoColumnView::buildMainColumn() called at [<phabricator>/src/view/phui/PHUITwoColumnView.php:121]
PHP message:   #10 <#2> PHUITwoColumnView::getTagContent() called at [<phabricator>/src/view/AphrontTagView.php:161]
PHP message:   #11 <#2> AphrontTagView::render() called at [<phabricator>/src/view/AphrontView.php:222]
PHP message:   #12 <#2> AphrontView::producePhutilSafeHTML() called at [<phabricator>/src/infrastructure/markup/render.php:115]
PHP message:   #13 <#2> phutil_escape_html(PHUITwoColumnView) called at [<phabricator>/src/infrastructure/markup/render.php:171]
PHP message:   #14 <#2> phutil_implode_html(string, array) called at [<phabricator>/src/view/page/PhabricatorBarePageView.php:58]
PHP message:   #15 <#2> PhabricatorBarePageView::willRenderPage() called at [<phabricator>/src/view/page/PhabricatorStandardPageView.php:217]
PHP message:   #16 <#2> PhabricatorStandardPageView::willRenderPage() called at [<phabricator>/src/view/page/AphrontPageView.php:46]
PHP message:   #17 <#2> AphrontPageView::render() called at [<

I do see a mention of wmf-ext-misc and I've let our Phab admins know as well, so if this is a purely "us" problem feel free to ignore, but I figured I'd pass it on here as well in case its something else.

That's very likely the same problem, and I think it should be fixed by updating to the current stable (rPf2a7db1 or newer).

I filed T13687 as a followup for preventing this particular sort of error (where a Phobject is incorrectly serialized directly).