search

WPTT / theme-sniffer

Theme Sniffer plugin using sniffs.
MIT License
270 stars 3 forks source link

No PHP code found - short open tags #140

Open Mocha365 opened 5 years ago

Mocha365 commented 5 years ago

When doing a theme check with the latest 1.0.0 version of Theme Sniffer, I am getting these warnings for every stylesheet:

WARNING No PHP code was found in this file and short open tags are not allowed by this install of PHP

Is this a bug or something else?

timelsass commented 5 years ago

No, you're correct it shouldn't be doing that, it looks like in testing this out it happens on .js files as well. In the mean time if you wish to suppress those warnings you can check the box "Check Only PHP Files." Thanks for letting us know!

Mocha365 commented 5 years ago

Ah, didn't think it was supposed to do that :) I actually put back in my previous NS Theme Check Version 0.1.4 for the time being. The new one also seems to ignore things like // WPCS: XSS OK ....also checked default WP themes too for testing. But I can bring up the other things in another ticket; I have some more testing to do with the new one.

Cheers!

timelsass commented 5 years ago

It looks like this can be closed as it deals with the upstream WPTRT/WPThemeReview , I went ahead and pushed a fix for it there, so the next release should get it squared away!

In terms of the // WPCS: XSS OK - this was syntax for an older version of WPCS - which has been deprecated in version 2.0.0, and version 3.0.0 will be removed. For ignoring it's recommended to use the standard // phpcs:ignore ruleset syntax like: // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped to prevent specific warnings/errors as needed.

This repo is using 1.2.1 of WPCS as of now, so the // WPCS: CSS OK should work as expected if you check the "Ignore Annotations" box. This was added so people don't just ignore everything and a reviewer for theme submission isn't able to see errors/notices. Let me know if that works for you :smile:

Mocha365 commented 5 years ago

Awesome! ...and thanks.

timelsass commented 5 years ago

No problem!

Just as some insight into this issue from what I've seen so far - for others who might look at this: It seems like something is happening internally in the sniff process. As a partial resolution to this, the PR https://github.com/WPTRT/theme-sniffer/pull/142 adds esprima JS syntax checking. Given that, we could probably offload all .js file checks to Esprima, which would leave phpcs to only run css and php checks. The next step would be to look at implementing stylelint or csslint to provide similar functionality if the same error outputs.

I did go through testing out a mostly unmodified Runner class in place of our custom runner. In doing this it does appear that PHPCS was detecting and skipping the minfied files it couldn't handle when they were ran through the Files\FileList class, and the PHP code/short open tags error no longer appears, and the report notes files that were skipped. It's a relatively large repo filewise, and I did notice files that weren't minified which were skipped (possibly due to size?). I see a few benefits to using Esprima - it's meant for JS, it catches actual issues that could cause errors in the scripts execution. PHPCS is just tokenizing and running sniffs, but comparing the same file through each sniffing it - Esprima catches real syntax errors, and I have yet to see anything relevant from PHPCS - other than that it skipped a file that it couldn't tokenize for no specific reason given.