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

Duplicate class definition detection generates false-positives in media queries #985

Closed
h3xx opened this issue Apr 28, 2016 · 5 comments
Closed

Comments

@h3xx
Copy link

h3xx commented Apr 28, 2016

Steps to reproduce

Invoke phpcs on the test code:

phpcs --standard=Squiz -s --sniffs=Squiz.CSS.DuplicateClassDefinition [FILE]

Test code

.foo { max-width: 100%; }
@media (max-width:1178px) {
    .bar { border: none; }
    .foo { width: auto; }
}

Produces output

FILE: test.css
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 4 | ERROR | Duplicate class definition found; first defined on line
   |       | 1 (Squiz.CSS.DuplicateClassDefinition.Found)
----------------------------------------------------------------------

Time: 59ms; Memory: 2.5Mb

Expected output

No output


Notes:

The following code does NOT produce any errors even though it's functionally equivalent to the test code above. All I did was swap line 3 and line 4.

.foo { max-width: 100%; }
@media (max-width:1178px) {
    .foo { width: auto; }
    .bar { border: none; }
}

This leads me to believe that the error lies in how @media() {...} is being processed; it seems to think the @media block ends at the first } it sees, OR it doesn't consider a @media block as a separate CSS document.

Found in phpcs version f4e18d4 (2016-04-27)

@h3xx
Copy link
Author

h3xx commented May 31, 2016

Tried again in 87efecd (2016-05-31). Bug still exists exactly as reported.

@gsherwood
Copy link
Member

Bug still exists exactly as reported.

That would be because I haven't fixed it :)

I am 100% certain that this isn't going to be fixed by accident due to where the code is. So unless I update this bug report with a fix, you should consider it unfixed.

I don't know when I will get time to look at it, but I did briefly look into it when I tagged it and it looked tricky at the time.

@rhorber
Copy link
Contributor

rhorber commented Aug 25, 2016

I noticed this too long ago, but it never occured to me to fix it...
After seeing that gsherwood has no time, I took the liberty and created a bugfix: #1131

@rhorber
Copy link
Contributor

rhorber commented Aug 25, 2016

FYI: The first class definition in a media query also contained the media query itself in the name,. Thus no error was generated for the first position, but the others.
That is also the reason why I added T_OPEN_CURLY_BRACKET to the find array.

@gsherwood
Copy link
Member

gsherwood commented Aug 25, 2016

Fixed by PR #1131

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants