squizlabs / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
10.67k stars 1.48k forks source link

Erroneous detection on based on the average length of lines #877

Closed ocnk closed 8 years ago

ocnk commented 8 years ago

We have warning «File appears to be minified and cannot be processed» in file template with inline image. e.g. :

<footer class="msg-footer msg-footer_anonym">
    <div class="msg-footer__text">
        ...
    </div>
    <span class="msg-footer__logo">
        <img src="data:image/png;base64,..." alt="">
    </span>
</footer>

Unfortunately, this check is not configured. It may be should add option to disable this check by file type

gsherwood commented 8 years ago

That check only runs when the file is not being processed as a PHP file, so it only applies to the JS and CSS tokenizers.

When I run my own test on a .php or .html file (both process by PHPCS as PHP files) I end up with an error report like this:

FILE: temp.php
-----------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
-----------------------------------------------------------------------
  2 | ERROR   | Missing file doc comment
 10 | WARNING | Line exceeds 85 characters; contains 762053 characters
-----------------------------------------------------------------------

What type of file are you checking? Have you told PHPCS to check it as a JS or CSS file?

ocnk commented 8 years ago

We use Blitz template engine (http://alexeyrybak.com/blitz/blitz_en.html) and have custom tokinazer PHP_CodeSniffer_Tokenizers_TPL. The files have the type .tpl

Sorry for not writing this sooner

gsherwood commented 8 years ago

have custom tokinazer PHP_CodeSniffer_Tokenizers_TPL

That explains things then.

The only way I can support a custom tokenizer with this feature is to get you to either set a member var I can check (something like $checkForMinifiedFiles) or override a method that does the actual minification check (something like isMinifiedFile()) and just return false as the PHP one would.

Either way, you'll need to make a minor modification to your tokenizer when this change is made, and after you've upgraded.

ocnk commented 8 years ago

The ideal solution would be to move the check in tokenayzer

gsherwood commented 8 years ago

The ideal solution would be to move the check in tokenayzer

Doing that duplicates the minification detection logic in both the CSS and JS tokenizers whereas including it where it is now allows it to remain in a single location. So the most likely change is to put in a member var that can be checked so the tokenizer can decide if it wants to check files that appear to be minified.

gsherwood commented 8 years ago

They way I did this was to check a $skipMinified member var in the tokenier. If it is set to TRUE, file that appear to be minified will be skipped. If there isn't a member var defined, or it is set to FALSE, all files will be processed.

So for your specific case, you wont have to change anything in your tokenizer. I thought it would be better to change the default behaviour of custom tokenizers to process all files.

I will be implementing this differently in the 3.0 branch because I have a proper base class there.

gsherwood commented 8 years ago

The way I've done this in the 3.0 branch is to put in a new isMinifiedContent() method in the base Tokenizer class and have the JS and CSS constructors use that. If a custom tokenizer wants to use it as well, it is there for them. But the default functionality is to check all files (3.0 actually introduced this already).