Escaping Tags in XLIFF #2936
Escaping Tags in XLIFF #2936
Conversation
backend/data/src/main/kotlin/io/tolgee/formats/xliff/util/XliffHtmlEscaper.kt
Outdated
Show resolved
Hide resolved
| fun escapeHtmlTags(text: String): String { | ||
| var result = text.replace("\"", """) | ||
|
|
||
| val regex: Regex = "<(/?[^>]+)>".toRegex() |
There was a problem hiding this comment.
imho no need to compile regex every function call since it's always same, I'd extract it to private variable. Also maybe question of tag validity is on the table??? because this regex will also match for example <<<p> which is not a valid tag according to specification (correct me if I'm wrong) @JanCizmar what is your take on this?
There was a problem hiding this comment.
Validity of the markup is guaranteed by the Parser, Escaping takes place after Parser has successfully parsed it
If you try it with invalid markup it will throw an error
There was a problem hiding this comment.
It would make sense to replace all the possible xml syntax, but I am proposing the Apache commons solution in my review.
@cardvvs Thanks for the review anyway.
neo773
force-pushed
the
xliff-escape-html
branch
from
419d60e to
717b5bf
Compare
| @@ -50,7 +51,13 @@ class Xliff12FileProcessor( | |||
| transUnitId: String, | |||
| language: String, | |||
| ) { | |||
| val converted = convertMessage(text, language) | |||
| val textToConvert = | |||
There was a problem hiding this comment.
I don't think it makes any sense to escape html when importing. This wasn't in the scope of the task.
There was a problem hiding this comment.
Will remove it, I only added it since the author of the issue mentioned it
| @field:Parameter( | ||
| description = ESCAPE_HTML_DESCRIPTION, | ||
| ) | ||
| override var escapeHtml: Boolean? = false, |
There was a problem hiding this comment.
Please add documentation comment.
/** Enabling or disabling HTML escaping for XLIFF format. Some tools expect the XML/HTML tags escaped and don't accept raw unescaped tags. */
| @@ -129,4 +129,7 @@ class ContentDeliveryConfig( | |||
|
|
|||
| @ActivityLoggedProp | |||
| override var fileStructureTemplate: String? = null | |||
|
|
|||
| @ActivityLoggedProp | |||
| override var escapeHtml: Boolean? = false | |||
There was a problem hiding this comment.
There is no reason to make it nullable. Make it false by default and add @ColumnDefault annotation so tests won't fail.
backend/data/src/main/kotlin/io/tolgee/formats/xliff/util/XliffHtmlEscaper.kt
Outdated
Show resolved
Hide resolved
neo773
force-pushed
the
xliff-escape-html
branch
from
d31dfb5 to
8e8b2ac
Compare
|
Apologies for the spam, the branch ran into several issues and I had to reset it and do a merge from a clean branch. |
|
Hey! So does it currently also work with the CDN and is it tested? |
|
@JanCizmar |
|
@JanCizmar a.mp4 |
|
Hello! Thanks a lot it's great. I would like to see a cypress test, which tests that the CD configuration is actually saved with the the new checkbox value. That's my last input before I jump on the final Code Review. Thanks a lot for the persistence on this. |
|
Hello! Are there any updates on this? |
backend/data/src/main/kotlin/io/tolgee/dtos/ExportParamsDocs.kt
Outdated
Show resolved
Hide resolved
backend/data/src/main/kotlin/io/tolgee/dtos/request/ContentDeliveryConfigRequest.kt
Outdated
Show resolved
Hide resolved
webapp/src/views/projects/developer/contentDelivery/CdEscapeHtml.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Jan Cizmar <[email protected]>
Co-authored-by: Jan Cizmar <[email protected]>
This PR adds support for escaping HTML tags in XLIFF
Check list
/claim #2912
/closes #2912