antisamy icon indicating copy to clipboard operation
antisamy copied to clipboard

Make SAC Parser replaceable

Open ahiltenkamp opened this issue 1 month ago • 5 comments

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.

ahiltenkamp avatar Nov 12 '25 15:11 ahiltenkamp

@spassarop - Have you reviewed this PR?

davewichers avatar Nov 17 '25 14:11 davewichers

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.

spassarop avatar Nov 22 '25 20:11 spassarop

In CssScanner#L171 I was not sure if it is ok to catch the CssException here because before it was a ParseException. Was it just because before there was no CssException. It's only some kind of reminder to extra check if I have broken something at this point.

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.

ahiltenkamp avatar Nov 24 '25 13:11 ahiltenkamp

@spassarop - can you review/answer these questions? I don't want to merge until the discussion is complete, you are OK with everything.

davewichers avatar Nov 24 '25 14:11 davewichers

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) https://github.com/nahsra/antisamy/pull/643#issuecomment-3571081553

@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 https://github.com/nahsra/antisamy/pull/643#issuecomment-3571081553, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHL3BMOZ2QNNW4MVBQDMBWL36MJBZAVCNFSM6AAAAACL42JFVKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTKNZRGA4DCNJVGM . You are receiving this because you were mentioned.Message ID: @.***>

spassarop avatar Nov 24 '25 15:11 spassarop