Open carolinan opened 8 years ago
This is the rule as stated in the handbook:
Include all scripts and resources it uses rather than hot-linking. The exception to this is Google Fonts.
Maybe the title of this issue should be adjusted ? CDNs can also be used for (background/header) images and AFAICS the handbook does not (currently) explicitly prohibit that.
Another handbook ref for this: https://make.wordpress.org/themes/handbook/review/required/theme-check-plugin/#cdn
Lastly, I'd like to see some discussion on whether the current list of CDNs which are checked for is sufficient or if more CDNs should be added and if so, which.
https://cdnjs.com/ should be on there.
I think this can be an ERROR. The only resources that can be hot linked are Google Fonts. Nothing else is allowed to my knowledge.
I have a sniff ready for this.
Hi! Do we have a decision on what CDN urls should be allowed? We should have a definite list so that we can update the #108 PR.
We should brought this up in a meeting, or if we already have this list we can add it here or in the PR so that it's merged.
None except google fonts. The requirement says: Include all scripts and resources. https://make.wordpress.org/themes/handbook/review/required/#stylesheets-and-scripts
Like Kevin says this should be an error not a warning. And we should avoid using the word "discouraged" when it is not allowed.
Then we should remove the scripts that are in the whitelist section of this PR and only allow google fonts.
I'm updating the PR at this moment. For the implementation of this, I have some additional questions to verify if I'm understanding this correctly and whether the sniff does what it's supposed to do:
wp_register_style()
or wp_enqueue_style()
. Never as a script.
If that's correct, any URL in the $source
of a script register/enqueue should always throw an error
.
For styles, the whitelist would be checked and if not Google fonts, an error
will be thrown.$source
is a variable ? Throw a warning
? Try and walk the scope within which it is found to find the variable declaration ?@joyously
No, it was not. If you look at the PR in #108, you can see that it used a blacklist and would only throw an error if something on that list was encountered.
Blacklist approaches tend to underreport (miss things which should trigger an error), whitelist approaches tend to overreport (throw errors for things which are fine).
A whitelist approach for this sniff is going to be interesting, but complicated to implement as the $src
can also be a local file and we'd want to minimize false positives. If the $src
, however, is passed as a variable, constant or via a nested function call, it will be very difficult in most cases to reliably determine whether a local file or a URL is being passed to the function call.
(and 4) In that case, I need a whole lot of code samples of things which are forbidden to properly determine what to sniff for.
So, please, everyone who wants this sniff to actually be finished and merged, please leave comments with code samples of things which are not allowed.
complicated to implement as the
$src
can also be a local file
That's the point. Everything should be a local file, except Google Fonts.
That's the point. Everything should be a local file
I know, but that doesn't mean that it is sniffable.
Well, it's either a variable or one of the get_template_*
functions or Google. What else do you need?
I suppose there could be a constant also, but that is discouraged.
This gist contains: Things we don't allow + some CSS, HTML and JS question marks
TLDR: https://stackpath.bootstrapcdn.com https://maxcdn.bootstrapcdn.com/ https://cdnjs.cloudflare.com/ajax/libs/ https://use.fontawesome.com/ https://ajax.googleapis.com/ajax/libs/jquery/ //code.jquery.com/ui/
and one new addition: db.onlinewebfonts.com/
That looks similar to the PR I put in for Theme Check. (not merged yet) https://github.com/WordPress/theme-check/pull/208/commits/eec60daadc010dfb888c0075783b2d912f331b78
use.typekit.net also needs to be blacklisted since we do not allow it and because of licensing.
And here is the gist with things that we do allow. https://gist.github.com/carolinan/7c2b002465b260ab3a74ec2d9b924c8c
Since we have a list of things to allow, and disallow, I'll remove the decision needed tag, so that we know this can be worked on.
Rule:
WARNING : Using a CDN is discouraged. All JS and CSS should be bundled.
Ref: https://make.wordpress.org/themes/handbook/review/required/#stylesheets-and-scripts
Theme check file covering this rule:
https://github.com/Otto42/theme-check/blob/master/checks/cdn.php
To do: