Add sniff and tests for No CDN URL #108

ernilambar · 2017-01-18T08:16:20Z

Based on #64 PR.

Warning message updated.
Maintain coding standard for the files

Fixes #20

grappler · 2017-01-21T15:57:13Z

Thank you for the PR. I had a look why we did not merge #64 yet.

We are working on a Abstract sniff in WPCS that will allow us to define a function e.g. wp_enqueue_script and then check the second parameter. Once we have this this sniff would only be a few lines of code then.
When we use the new sniff it would be easier to whitelist the google fonts URLs then blacklist all of the CDNs.
I don't think we need to check for the CDN URLS for styles and scripts as there will be an error if styles and scripts are hardcoded from WordPress.WP.EnqueuedResources

In the normal case it would make sense to give Kevin props in the commit but if the sniff is completely rewritten then it is not needed.

grappler · 2017-01-21T15:59:04Z

WordPress/Sniffs/Theme/NoCDNSniff.php

+
+				foreach ( $cdn_list as $cdn_url ) {
+					if ( false !== strpos( $match[0], $cdn_url ) ) {
+						$phpcsFile->addError( 'Found the URL to a CDN: (' . $cdn_url . ') The CSS or JavaScript resources cannot be loaded from a CDN but must be bundled.', $stackPtr, 'CDNFound' );


The warning message can contain variables making it a bit cleaner

$phpcsFile->addError( 'Found the URL to a CDN: (%s). The CSS or JavaScript resources cannot be loaded from a CDN but must be bundled.', $stackPtr, 'CDNFound', array( $cdn_url ) );

grappler · 2017-01-30T15:52:33Z

@ernilambar We have merged WPCS into our fork. You can now rebase your PR and extend WordPress_AbstractFunctionParameterSniff instead.

You can have a look how I have done it here. #26

ernilambar · 2017-01-31T09:59:57Z

Looks like I did some mess while merging. I will create new PR for this.

grappler · 2017-01-31T10:00:44Z

@ernilambar We can still fix this...

grappler

This looks great otherwise. Thanks!

grappler · 2017-01-31T10:54:46Z

WordPress/Sniffs/Theme/NoCDNSniff.php

+
+		$matched_parameter = $this->strip_quotes( $parameters[ $position ]['raw'] );
+
+		foreach ( $this->cdn_urls as $cdn ) {


This can be make a bit more performant with if ( isset( $this->cdn_urls[ $matched_parameter ] ) )

Key is like bootstrapcdn.com and $matched_parameter contains full URL like http://maxcdn.bootstrapcdn.com/bootstrap/file.ext. So, I used strpos() check here. Should we parse the $matched_parameter to get domain name only?

Oh, I did not think of that. Parsing the URL will be resource intensive than the foreach. We could check that it is an URL before hand.

if ( false === strpos( $matched_parameter, '.com' ) ) { return; }

We could also stop the foreach once we have a match by adding break;

grappler · 2017-01-31T10:56:34Z

WordPress/Tests/Theme/NoCDNUnitTest.inc

+wp_enqueue_script( 'bootstrap', 'https://code.jquery.com/jquery-whatever/file.js' );  // Bad
+wp_enqueue_script( 'bootstrap', 'https://oss.maxcdn.com/libs/respond.js' );  // Bad
+
+/* ----- links in php comments will not be picked up ----- */


I would add a few google fonts examples here as they they are the exception. If I remember correctly the google font URLs are not always the same. Try to find any many different examples as possible. I am not sure if one of them has googlecode.com in it.

Do we allow Google Fonts URL other than https://fonts.googleapis.com?

It seems not. I am not sure what I had in mind. Could you add it to the test data so that we can make sure in the future it is not marked as an issue?

Google fonts CDN is added in the test.

grappler · 2017-01-31T15:10:46Z

WordPress/Sniffs/Theme/NoCDNSniff.php

+		'maxcdn.com'       => 'maxcdn.com',
+		'jquery.com'       => 'jquery.com',
+		'cdnjs.com'        => 'cdnjs.com',
+		'googlecode.com'   => 'googlecode.com',


Here are few more URLs that we can add
cdnjs.cloudflare.com
ajax.googleapis.com
cdn.jsdelivr.net
use.fontawesome.com
opensource.keycdn.com

ernilambar · 2017-02-15T05:56:27Z

@grappler Anything I need to do from my side here?

grappler · 2017-02-15T06:47:06Z

I would like to get @jrfnl to review it too before we merge it.

jrfnl · 2017-02-15T15:56:34Z

Put it on my list to review. Bear with me. I'm trying to catch up.

jrfnl · 2017-03-05T01:27:55Z

I don't think we need to check for the CDN URLS for styles and scripts as there will be an error if styles and scripts are hardcoded from WordPress.WP.EnqueuedResources

@grappler In general, it is a good idea to have sniffs be completely independent of each other. Now an error which will be identified by one sniff in a way "hides" a following error.

From a theme review process perspective, I can imagine this might be frustrating as the theme author would first get a set of feedback and then once the first set is fixed, they'd get a second set of completely new errors.
In that respect I'd suggest you may want to reconsider sniffing for the CDNs in text strings in general, rather than only limited to the functions now being checked. All the same, you are the better judge of what would work in the theme review process, so it's up to you.

jrfnl

@ernilambar Thanks for your patience. I've added some remarks and questions inline.

Generally: looking good.

One real question I still have which is more a policy one: should the sniff whitelist or blacklist ? or a combination of both ?
Depending on the answer, I might have some more feedback.

jrfnl · 2017-03-05T00:56:49Z

WordPress/Sniffs/Theme/NoCDNSniff.php

+	/**
+	 * Array of functions with positions.
+	 *
+	 * The number represents the position in the function call


Grammer: The number represents the position of the target parameter in the function call

jrfnl · 2017-03-05T00:57:19Z

WordPress/Sniffs/Theme/NoCDNSniff.php

+	 * Array of functions with positions.
+	 *
+	 * The number represents the position in the function call
+	 * passed variables, here the capability is to be listed.


capability ?

jrfnl · 2017-03-05T01:00:32Z

WordPress/Sniffs/Theme/NoCDNSniff.php

+	);
+
+	/**
+	 * Array of CDN URLs.


Is this an array of whitelisted or blacklisted CDNs ? Please expand the short description a little to make this clearer.

You may also want to consider changing the property name to $cdn_url_blacklist or just $cdn_blacklist.

jrfnl · 2017-03-05T01:03:17Z

WordPress/Sniffs/Theme/NoCDNSniff.php

+	 *
+	 * @var array
+	 */
+	protected $cdn_urls = array(


Something is niggling in the back of my mind saying that some CDNs use a number of different URLs including regional ones - so a URL might be bootstrapcdn.de when in Germany. Not sure whether this is actually true or that I'm confusing different things here, but this might need to be investigated and confirmed to be false (and so documented) to be sure.

As I understand it there is normally just one URL but the CDN then chooses which server to load the content from when the receive the request.

jrfnl · 2017-03-05T01:05:40Z

WordPress/Sniffs/Theme/NoCDNSniff.php

+	 *
+	 * @var array
+	 */
+	protected $target_functions = array(


Are CDNs allowed for loading theme images ? If not, these functions do not cover that, so how is this handled ?

This is a good point. I have not seen many themes use external sites to load their images. Normally the reason being that sites do not like hotlinking.

If we wanted to check for this then we would need to check the <img src=""> tag.

I think it is good to check image also. I have seen usage of images from placehold.it directly.

I have no opinion on whether CDNs should be allowed and of is so where, but if you would decide to check text strings like the <img..> tag, I would suggest also checking the <script...> and <style..> tags to prevent the "issues hiding behind other issues".

jrfnl · 2017-03-05T01:08:03Z

WordPress/Sniffs/Theme/NoCDNSniff.php

+		}
+
+		$matched_parameter = $this->strip_quotes( $parameters[ $position ]['raw'] );
+


You may want to check whether the parameter is empty here and if so, return early as further checking would be unnecessary.

jrfnl · 2017-03-05T01:10:00Z

WordPress/Sniffs/Theme/NoCDNSniff.php

+
+		foreach ( $this->cdn_urls as $cdn ) {
+			if ( false !== strpos( $matched_parameter, $cdn ) ) {
+				$this->phpcsFile->addError( 'Loading resources from %s is prohibited.', $stackPtr, $matched_parameter . ' Found', array( $matched_parameter ) );


Please use the string_to_errorcode() method to make sure that the dynamic error code is safe to use.

jrfnl · 2017-03-05T01:11:38Z

WordPress/Sniffs/Theme/NoCDNSniff.php

+	 *
+	 * @var array
+	 */
+	protected $cdn_urls = array(


Is there any logic to the order in which the CDNs are listed here ?

For updating, alphabetical order would be simplest, for efficiency of the sniff, the order of most encountered CDNs would be best (but might need some research to determine).

jrfnl · 2017-03-05T01:16:02Z

WordPress/Sniffs/Theme/NoCDNSniff.php

+	 *
+	 * @var array
+	 */
+	protected $cdn_urls = array(


As the array key isn't used at all, it could be removed.

jrfnl · 2017-03-05T01:18:24Z

WordPress/Tests/Theme/NoCDNUnitTest.inc

@@ -0,0 +1,30 @@
+<?php
+// Now lets check inside php.


Good call on having some examples with both http as well as https. What about other protocols ? Is ftp ever used ? Any other protocols you may have encountered ?

I realize that for the sniff it won't make any difference, but might be good to show that those cases (if they exist) are covered.

I have not seen any other protocols being used.

grappler · 2017-03-07T15:03:10Z

I found two more CDN's npmcdn.com from http://imagesloaded.desandro.com/ and unpkg.com

jrfnl · 2018-05-31T10:34:22Z

@ernilambar Hi Nilambar, how are you ? Are you still interested in finishing off this sniff ? If so, would you be willing to work with me over the next few weeks to get it done ?
If not, would you be ok if I updated the PR ?

Updates needed:

PHPCS 3.x compatibility updates - see Update for PHPCS 3.x and WPCS 0.14.1 #145
Updates based on last review

grappler reviewed Jan 21, 2017

View reviewed changes

grappler added pr: needs update pr: wait for upstream labels Jan 21, 2017

grappler removed the pr: wait for upstream label Jan 30, 2017

ernilambar added 4 commits January 31, 2017 11:03

Add sniff and tests for NO CDN URL

5af0327

Fix Yoda condition

61cb057

Update sniff and related test

dfb75b5

Refactor sniff class

0f7c73d

grappler requested changes Jan 31, 2017

View reviewed changes

grappler reviewed Jan 31, 2017

View reviewed changes

ernilambar added 3 commits February 1, 2017 09:58

Adding more CDN URLs to detect

584c059

Bail if matched content is not URL

7998ec3

Google fonts CDN is allowed

1fee0af

grappler mentioned this pull request Feb 10, 2017

No CDN Allowed #64

Closed

grappler added pr: needs review and removed pr: needs update labels Feb 15, 2017

grappler approved these changes Feb 15, 2017

View reviewed changes

jrfnl self-requested a review February 15, 2017 15:55

jrfnl requested changes Mar 5, 2017

View reviewed changes

grappler added pr: needs update and removed pr: needs review labels Mar 21, 2017

dingo-d mentioned this pull request Jul 28, 2018

[New sniff] Using a CDN is discouraged #20

Open

2 tasks

jrfnl mentioned this pull request Jul 28, 2018

Meeting Notes 2018-07-28 #172

Open

jrfnl closed this Sep 23, 2018


		$matched_parameter = $this->strip_quotes( $parameters[ $position ]['raw'] );

		foreach ( $this->cdn_urls as $cdn ) {

Add sniff and tests for No CDN URL #108

Add sniff and tests for No CDN URL #108

Conversation

ernilambar commented Jan 18, 2017 • edited by jrfnl Loading

grappler commented Jan 21, 2017

Choose a reason for hiding this comment

grappler commented Jan 30, 2017

ernilambar commented Jan 31, 2017

grappler commented Jan 31, 2017

grappler left a comment

Choose a reason for hiding this comment

grappler Jan 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ernilambar commented Feb 15, 2017

grappler commented Feb 15, 2017

jrfnl commented Feb 15, 2017

jrfnl commented Mar 5, 2017

jrfnl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl Mar 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grappler commented Mar 7, 2017

jrfnl commented May 31, 2018

ernilambar commented Jan 18, 2017 •

edited by jrfnl

Loading

grappler Jan 31, 2017 •

edited

Loading

jrfnl Mar 5, 2017 •

edited

Loading