-
Notifications
You must be signed in to change notification settings - Fork 71
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
Properly handle <!-- <script </script in JavaScript #1429
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1429 +/- ##
==========================================
- Coverage 49.10% 49.09% -0.02%
==========================================
Files 9143 9150 +7
Lines 80545 80633 +88
==========================================
+ Hits 39553 39584 +31
- Misses 40992 41049 +57 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explanatory comments for reviewers.
repository/Seaside-Canvas.package/WAScriptTag.class/instance/with..st
Outdated
Show resolved
Hide resolved
repository/Seaside-Canvas.package/WAScriptTag.class/instance/with..st
Outdated
Show resolved
Hide resolved
...easide-Tests-Canvas.package/WADefaultScriptGeneratorTest.class/instance/testNestedScripts.st
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for having addressed this. It was pending on my stack since a while ;-)
Just a minor method revert to change and this is good to go for me!
repository/Seaside-Tests-Canvas.package/WAScriptGeneratorTest.class/instance/assert.gives..st
Outdated
Show resolved
Hide resolved
repository/Seaside-Canvas.package/WAScriptTag.class/instance/with..st
Outdated
Show resolved
Hide resolved
@marschall I noticed you made the change but did not trigger a new review. I assume this PR is ready? If it is, I will happily merge it. |
Yes, it is.
That would be great. |
This is my try a fixing #1379.
If we check the release version of 4.12.1.3 Restrictions for contents of script elements we need to handle <!--, <script and </script.
I tried to do meaningful commits.
Fixes #1379