Added Enqueued functions check for version and in footer #1233

ghost · 2017-11-20T13:18:48Z

I have created a custom Sniff based from:

This checks that IF source ($src) if specified then it will check to make sure there is a Version ($ver), and it will error if not.

Also added another check for In Footer ($in_footer) to warn the user that no value is set.

dingo-d · 2017-11-20T13:23:13Z

WordPress/Sniffs/PHP/FunctionEnqueuedSniff.php

+		'wp_enqueue_script'  => true,
+		'wp_register_style'  => true,
+		'wp_enqueue_style'   => true,
+		'wp_register_style'  => true


Why do you have this two times?

Also looks like it was accidentally duplicated in the class docblock.

That would be a duplication error, I shall remove that

JDGrimes

Thanks for the PR! There are just a few things that need to be cleaned up here (see my comments). The sniff also doesn't follow WPCS's code style, which is one reason the Travis build is failing. That will also need to be corrected.

I would also like to see some unit tests added before this is merged. Would you like to try adding some (you can look at the tests for the other sniffs as an example)?

JDGrimes · 2017-11-20T13:28:59Z

WordPress/Sniffs/PHP/FunctionEnqueuedSniff.php

+		'wp_enqueue_script'  => true,
+		'wp_register_style'  => true,
+		'wp_enqueue_style'   => true,
+		'wp_register_style'  => true


Also looks like it was accidentally duplicated in the class docblock.

JDGrimes · 2017-11-20T13:30:48Z

WordPress/Sniffs/PHP/FunctionEnqueuedSniff.php

+ * wp_enqueue_script
+ * wp_register_style
+ * wp_enqueue_style
+ * wp_register_style


Looks like you accidentally listed wp_register_style() twice here too.

JDGrimes · 2017-11-20T13:33:00Z

WordPress/Sniffs/PHP/FunctionEnqueuedSniff.php

+ *
+ * @package WPCS\WordPressCodingStandards
+ *
+ * @since   0.14.0 This new sniff will check for a version in an Enqueued WP function


All of the @since version numbers should probably be updated to 0.15.0 (presumably the next version that will be released).

JDGrimes · 2017-11-20T13:35:18Z

WordPress/Sniffs/PHP/FunctionEnqueuedSniff.php

+
+		// Check to see IF a source ($src) is specified otherwise its fine
+		if(isset($parameters[2])) {
+			/**


Since this isn't a docblock for an element, the second * should probably be left off.

JDGrimes · 2017-11-20T13:35:29Z

WordPress/Sniffs/PHP/FunctionEnqueuedSniff.php

+				return;
+			}
+
+			/**


Same as above.

JDGrimes · 2017-11-20T13:37:13Z

WordPress/Sniffs/PHP/FunctionEnqueuedSniff.php

+				$errorcode = 'MissingVersion';
+
+				$this->phpcsFile->addError(
+					'No Version found for %s; Please supply value for forth argument',


I'd change the last part to "Please supply a value for the forth argument"

JDGrimes · 2017-11-20T13:41:52Z

WordPress/Sniffs/WP/EnqueuedCheckSniff.php

@@ -0,0 +1,119 @@
+<?php


Looks like you've committed the same sniff twice with two different names. (Or maybe I am missing something?)

Perhaps EnqueuedCheck is a better name than EnqueuedVersion, since the sniff also checks the $in_footer arg.

Does anybody else have an opinion?

@JDGrimes Thanks for checking the code :)

Yes I added this one in as EnqueuedCheck seemed better than EnqueuedVersion hence why I removed the EnqueuedResourcesSniff.php file. Sorry about that

JDGrimes · 2017-11-20T13:43:32Z

WordPress/Sniffs/WP/EnqueuedResourcesSniff.php

@@ -1,65 +0,0 @@
-<?php


I'm not sure you intended to remove this sniff. If you did, could you please explain why? I think that it needs to stay, because it checks to make sure that the wp_enqueue_*() functions are actually used, instead of hard-coded script/style tags.

Based on the presence of a seemingly duplicate sniff, and the seemingly accidental removal of this one, I think it was a simple mistake of deleting the wrong file.

Good point; that's easy to do. 😄

jrfnl

@snightingale Welcome & congrats on your first PR to WPCS.

Don't be alarmed by the amount of comments you will receive. Any new sniff is generally reviewed very critically here in an effort to keep the quality of WPCS high and to teach contributors about writing sniffs. We're here to help.

Having said that, I agree with @JDGrimes that unit tests are needed.
I've also left some comments inline.
Please also review all inline comments and docblocks in the sniff for correct capitalization and not having duplicate info.

jrfnl · 2017-11-20T13:59:09Z

WordPress/Sniffs/PHP/FunctionEnqueuedSniff.php

+	 * @since 0.11.0
+	 * @var string
+	 */
+	protected $group_name = 'strict';


Copy/paste artifact - please update for this sniff.

jrfnl · 2017-11-20T13:59:42Z

WordPress/Sniffs/PHP/FunctionEnqueuedSniff.php

+	protected $group_name = 'strict';
+
+	/**
+	 * List of array functions to which a $strict parameter can be passed.


Copy/paste - please update the documentation to be applicable for this sniff.

jrfnl · 2017-11-20T14:01:00Z

WordPress/Sniffs/PHP/FunctionEnqueuedSniff.php

+	 * @return void
+	 */
+	public function process_parameters($stackPtr, $group_name, $matched_content, $parameters) {
+		// Check if the strict check is actually needed.


Copy/paste artifact - is this condition valid the way it is for this sniff ? If so, please update the comment. Otherwise remove.

jrfnl · 2017-11-20T14:05:09Z

WordPress/Sniffs/PHP/FunctionEnqueuedSniff.php

+			}
+		}
+
+		// Check to see IF a source ($src) is specified otherwise its fine


its => it's

This check could be reversed to reduce nesting level:

if ( ! isset( $parameters[2] ) ) { return; }

jrfnl · 2017-11-20T14:05:55Z

WordPress/Sniffs/PHP/FunctionEnqueuedSniff.php

+			 * Otherwise it will Show a warning to add a Src (url) to the Enqueued function
+			 */
+			if (false === isset($parameters[4]) || !$parameters[4]['raw']) {
+				$errorcode = 'MissingVersion';


No need for a variable, just set this in the function call.

jrfnl · 2017-11-20T14:06:47Z

WordPress/Sniffs/PHP/FunctionEnqueuedSniff.php

+			if (false === isset($parameters[4]) || !$parameters[4]['raw']) {
+				$errorcode = 'MissingVersion';
+
+				$this->phpcsFile->addError(


The documentation a few lines above, says it will throw a warning, so why is an error being thrown ?

jrfnl · 2017-11-20T14:07:13Z

WordPress/Sniffs/PHP/FunctionEnqueuedSniff.php

+			 * Version Check
+			 *
+			 * Check to make sure a Version ($ver) is set
+			 * Otherwise it will Show a warning to add a Src (url) to the Enqueued function


The documentation does not seem to be in line with the functionality.

jrfnl · 2017-11-20T14:08:11Z

WordPress/Sniffs/PHP/FunctionEnqueuedSniff.php

+
+				$this->phpcsFile->addError(
+					'No Version found for %s; Please supply value for forth argument',
+					(isset( $parameters[4]['start']) ? $parameters[4]['start'] : $parameters[1]['start']),


Why not simply use the $stackPtr ?

jrfnl · 2017-11-20T14:10:54Z

WordPress/Sniffs/PHP/FunctionEnqueuedSniff.php

+			 * Check to make sure that $in_footer is set to true
+			 * Otherwise it will warn the user to make sure if its correct
+			 */
+			if (false === isset($parameters[5]) || 'true' !== $parameters[5]['raw']) {


Same comments as above for this whole snippet + I'm wondering whether if false is supplied as an explicit argument, the warning should still be thrown. In that case, it's a conscious choice of the dev to not enqueue in the footer.

JDGrimes · 2017-11-22T16:34:13Z

@snightingale I think the remaining failures on Travis may possibly be unrelated. So don't worry about them right now. It may be a little while before we merge any new sniffs anyway, because the next release may end up being focused just on re-organizing some existing things. So hopefully whatever is causing this will get fixed in the mean time.

JDGrimes

This is getting closer, just a few more things I spotted.

JDGrimes · 2017-11-22T16:35:45Z

WordPress/Sniffs/PHP/EnqueuedCheckSniff.php

+ * wp_register_script
+ * wp_enqueue_script
+ * wp_register_style
+ * wp_enqueue_style


I guess maybe this should be rewritten as a list:

* - wp_register_script() * - wp_enqueue_script() * - ...

JDGrimes · 2017-11-22T16:36:22Z

WordPress/Sniffs/PHP/EnqueuedCheckSniff.php

+	 * @since 0.11.0 Renamed from $array_functions to $target_functions.
+	 *
+	 * @var array <string function_name> => <bool always needed ?>
+	 */


This docblock can be removed now that the property it was for has been removed.

JDGrimes · 2017-11-22T16:37:32Z

WordPress/Sniffs/PHP/EnqueuedCheckSniff.php

+		 * Check to make sure that $in_footer is set to true
+		 * Otherwise it will warn the user to make sure if its correct
+		 */
+		// print_r($parameters);


This debug code can be removed. 😃

JDGrimes · 2017-11-22T16:41:25Z

Actually, a few of the Travis issues might actually be related to this sniff: https://travis-ci.org/WordPress-Coding-Standards/WordPress-Coding-Standards/jobs/305880198#L780

But I think some of the others are not.

jrfnl

Definitely going in the right direction.

The code-style needs some attention still to comply with WPCS itself:
https://travis-ci.org/WordPress-Coding-Standards/WordPress-Coding-Standards/jobs/305880198#L780-L831
https://travis-ci.org/WordPress-Coding-Standards/WordPress-Coding-Standards/jobs/305880198#L903-L905

~~I suspect that when these things are fixed, the other errors will magically disappear. (or not, but in that case, you can leave them to me to have a look at)~~

~~Reminder to self: look into the fixer conflict being flagged.~~

jrfnl · 2017-11-22T20:10:24Z

WordPress/Sniffs/PHP/EnqueuedCheckSniff.php

+	/**
+	 * The group name for this group of functions.
+	 *
+	 * @since 0.11.0


Outdated @since tag. This is a new sniff, so the @since should indicate the version the sniff will be merged into, most likely ~~0.16.0~~ 1.0.0 (see @JDGrimes's comment about the 0.15.0 release)

jrfnl · 2017-11-22T20:11:06Z

WordPress/Sniffs/PHP/EnqueuedCheckSniff.php

+	 * @link https://developer.wordpress.org/reference/functions/wp_enqueue_style/
+	 *
+	 * @since 0.10.0
+	 * @since 0.11.0 Renamed from $array_functions to $target_functions.


Copy/paste issue - please update @since tag

jrfnl · 2017-11-22T20:12:15Z

WordPress/Sniffs/PHP/EnqueuedCheckSniff.php

+
+	/**
+	 * List of Enqueued functions that need to be check to make sure
+	 * IF a source ($src) value is passed, then version ($ver) needs to have a value.


This line and the two below it should be part of the class docblock. They are not directly related to this parameter so this info shouldn't be placed here.

jrfnl · 2017-11-22T20:12:28Z

WordPress/Sniffs/PHP/EnqueuedCheckSniff.php

+	/**
+	 * Process the parameters of a matched function.
+	 *
+	 * @since 0.11.0


Copy/paste issue - please update @since tag

jrfnl · 2017-11-22T20:13:17Z

WordPress/Tests/PHP/EnqueuedCheckUnitTest.php

+ * @package WPCS\WordPressCodingStandards
+ *
+ * @since   0.9.0
+ * @since   0.13.0 Class name changed: this class is now namespaced.


Copy/paste issue - please update @since tag

jrfnl · 2017-11-22T20:13:45Z

WordPress/Tests/PHP/EnqueuedCheckUnitTest.php

+use PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest;
+
+/**
+ * Unit test class for the FunctionEnqueued sniff.


Copy/paste issue - please update the sniff name

jrfnl · 2017-11-22T20:20:24Z

WordPress/Sniffs/PHP/EnqueuedCheckSniff.php

+		 * Otherwise it will warn the user to make sure if its correct
+		 */
+		// print_r($parameters);
+		if ( isset( $parameters[5] ) ) {


The sniff will now not throw an error if the $in_footer param is missing, which I believe it should.

If an explicit true or false is given, IMHO, the sniff should not report on it at all as that's a conscious developer choice.
If at all, it should be a warning for any non-true value, but with a separate errorcode for a false value so that particular warning can be silenced easily.

jrfnl · 2017-11-22T20:22:08Z

WordPress/Tests/PHP/EnqueuedCheckUnitTest.inc

+
+wp_register_script( 'someScript-js', 'https://domain.com/someScript.js' , array( 'jquery' ), '1.1.1', true ); // Ok.
+wp_register_script( 'someScript-js', 'https://domain.com/someScript.js' , array( 'jquery' ), '1.1.1', false ); // Warning.
+wp_register_script( 'someScript-js', 'https://domain.com/someScript.js' , array( 'jquery' ) ); // Error.


I think the following two cases should be added:

wp_register_script( 'someScript-js', 'https://domain.com/someScript.js' , array( 'jquery' ), '1.1.1' ); // Error - missing $in_footer. wp_register_script( 'someScript-js', 'https://domain.com/someScript.js' , array( 'jquery' ), '1.1.1', $in_footer ); // Warning - $in_footer is not a boolean.

jrfnl · 2017-11-22T20:27:39Z

Last point: please look into the grammar of both the messages being thrown as well as the grammar of the text in the docblocks. IF => If, and other capitalization errors are the most notable issues.

jrfnl · 2017-11-23T23:13:27Z

@snightingale Could you rebase the PR against the current develop branch ? That way the unrelated build failure will no longer trouble you. The codestyle issues in this PR will still need to be fixed though.

jrfnl · 2018-03-01T07:40:58Z

@snightingale Just checking: Do you have any intention of updating this PR based on the above feedback or should we close the PR or open it up to one of the WPCS maintainer to update the PR ?

darthvader666uk · 2018-03-01T10:05:13Z

@jrfnl (have to use this account for now) Yes, I managed to rebase this branch from Master changes and I have a spare time this weekend to have a look at the requested changes. So sorry fo the delay :)

jrfnl · 2018-03-01T10:08:39Z

@darthvader666uk No worries, just glad to hear you're still interested in continuing work on this.

darthvader666uk · 2018-03-08T09:14:21Z

@jrfnl I have now tweaked the code from your comments / travis fails and it appears to be ok. Let me know if you see any further issues

GaryJones · 2018-03-08T09:14:24Z

WordPress/Sniffs/PHP/EnqueuedCheckSniff.php

+use WordPress\AbstractFunctionParameterSniff;
+
+/**
+ * Based off the StrictInArraySniff.php Sniff, THis checks the Enqueued 4th Parameter to make sure a Version is available.


Several unnecessary uppercase characters here.

Also, the description doesn't represent all the checks that happen here.

GaryJones · 2018-03-08T09:15:34Z

WordPress/Sniffs/PHP/EnqueuedCheckSniff.php

+/**
+ * Based off the StrictInArraySniff.php Sniff, THis checks the Enqueued 4th Parameter to make sure a Version is available.
+ * The Enqueued functions are:
+ * wp_register_script()


Use:

* - `wp_register_script()`

for each item here, so that extracted docs will display it as a list, with code markup.

GaryJones · 2018-03-08T09:16:38Z

WordPress/Sniffs/PHP/EnqueuedCheckSniff.php

+ * wp_enqueue_script()
+ * wp_register_style()
+ * wp_enqueue_style()
+ * IF a source ($src) value is passed, then version ($ver) needs to have a value.


Leave a line space under the list, and no need for uppercase IF or other random capitals.

GaryJones · 2018-03-08T09:18:12Z

WordPress/Sniffs/PHP/EnqueuedCheckSniff.php

+	/**
+	 * List of Enqueued functions that need to be check to make sure
+	 *
+	 * @link https://developer.wordpress.org/reference/functions/wp_register_script/


Include the protocol if using URLs here.

GaryJones · 2018-03-08T09:22:13Z

WordPress/Sniffs/PHP/EnqueuedCheckSniff.php

+		 * Version Check
+		 *
+		 * Check to make sure a Version ($ver) is set
+		 * Otherwise it will Show an error to add a Src (url) to the Enqueued function


This comment is incorrect, but the whole comment block is redundant anyway.

// Check version is set.

would be sufficient.

Strongly consider moving each check to its own protected/private method, and document that instead, and then add calls to those new methods in this method.

GaryJones · 2018-03-08T09:22:46Z

WordPress/Sniffs/PHP/EnqueuedCheckSniff.php

+use WordPress\AbstractFunctionParameterSniff;
+
+/**
+ * Based off the StrictInArraySniff.php Sniff, THis checks the Enqueued 4th Parameter to make sure a Version is available.


Also, the description doesn't represent all the checks that happen here.

GaryJones · 2018-03-08T09:24:56Z

WordPress/Sniffs/PHP/EnqueuedCheckSniff.php

+				case 'wp_register_script':
+				case 'wp_enqueue_script':
+					if ( 'true' !== $parameters[5]['raw'] ) {
+						$this->phpcsFile->addWarning( 'If the Footer is not set to True for %s; Double check if correct or set to True', $stackPtr, 'MissingInFooter', array( $matched_content ) );


If is not needed here.

"The $in_footer is not set to true. ..."

Hi @GaryJones With the IF removed, all checks fail. THis check needs to be here in order to get the warning. Unless I missed something?

@darthvader666uk I think @GaryJones's remark was not about the conditional, but about the phrasing of the error message.

For that matter, mind capitalizing words in the error message when it's not needed.

GaryJones · 2018-03-08T09:25:39Z

WordPress/Tests/PHP/EnqueuedCheckUnitTest.inc

+wp_register_style( 'script-name', 'https://domain.com/someScript.js' ); // Error.
+
+wp_enqueue_style( 'script-name', 'https://domain.com/someScript.js', false, '1.0.0'); // Ok.
+wp_enqueue_style( 'script-name', 'https://domain.com/someScript.js' ); // Error.


Clear line space needed at the end.

GaryJones · 2018-03-08T09:26:28Z

WordPress/Tests/PHP/EnqueuedCheckUnitTest.inc

@@ -0,0 +1,15 @@
+<?php
+


What about cases for a couple of the functions that have a version but no in_footer param?

GaryJones · 2018-03-08T09:34:40Z

Note that between https://core.trac.wordpress.org/ticket/12009 and https://core.trac.wordpress.org/ticket/22249 the parameters for the functions covered here may change.

This adds the changelog for the 0.14.1 which can be tagged straight after merging this PR.

jrfnl · 2018-06-11T21:35:54Z

@snightingale @darthvader666uk Hiya, just checking in again to see how work is progressing. If you haven't got time to finish this, please let us know and we'll see if we can find someone to take over.

darthvader666uk · 2018-06-12T08:25:58Z

@jrfnl I do appologies for the crazy late reply to this issue. However, with work commitments I am unable to proceed with the fix. If you have someone else ready to take over, I think that would be for the best :)

jrfnl · 2018-06-12T21:50:10Z

@darthvader666uk Thanks for your response! I understand and I'll see if we can find someone to take over.

moorscode · 2018-06-14T14:10:04Z

PR #1378 supersedes this one.

jrfnl · 2018-07-12T20:36:26Z

@darthvader666uk Thanks for initiating this sniff and your initial work on it. The continuation of your work by @moorscode has now been merged 🎉

dingo-d reviewed Nov 20, 2017

View reviewed changes

JDGrimes suggested changes Nov 20, 2017

View reviewed changes

jrfnl requested changes Nov 20, 2017

View reviewed changes

JDGrimes suggested changes Nov 22, 2017

View reviewed changes

jrfnl requested changes Nov 22, 2017

View reviewed changes

jrfnl mentioned this pull request Nov 22, 2017

Update default minimum WP version #1237

Merged

darthvader666uk mentioned this pull request Feb 19, 2018

Master #1303

Closed

GaryJones reviewed Mar 8, 2018

View reviewed changes

GaryJones requested changes Mar 8, 2018

View reviewed changes

jrfnl and others added 12 commits March 15, 2018 09:58

Release 0.14.1

b227b62

This adds the changelog for the 0.14.1 which can be tagged straight after merging this PR.

Added EnqueuedCheck Sniff and Unit tests

9a75764

updated a few points from pull request

42172bd

updated sniff name

938cc1b

rearranged the test

d2a3fe5

tweaked the error

f32f7b2

ci updates

76c4f99

more ci updates

9e5549b

updated unnecessary uppercase characters

4f9b4a0

updated code

baeb336

removed check

3aea768

check removal didnt work

9b4c876

Stuart Nigtingale added 2 commits March 15, 2018 10:33

revert

ac208dc

added another test

97bb780

moorscode mentioned this pull request Jun 14, 2018

Added Enqueued functions check for version and in footer #1378

Merged

jrfnl mentioned this pull request Jun 25, 2018

Release 1.0.0 #1388

Closed

10 tasks

JDGrimes closed this in #1378 Jul 12, 2018

jrfnl added this to the 1.0.0 milestone Jul 12, 2018

jrfnl mentioned this pull request Jul 28, 2018

Add sniff: WordPress.WP.EnqueuedResourceParameters WPTT/WPThemeReview#170

Open

GaryJones removed this from the 1.0.0 milestone Aug 22, 2018

+              				return;
+              			}
+              			/**

Added Enqueued functions check for version and in footer #1233

Added Enqueued functions check for version and in footer #1233

Conversation

ghost commented Nov 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JDGrimes 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

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 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

Choose a reason for hiding this comment

JDGrimes commented Nov 22, 2017

JDGrimes 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

JDGrimes commented Nov 22, 2017

jrfnl left a comment • edited

Choose a reason for hiding this comment

jrfnl Nov 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl Nov 22, 2017 • edited

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 commented Nov 22, 2017

jrfnl commented Nov 23, 2017

jrfnl commented Mar 1, 2018

darthvader666uk commented Mar 1, 2018

jrfnl commented Mar 1, 2018

darthvader666uk commented Mar 8, 2018

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

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

GaryJones commented Mar 8, 2018

jrfnl commented Jun 11, 2018

darthvader666uk commented Jun 12, 2018

jrfnl commented Jun 12, 2018

moorscode commented Jun 14, 2018

jrfnl commented Jul 12, 2018

jrfnl left a comment •

edited

jrfnl Nov 22, 2017 •

edited

jrfnl Nov 22, 2017 •

edited