Proper use of json_encode and error handling for outputArray() in processors #modxsnowup

[christianseel]

What does it do?

This PR replaces the $modx->toJSON() and the hardcoded string in modProcessor::outputArray() with a full json_encode() call. It also adds a log message to the MODX error log if json_encode() fails and makes sure the processor still returns valid JSON.

Why is it needed?

If the $array was for any reason malformed (in our case it contained illegal UTF-8 characters), the $modx->toJSON() returns false instead of valid JSON. Due to the fact that the rest of the JSON response was hardcoded as a string the processor returned invalid JSON like

{"success":true,"total":"3","results":}

(notice the missing data after "results":)

Related issue(s)/PR(s)

(none known)

PR powered by the #modxsnowup

[christianseel]

Travis probably failed not because of the changes in this PR:

PHP Fatal error:  Class 'PHPUnit_Framework_TestCase' not found in /home/travis/build/modxcms/revolution/_build/test/MODxTestCase.php on line 29

[rtripault]

Indeed, unrelated and apparently safe to ignore

[OptimusCrime]

This +100. So much better doing it this way. Note that this breaks compatibility, so it might need to go in the 3.x branch? Perhaps the 2.x could work too? No matter which version it is introduced it, it the PHP JSON extension will need to be a server requirement from the version and up.

[christianseel]

@OptimusCrime json_encode is already widely used in the MODX core: https://github.com/modxcms/revolution/search?utf8=%E2%9C%93&q=json_encode
So I'd say it doesn't add breaking changes and it can be considered as a bug fix.

[OptimusCrime]

I see, my bad @christianseel :)

I see that the toJSON method has been changed from xpdo 2.x to 3.x.

I also see the JSON listed in the server requirements under extensions, which makes me wonder why the core has used toJSON to tasks that could just be json_encode/json_decode calls from the beginning of, as this method more or less serves not purpose other than attempting to use a user library in the 2.x version if the native json methods are missing.

[Mark-H]

The toJSON/fromJSON methods date back from a time where json_encode/json_decode support was not as universal as it is today. No longer relevant, and deprecated as listed in #13199.

[OptimusCrime]

Should it be deprecated and removed in 3.x then? This PR targets 2.5.x. I'm all in favor of removing this unnecessary library, I am just trying to understand how you are handling this + xPDO and semver.

[opengeek]

Yes, toJSON and fromJSON should be deprecated and removed from xPDO in 3.x. We still need to target 2.6 (i.e. 2.x branch) with this change.

[opengeek]

This was merged into 2.5.x and cherry-picked into 2.x — modify the base branch before merging so these PRs will automatically close.