-
Notifications
You must be signed in to change notification settings - Fork 95
Make SAC Parser replaceable #643
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
base: main
Are you sure you want to change the base?
Make SAC Parser replaceable #643
Conversation
# Introduce ParserFactory which allows to use a different CssParser # Make CssScanner independent of the concrete implementation of the parser # Encapsulate CSS Parser implementation specific stufff in separate package and class
|
@spassarop - Have you reviewed this PR? |
|
Regarding what the PR intends to implement, it looks good to me. Unfortunately, we have no context about the exception handling you are curious about. One of them will be deleted in the next major version anyway. @davewichers if the styling or anything else you want to check at high level is all right, then it can be merged. |
|
In In CssScanner#L324 - in my opinon the Batik ParseException can never happen, not sure if I missed something here. I was a little bit confused, why it is caught at this point. The parsing happens later. The http client only returns a string which later gets parsed as css, but maybe I am wrong, because I've never written such an handler. |
|
@spassarop - can you review/answer these questions? I don't want to merge until the discussion is complete, you are OK with everything. |
|
For the exception we are not sure why it’s there, even though CssException
is added, ParseException can be readded and have everyone to ensure
“backwards compatibility” unless there is a reason for not doing it.
The rest, as I said yesterday, it’s fine.
Il giorno lun 24 nov 2025 alle 11:30 Dave Wichers ***@***.***>
ha scritto:
… *davewichers* left a comment (nahsra/antisamy#643)
<#643 (comment)>
@spassarop <https://github.com/spassarop> - can you review/answer these
questions? I don't want to merge until the discussion is complete, you are
OK with everything.
—
Reply to this email directly, view it on GitHub
<#643 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHL3BMOZ2QNNW4MVBQDMBWL36MJBZAVCNFSM6AAAAACL42JFVKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTKNZRGA4DCNJVGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@spassarop - Can you do whatever other work is needed to get this ready to be merged in? |
|
My proposal would be not re-add the ParseException because when re-adding it we loose the separation of the batik part and the non-batik part. In my opininion the way it is in the pull request should work as it is. I was only asking the question about the exception handling to be sure that I did not miss something. |
|
I get your point. Regarding the TODO comments, they can be removed. And the @ahiltenkamp - Can you do that so this is merged? |
The Batik CSS Parser is not active maintained and supports only CSS 2.0
As a step towards replacing Batik all parser implementation specific things where put into the CSSParser class.
Because the CSSParser is the only Class that has a direct dependency to Batik it was moved to a separate batik package
The CSSScanner now uses the SAC Parser Interface instead of the CssParser which extends the BatikParser
It also uses a (own) ParserFactory which allows to use a different Css parser without having to modify the AntiSamy code.
There are two places where the old exception handling was a little bit weird, and I don't know the reason why it was this way. I've marked the two places with @todo
The failing test also exist currently in the antisamy main branch - I did not try to fix it.