Skip to content
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

Fix error when parsing values consisting of !important only #168

Merged
merged 1 commit into from
Oct 13, 2024

Conversation

leonid-shevtsov
Copy link
Contributor

@leonid-shevtsov leonid-shevtsov commented Oct 8, 2024

I haven't found a good justification for raising the error, and it helps a lot when parsing 3rd party CSS. It would still be raised when manually creating the declarations, and not parsing a block of CSS.

The performance impact is not noticable in my benchmarks... i also have a version that optimizes adding the declaration from parsed data, but it doesn't give any more than 3% speedup, so IMO the increased complexity wasn't worth it

Feel free to close if the error is valuable.

Coincidentally it also removes the sole use case for rule_set_exceptions, so i had to emulate the error in tests.

Pre-Merge Checklist

  • CHANGELOG.md updated with short summary

@grosser
Copy link
Contributor

grosser commented Oct 8, 2024

color: !important; is technically valid but not useful right ?
... so the css that is being parsed is some auto-generated stuff that just happens to be valid and it should not blow up the parser is what you are saying ?

lib/css_parser/rule_set.rb Outdated Show resolved Hide resolved
@leonid-shevtsov
Copy link
Contributor Author

color: !important; is technically valid but not useful right ?

yes - technically valid in the sense that it does not break the overall CSS code and you would not typically notice its presence. It's certainly more valid than a missing closing brace which css_parser will silently allow: 😀

https://github.com/premailer/css_parser/blob/master/test/test_css_parser_basic.rb#L32

... so the css that is being parsed is some auto-generated stuff that just happens to be valid and it should not blow up the parser is what you are saying ?

Not really auto-generated, just some 3rd party CSS. Yeah, it was surprising that this benign mistake would cause an exception.

@leonid-shevtsov leonid-shevtsov force-pushed the fix-important-without-value branch from f56e2b5 to 037d882 Compare October 9, 2024 11:28
@grosser grosser merged commit 24077d8 into premailer:master Oct 13, 2024
6 checks passed
@grosser
Copy link
Contributor

grosser commented Oct 13, 2024

1.19.1

@leonid-shevtsov leonid-shevtsov deleted the fix-important-without-value branch October 14, 2024 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants