Skip to content

771: restore pre-py3.13 xml attribute escaping behaviour for \r\n\t#772

Merged
lindsay-stevens merged 1 commit into
XLSForm:masterfrom
lindsay-stevens:pyxform-771
Jun 17, 2025
Merged

771: restore pre-py3.13 xml attribute escaping behaviour for \r\n\t#772
lindsay-stevens merged 1 commit into
XLSForm:masterfrom
lindsay-stevens:pyxform-771

Conversation

@lindsay-stevens

@lindsay-stevens lindsay-stevens commented Jun 14, 2025

Copy link
Copy Markdown
Contributor

Closes #771

Why is this the best possible solution? Were any other approaches considered?

In py3.13, minidom started escaping \r\n\t in attributes in order to comply with the XML spec, which pyxform adopted for all python versions in an earlier commit. But it was found that this change caused a HTTP 500 error for Enketo (not Collect/Webforms). If a user had choice filter in a multi-language form, and the filter expression used select(), then Enketo could not display the form but displayed an error saying "Cannot read properties of undefined ( reading length)". Details in XLSForm/pyxform/#771 - many test cases were explored such as constraint, calculate, relevant, single or multi-language forms, choice filter expressions and operators, but only the one mentioned above seemed to trigger the error. Since it seems a reasonably common trigger (using newlines to format expressions, and using selected() in choice filters, within a multi-language form), and it may take some time for both Enketo to fix the issue and for that release to be adopted by users, pyxform reverts to the previous behaviour.

What are the regression risks?

It seems like there are a few XForm generator tools out there so if they adopt python 3.13 they may hit the same issue, but that is not really a pyxform problem as such - they could follow/comment/fix the issue enketo/enketo#1408

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No, it seems a bit too detailed an unlikely an issue to mention anywhere that pyxform isn't escaping \r\n\t in attribute values. If needed, a forum post could reference this PR and it's linked issues.

Before submitting this PR, please make sure you have:

  • included test cases for core behavior and edge cases in tests
  • run python -m unittest and verified all tests pass
  • run ruff format pyxform tests and ruff check pyxform tests to lint code
  • verified that any code or assets from external sources are properly credited in comments

- in py3.13, minidom started escaping \r\n\t in attributes in order to
  comply with the XML spec, which pyxform adopted for all python
  versions in an earlier commit. But it was found that this change
  caused a HTTP 500 error for Enketo (not Collect/Webforms). If a
  user had choice filter in a multi-language form, and the filter
  expression used `select()`, then Enketo could not display the form
  but displayed an error saying "Cannot read properties of undefined (
  reading length)". Details in XLSForm/pyxform/XLSForm#771 - many test cases
  were explored such as constraint, calculate, relevant, single or
  multi-language forms, choice filter expressions and operators, but
  only the one mentioned above seemed to trigger the error. Since it
  seems a reasonably common trigger (using newlines to format
  expressions, and using `selected()` in choice filters), and it may
  take some time for both Enketo to fix the issue and for that release
  to be adopted by users, pyxform reverts to the previous behaviour.
@lindsay-stevens lindsay-stevens changed the title chg: restore pre-py3.13 xml attribute escaping behaviour for \r\n\t 771: restore pre-py3.13 xml attribute escaping behaviour for \r\n\t Jun 14, 2025

@lognaturel lognaturel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double checking my understanding -- when I reviewed the original PR my understanding was that by virtue of using Python 3.13, the escaping would happen automatically for pyxform. And then I thought the changes that you're undoing here were just to make other Python versions consistent. But seeing this now, it looks like this escaping was always custom for all Python versions, is that correct?

@lindsay-stevens

Copy link
Copy Markdown
Contributor Author

@ln yes the custom implementations are there to achieve output that is consistent / compliant XML / compatible with Enketo across python versions. Even if there was no Enketo issue, custom implementations would still have been needed:

  • element text: don't escape double quotes (they are in < py3.13 standard lib, but are not >= py3.13).
  • attributes: don't escape \r\n\t (they are not in < py3.13 standard lib, but are >= py.3.13).

Also the standard library escaping algo does repeated searches/calls like if token in string: string.replace() while pyxform tries to do less operations with if any(t in string for t in tokens): string.translate().

@lindsay-stevens lindsay-stevens merged commit 660d04c into XLSForm:master Jun 17, 2025
13 checks passed
@lindsay-stevens lindsay-stevens deleted the pyxform-771 branch June 17, 2025 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Some forms with escaped newlines crash Enketo

2 participants