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

#690 - Grant access to StringEscapeUtils.escapeXml11 #692

Closed
wants to merge 1 commit into from

Conversation

bpeptena
Copy link

@bpeptena bpeptena commented Mar 28, 2024

This commit fixes #690

@kwin
Copy link
Member

kwin commented Mar 28, 2024

@bpeptena Looks good, can you add a unit test for it?

@Positronic-Brain
Copy link
Contributor

I'd also document it under /docs/AdvancedFeatures.md

@Positronic-Brain
Copy link
Contributor

AH, already there- Please disregard my comment - leaving my old comment as proof of my near-blindness >_<

@@ -24,6 +24,7 @@ Function Signature | Description
`startsWith(String str, String prefix)`| [`StringUtils.startsWith(...)`](https://commons.apache.org/proper/commons-lang/javadocs/api-3.3/org/apache/commons/lang3/StringUtils.html#startsWith(java.lang.CharSequence,%20java.lang.CharSequence))
`length(String string)`| [`StringUtils.length(...)`](https://commons.apache.org/proper/commons-lang/javadocs/api-3.3/org/apache/commons/lang3/StringUtils.html#length(java.lang.CharSequence))
`defaultIfEmpty(String str, String default)` | [`StringUtils.defaultIfEmpty(...)`](https://commons.apache.org/proper/commons-lang/javadocs/api-3.3/org/apache/commons/lang3/StringUtils.html#defaultIfEmpty(T,%20T))
`escapeXml11(String str)` | [`StringEscapeUtils.escapeXml11(...)`](https://commons.apache.org/proper/commons-lang/javadocs/api-3.3/org/apache/commons/lang3/StringEscapeUtils.html#escapeXml11(java.lang.String))
Copy link
Member

Choose a reason for hiding this comment

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

Sorry to only look at this now - @bpeptena I would prefer to expose this method as escapeXml() to the yaml files and I would keep the fact, that StringEscapeUtils.escapeXml11() is called under the hood an implementation detail (it's fine to mention it in the docu, but I would not force all users to write the method with ...11 in the yaml files.

Especially because XML is not exactly a fast moving technology, I don't expect a StringEscapeUtils.escapeXml12() or even StringEscapeUtils.escapeXml20() to pop up soon ;-)

Copy link
Member

Choose a reason for hiding this comment

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

There is a difference between XML 1.0 and 1.1 with respect to escape characters. We need the 1.0 variant for FileVault XML. Also making it explicit in the method name exposed in EL is IMHO a good thing.

Copy link
Member

@ghenzler ghenzler May 8, 2024

Choose a reason for hiding this comment

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

I'd prefer to have the AC Tool select the correct encoding, as developer I just want to encode the xml (and call the method encodeXml(), then the AC Tool should select the correct way (maybe even depending on the AEM version in the future, but most likely will just be StringEscapeUtils.escapeXml10() as Docview requires that)

Copy link
Member

Choose a reason for hiding this comment

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

How should ACTool know the context? In some contexts XML 1.1 might make sense. The EL Parser doesn't really consider where this is being used (if with initialContent or somewhere else and we should IMHO leave it context free.

@kwin
Copy link
Member

kwin commented May 8, 2024

Superseded by #712

@kwin kwin closed this May 8, 2024
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.

When using variables in initialContent, allow to properly escape them
4 participants