Automattic / VIP-Coding-Standards

PHP_CodeSniffer ruleset to enforce WordPress VIP coding standards.
https://wpvip.com/documentation/how-to-install-php-code-sniffer-for-wordpress-com-vip/
Other
237 stars 40 forks source link

Flag redundant font file types #700

Closed rebeccahum closed 3 years ago

rebeccahum commented 3 years ago

Describe the solution you'd like

As per https://caniuse.com/?search=woff, .woff and .woff2are the only custom font file types that are needed now, so file types of .eot, .ttf, and .otf should be flagged as not being needed.

props @GaryJones

This might be do-able with getFilename(). @jrfnl thoughts?

jrfnl commented 3 years ago

@rebeccahum I don't think PHPCS is the tool to use for a check like that.

PHPCS only processes files which comply with the file extensions registered. Typically, those are php, inc, css and js. Registering eot etc as file types for PHPCS to scan is a bad idea as PHPCS will then try to tokenize those files, which will cause no end of problems.

You could, of course, create a sniff which will execute a system command to try and find those files anyway, but that sniff would still need to be triggered somehow.

The only thing I can think of for this would be a sniff which would scan inline HTML in PHP files and scan CSS files for the registration of those redundant file types. A sniff like that would need plenty of code samples to test against and will probably still not catch everything and may throw false positives too.

What do you want to achieve with this check ? As far as I can see, nothing will break if those extra files are there and registered. They are just redundant.

If you are trying to inform/educate dev-users, wouldn't a blogpost be better ?

rebeccahum commented 3 years ago

If we added the check as a sniff into VIPCS, then it could also be run locally to capture previously added old types

This will help to keep the repo smaller.

Based on internal discussions, I believe @GaryJones wants to use it to keep the repositories smaller. In case I'm missing something, can you please speak more about this, @GaryJones?

Thanks for the feedback — yep, it does make sense that this is not the right tool for it since VIPCS is not a catch-all for issues like this. I wasn't 100% sure if other file types could be tokenized as well, but wanted your input in case I was missing a learning opportunity 🙂 .

jrfnl commented 3 years ago

Thanks for the feedback — yep, it does make sense that this is not the right tool for it since VIPCS is not a catch-all for issues like this. I wasn't 100% sure if other file types could be tokenized as well, but wanted your input in case I was missing a learning opportunity 🙂 .

No worries and while we're talking learning:

Your question does make me wonder about whether a potential efficiency tweak has been applied (yet) to PHPCS.

I presume PHPCS checks whether there are sniffs registered for a file type before tokenizing and skips otherwise, as Greg is generally good about things like that, but would be good to be sure. When I have some time, I may want to investigate that.

GaryJones commented 3 years ago

I believe @GaryJones wants to use it to keep the repositories smaller.

Correct, but as per Juliette's explanations, the focus on the file extensions, rather than the rest of the file name, means PHPCS probably isn't the tool for this. The check could be added direct to the vip-go-ci bot though I imagine.