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

@media block gives syntax error #352

Closed
crisp-tweakers opened this issue Dec 24, 2021 · 9 comments · Fixed by #456
Closed

@media block gives syntax error #352

crisp-tweakers opened this issue Dec 24, 2021 · 9 comments · Fixed by #456

Comments

@crisp-tweakers
Copy link

@media (min-width: 1220px) {
  div { width:auto; }
}

gives an "Syntax error in CSS: Identifier expected. Got “}” [line no: 3]"

This looks like a regression in version 8.4.0 - I'm suspecting #162

@michaelhue
Copy link

michaelhue commented Jan 7, 2022

I can confirm the issue. We also had errors in @keyframes rules.
Reverting to 8.3.0 fixed the problem.

@mikkorantalainen
Copy link

Is there any progress with this? I tried to upgrade from 8.3.0 to latest code from composer which claims to be be "8.4.0" even though no such version has been released yet in GitHub. That version fails to parse even simple media queries like

@media print
{
	html
	{
		background: white;
		color: black;
	}
}

so I have to revert back to 8.3.0 and apply PHP 8 compatibility fixes manually.

@crisp-tweakers
Copy link
Author

This issue still seems to be present in version 8.5.0 so unfortunately we still can't update to the latest version.

However it is good to see that this library gets some much needed maintenance and care. As far as I know it is the only PHP-based CSS parser/validator available. Keep up the good work, it is highly appreciated!

@JakeQZ
Copy link
Contributor

JakeQZ commented Feb 10, 2024

I can only reproduce this with strict parsing (Settings::beStrict()).

I think the mistake is in 134f4e6 with the change to CSSList.php.

The logic has not been correctly preserverd. When a } is encountered and bIsRoot is false, it should not fail, but merely return null to indicate the end of the list. But now it's throwing an exception if strict parsing is set.

This was picked up by turning on strict parsing in the tests. So maybe we should be running the tests with strict parsing set (actually I think both flavours).

@sabberworm, does my analysis make sense?

@JakeQZ
Copy link
Contributor

JakeQZ commented Feb 10, 2024

@sabberworm, does my analysis make sense?

It seems that this bit of code should not have been changed at all for #162. After refactoring to not throw an exception when it shouldn't, it has the same logic as before, so we may as well change it back, with an additional comment about what returning null means.

JakeQZ added a commit that referenced this issue Feb 10, 2024
The reverts the change to `CSSList` in
134f4e6
and adds a comment that `null` is an expected return value when the end of the list (or block) is reached.

Fixes #352
JakeQZ added a commit that referenced this issue Feb 10, 2024
The reverts the change to `CSSList` in
134f4e6
and adds a comment that `null` is an expected return value when the end of the list (or block) is reached.

Fixes #352
JakeQZ added a commit that referenced this issue Feb 10, 2024
The reverts the change to `CSSList` in
134f4e6
and adds a comment that `null` is an expected return value when the end of
the list (or block) is reached.

It also adds a convention, for now, that a test filename beginning with `=`
should mean that it is initially tested with strict parsing enabled.

Fixes #352
@VaguelyOnline
Copy link

Having the same issue here. The rule it's failing on for us is:

@media only screen and (max-width: 768px) {
.body-signin .content {
margin-top: 120px;
}
}

Should I just hang-tight and do a composer update in the next couple of days?

oliverklee pushed a commit that referenced this issue Feb 13, 2024
The reverts the change to `CSSList` in
134f4e6
and adds a comment that `null` is an expected return value when the end of
the list (or block) is reached.

Fixes #352
@JakeQZ
Copy link
Contributor

JakeQZ commented Feb 13, 2024

Should I just hang-tight and do a composer update in the next couple of days?

I think we are planning to make an 8.5.1 release with this fix soon.

The other option you have is to not use strict parsing. In future major releases we plan to deprecate this and replace it with a logging option.

JakeQZ added a commit that referenced this issue Feb 13, 2024
The reverts the change to `CSSList` in
134f4e6
and adds a comment that `null` is an expected return value when the end of
the list (or block) is reached.

Fixes #352
oliverklee pushed a commit that referenced this issue Feb 14, 2024
The reverts the change to `CSSList` in
134f4e6
and adds a comment that `null` is an expected return value when the end of
the list (or block) is reached.

Fixes #352
@VaguelyOnline
Copy link

I think we are planning to make an 8.5.1 release with this fix soon.

The other option you have is to not use strict parsing. In future major releases we plan to deprecate this and replace it with a logging option.

Thanks for the info. Really appreciate the transparency and the utility of the project. Thanks for the hard work.

@oliverklee
Copy link
Contributor

I've just tagged V8.5.1 and published the release: https://github.com/MyIntervals/PHP-CSS-Parser/releases/tag/v8.5.1

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 a pull request may close this issue.

6 participants