Better error handling in CRM_Utils_JS::decode

[colemanw]

Overview

Improves the function to better handle malformed js and adds tests.

Before

Malformed js caused php notices & possibly errors.

After

Malformed js passed to json_decode which simply outputs NULL.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[totten]

Eh, returning NULL for an error condition feels ambiguous to me.

Suppose you need to validate that various documents with embedded JS are well-formed. Example documents:

1: <foo bar="{a: 123}" />
2: <foo bar="{inva.l.id" />
3: <foo bar="null"/ >
4: <foo bar="true"/ >
5: <foo bar="1234"/ >

The return value is NULL for errors, so I suppose you'd do a NULL check:

if (CRM_Utils_JS::decode($foo->bar) !== NULL) {
  echo "The foobar property is well formed.";
}
 else {
  echo "The foobar property is not well formed.";
}

But then case 3 would be a false-negative, no? (CRM_Utils_JSTest::decodeExamples() asserts that strings 'null'/'true'/etc are parsed NULL/TRUE/etc.)

Maybe an exception would clear the ambiguity?

try {
  CRM_Utils_JS::decode($foo->bar);
  echo "The foobar property is well formed.";
}
catch (JSException $e) {
  echo "The foobar property is not well formed.";
}

[colemanw]

Well, my main use-case here is to deal with javascript strings embedded in html properties. Take for example:

<af-foo af-prop="{a: 1, b: false, c: 'something', d: fetchD().result}" />

We want to read this into php because we care about the a, b & c properties, and we'll ignore d because it can't be parsed into php (it's valid JS though):

[
  'a' => 1,
  'b' => FALSE,
  'c' => 'something',
  'd' => NULL,
]

That's what I want to get back, because I need to know what's in a, b & c even if d is unparasable. If it just threw an error every time it encountered something it doesn't understand, the whole application would be more prone to failure.

[totten]

OK, so there is a reasonable scenario for hiding the error -- i.e. in the context of runtime logic, you might hit upon an expression which is valid in the eyes of the user+browser but not recognized. I can see the argument for letting execution proceed there.

In the context of afform_gui or afform_auditor, wouldn't you expect them to display a warning/error if you give them a document like this:

<af-foo af-prop="{a: "parse error no closing bracket" />

Of course, pragmatically, we're not actually writing those bits right now, and this function is mostly internal, so I don't really object to merging it. My main concern is more the principle:

"hidden errors" != "better errors"

But if there's a sense that the contract can evolve to support error-reporting when we're in a position to use it, then that's OK.

(Example "evolution": Maybe one could pass an optional $onError parameter -- function decode(string $js, callable $onError = NULL); in absence of $onError, it can continue to coerce unrecognized expressions to NULL. But with $onError, bad values can be reported.)

[colemanw]

I agree with the principle, but wonder if what we have in this PR can't suffice, at least for now. As it stands, anything it can't parse is passed to json_decode() which spits out NULL on error but also logs those errors, and they can be retrieved with json_last_error().

[totten]

OK, that sounds reasonable from current POV. 👍

Merging!