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

Replace CaseType's own XML encoding function #17100

Merged
merged 1 commit into from
Apr 18, 2020

Conversation

artfulrobot
Copy link
Contributor

The problem is detailed at https://lab.civicrm.org/dev/core/-/issues/1719

In a nutshell:

  1. Creating a case type crashes if the name of a case status is not valid XML content. e.g. if a name has & in it, but other special characters would also cause problems.

  2. It dies because it fails to properly encode the value.

  3. It uses its own encoding functions instead of a proper XML encoder.

This PR:

  1. replaces the homespun XML encoder with php's own built in one, and properly encodes everything.

  2. adds a test fixture for the case that & is in a status name.

As a side effect, I believe it fixes 1719

@civibot
Copy link

civibot bot commented Apr 17, 2020

(Standard links)

@civibot civibot bot added the master label Apr 17, 2020
@artfulrobot artfulrobot force-pushed the artfulrobot-lab-1917 branch from eee4775 to e97ef29 Compare April 17, 2020 17:12
@artfulrobot
Copy link
Contributor Author

Note: I had to update the xml fixtures because PHP's https://www.php.net/manual/en/book.xmlwriter.php writes the doctype a little differently (UTF not utf and no space before the closing ?>).

@mattwire
Copy link
Contributor

ping @demeritcowboy

@demeritcowboy
Copy link
Contributor

The test failure is real but seems like it's mostly just the utf->UTF thing. There's also a whitespace difference between what came out before and after, but that seems mostly clerical. If anybody is relying on specific whitespace in their xml they shouldn't be, but the test is complaining.

@artfulrobot artfulrobot force-pushed the artfulrobot-lab-1917 branch from e97ef29 to a45cd5a Compare April 18, 2020 07:56
@artfulrobot
Copy link
Contributor Author

Thanks @demeritcowboy Pushed new commit with fixture updates (whtespace and case of 'utf' → 'UTF' only) seems to get the test passing. Tested locally for the failed test and the original tests, all fine, but let's see what Jenkins has to say about it.

@colemanw
Copy link
Member

Code is an excellent improvement, and test coverage is solid.

@colemanw colemanw merged commit 4327f20 into civicrm:master Apr 18, 2020
@artfulrobot artfulrobot deleted the artfulrobot-lab-1917 branch April 18, 2020 16:25
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