-
Notifications
You must be signed in to change notification settings - Fork 38
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
[New sniff] Using a CDN is discouraged #20
Comments
This is the rule as stated in the handbook:
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: 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. Like Kevin says this should be an error not a warning. |
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:
|
|
|
That's the point. Everything should be a local file, except Google Fonts. |
I know, but that doesn't mean that it is sniffable. |
Well, it's either a variable or one of the |
Actions needed
|
This gist contains: Things we don't allow + some CSS, HTML and JS question marks TLDR: and one new addition: |
That looks similar to the PR I put in for Theme Check. (not merged yet) |
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:
The text was updated successfully, but these errors were encountered: