mraible commented on code in PR #228:
URL: https://github.com/apache/roller/pull/228#discussion_r4402290752
##########
app/src/test/java/org/apache/roller/weblogger/util/UtilitiesTest.java:
##########
@@ -99,6 +99,29 @@ public void testRemoveHTML() {
assertEquals(expect, result);
}
+ @Test
+ public void testRemoveHTMLUnclosedTag() {
+ assertEquals("about", Utilities.removeHTML("about <img src=x", false));
+ assertEquals("about", Utilities.removeHTML("about <p>", false).trim());
+ assertEquals("a", Utilities.removeHTML("a </p", false));
+ assertEquals("a", Utilities.removeHTML("a <!-- note", false));
+ assertEquals("", Utilities.removeHTML("<img src=x", false));
+ }
+
+ @Test
+ public void testRemoveHTMLKeepsLessThanText() {
+ assertEquals("a < b", Utilities.removeHTML("a < b", false));
+ assertEquals("1 <2", Utilities.removeHTML("1 <2", false));
+ assertEquals("x <", Utilities.removeHTML("x <", false));
+ }
Review Comment:
These only cover `addSpace=false`, but `Weblog.setAbout` and friends call
`removeHTML(str)`, which passes `true`. Could you add the default overload and
a no-space case like `removeHTML("x<y", false)`, so the behavior change is
explicit?
##########
app/src/main/java/org/apache/roller/weblogger/util/Utilities.java:
##########
@@ -183,10 +183,20 @@ public static String removeHTML(String str, boolean
addSpace) {
start = endTag + 1;
beginTag = str.indexOf('<', start);
}
- // if no endTag found, get rest of str and break
+ // if no endTag found, drop the rest if this '<' starts a tag,
+ // which a browser would still parse as markup; otherwise keep the
+ // '<' as text and look at the next one
else {
- ret.append(str.substring(beginTag));
- break;
+ if (startsTag(str, beginTag)) {
Review Comment:
This also drops plain text where a letter follows `<` and no `>` comes
later. `removeHTML("x<y", false)` used to return `x<y` and now returns `x`, and
`List<String` becomes `List`. Weblog uses property access, so
`setName`/`setTagline`/`setAbout` run this on every load, and an existing
weblog named `C<C++ fans` would show as `C`. It's also used by category names
and `WeblogEntry.getDisplayTitle`. Could the fallback be narrower, for example
only dropping the tail when it looks like a tag name plus attributes? Or at
least call it out in CHANGES.md.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
