Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CRM-21458: Replace addcslashes call with json_encode creating options #196

Merged
merged 2 commits into from
Nov 21, 2017

Conversation

agileware-fj
Copy link
Contributor

json_encode should ensure that all escapes required for JS string values
are present.


…tions

json_encode should ensure that *all* escapes required for JS string values
are present.

----------------------------------------
* CRM-21458: (CIVICRM-742) HTML_QuickForm_hierselect doesn't handle JS escaping properly
  https://issues.civicrm.org/jira/browse/CRM-21458
@civicrm-builder
Copy link

Can one of the admins verify this patch?

@eileenmcnaughton
Copy link
Contributor

test this please

@agileware-fj
Copy link
Contributor Author

From http://php.net/manual/en/function.json-encode.php:

Like the reference JSON encoder, json_encode() will generate JSON that is a simple value (that is, neither an object nor an array) if given a string, integer, float or boolean as an input value. While most decoders will accept these values as valid JSON, some may not, as the specification is ambiguous on this point.

Because the output of this function is actually generating an inline script to be loaded on the page, we don't need to be worried about JSON parsing compatibility, while enjoying the benefit of a string that is encoded to work correctly with Javascript.

@totten
Copy link
Member

totten commented Nov 21, 2017

(CiviCRM Review Template WORD-1.0)

  • (r-jira) Pass
  • (r-test) Pass
  • (r-code) Pass: I was a little concerned because civicrm-packages traditionally had a weird protocol wherein one was expected to copy the old file to _ORIGINAL_ (to indicate that it's been forked). Fortunately, that's already been done. 👍
  • (r-doc) Pass
  • (r-maint) Pass
  • (r-run) Pass: Tested the steps described in CRM-21458. Couldn't think of anything else to try.
  • (r-user) Pass: Generally preserves behavior of UI.
  • (r-tech) Pass:
    • This only affects the UI.
    • I suppose it's possible that someone might have put funny codes in their titles (in their data) to work-around this bug. However, that seems unlikely given that addcslashes() was already active (which would have covered several edge-cases and also would interfere with manual workarounds).

@totten totten merged commit c69d286 into civicrm:master Nov 21, 2017
@agileware-fj agileware-fj deleted the CRM-21458 branch November 26, 2017 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants