Check for deprecated parameters #826

grappler · 2017-01-28T19:39:42Z

Solves part of #576

I got the list of deprecated parameters by looking where _deprecated_argument() was being used.

This is the first of three sniffs. The other two that will follow are

WordPress.WP.DeprecatedParameterValues e.g. bloginfo( 'url' ); where the value url is deprecated
WordPress.WP.DeprecatedParameterArgValues e.g. For wp_dropdown_categories( $args ) $args['type'] cannot be links.

There will be an notice if the parameter value is not what is used by default. The default value is important as in some cases the deprecated parameter maybe in the middle. e.g. load_plugin_textdomain()

If a dynamic parameter is used then there will be still an error. The values for the deprecated functions are normally empty strings, bool or null. It is best to write them as is then using a function or variable.

I have again included a property to define the minimum supported WordPress version. Perhaps for future discussion: Would it make more sense to define the minimum supported WordPress version in the Sniff.php instead so it would only need to defined once instead for every deprecated sniff.

There maybe a better way to add the ordinal suffix. I have not added support for 11th, 12th or 13th as that many parameters would be an edge case.

jrfnl · 2017-01-28T20:07:48Z

Would it make more sense to define the minimum supported WordPress version in the Sniff.php instead so it would only need to defined once instead for every deprecated sniff.

That would make no difference as properties have to be defined per sniff and cannot be defined for all sniffs in a standard in one go via a ruleset. The only difference it would make is that that property would then be available to be set for all sniffs, while only a few use it, so I think it could lead to confusion.

IMHO it would be best to keep the property in the sniff class which actually uses it, but use the same name for the property everywhere. In the wiki page for properties (#614), these would then all be grouped together and intuitive to use.
We could also consider allowing that particular property to also be set from the command-line, similar to what was implemented for the I18n sniff and allow the command line value to be valid for all sniffs which use the property.

jrfnl

Great addition! Looks useful.
Aside from the remarks below, please have a look at the code style issues causing the travis build to fail.

jrfnl · 2017-01-28T22:10:35Z

WordPress/Sniffs/WP/DeprecatedParametersSniff.php

+ */
+
+/**
+ * Check for usage of deprecated parameters in WP functions and provide alternative based on the parameter passed.


provide => suggest

jrfnl · 2017-01-28T22:11:31Z

WordPress/Sniffs/WP/DeprecatedParametersSniff.php

+	 *
+	 * This sniff will throw an error when usage of deprecated parameters is
+	 * detected if the parameter was deprecated before the minimum supported
+	 * WP version; a warning otherwise.


This first line seems more like a "long description" for the sniff itself, so might be better placed in the class doc.
Possibly the second line too.

jrfnl · 2017-01-28T22:12:19Z

WordPress/Sniffs/WP/DeprecatedParametersSniff.php

+	 * By default, it is set to presume that a project will support the current
+	 * WP version and up to three releases before.
+	 * This variable allows changing the minimum supported WP version used by
+	 * this sniff by setting a property in a custom phpcs.xml ruleset.


A ruleset does not need to be named phpcs.xml

jrfnl · 2017-01-28T22:12:46Z

WordPress/Sniffs/WP/DeprecatedParametersSniff.php

+	 *
+	 * @since 0.11.0
+	 *
+	 * @var string WordPress versions.


versions => version

jrfnl · 2017-01-28T22:13:28Z

WordPress/Sniffs/WP/DeprecatedParametersSniff.php

+		'get_last_updated' => array(
+			1 => array(
+				'value'   => '',
+				'version' => '0.0',


version 0.0 ?

The parameter was already deprecated when it was merged into core. https://developer.wordpress.org/reference/functions/get_last_updated/

In that case, I suggest setting it to the version nr in which it was merged into core or to MU as may be the case.
Goes for the other function(s) with version 0.0 as well.

Turns out xfn_check() was deprecated in version 2.5. The version in core was incorrect. https://core.trac.wordpress.org/ticket/39737

jrfnl · 2017-01-28T22:40:48Z

WordPress/Tests/WP/DeprecatedParametersUnitTest.inc

@@ -0,0 +1,53 @@
+<?php
+
+// all will give an ERROR.


Comments should start with a capital.

jrfnl · 2017-01-28T22:46:16Z

WordPress/Sniffs/WP/DeprecatedParametersSniff.php

+	public function process_parameters( $stackPtr, $group_name, $matched_content, $parameters ) {
+		foreach ( $this->target_functions[ $matched_content ] as $position => $paramerter_args ) {
+
+			if ( ! isset( $parameters[ $position ] ) ) {


It might be an idea to add a $paramCount = count( $parameters ); before the loop and to add a if ( $position > $paramCount) { break; } above this line (or instead of this line) for efficiency.
After all, no need to continue looping through for parameters which aren't set anyway.

jrfnl · 2017-01-28T22:47:32Z

WordPress/Tests/WP/DeprecatedParametersUnitTest.inc

+wp_new_user_notification( '', function_name() );
+
+
+// all will give an WARNING.


jrfnl · 2017-01-28T22:47:40Z

WordPress/Tests/WP/DeprecatedParametersUnitTest.inc

+wp_title_rss( 'deprecated' );
+unregister_setting( '', '', '', 'deprecated' );
+
+// all will be OK.


jrfnl · 2017-01-28T22:49:08Z

WordPress/Tests/WP/DeprecatedParametersUnitTest.inc

+wp_title_rss( 'deprecated' );
+unregister_setting( '', '', '', 'deprecated' );
+
+// all will be OK.


Maybe add some info on why these are ok ? Is it because they have the default value ? or because the params aren't deprecated ? or ... ?

grappler · 2017-01-29T22:21:05Z

Thank you @jrfnl for the review. The issues you mentioned should be fixed now.

jrfnl · 2017-01-29T22:59:01Z

WordPress/Sniffs/WP/DeprecatedParametersSniff.php

+				$message .= ' Instead do not pass the parameter.';
+			}
+
+			if ( version_compare( $parameter_args['version'], $this->minimum_supported_version, '>=' ) ) {


Depending on which PR gets merged first, this toggle can be replaced with the addMessage() helper function as introduced in PR #827

FYI: the addMessage() utility function is now available as part of develop.

jrfnl · 2017-01-30T14:55:56Z

@grappler I'll try and have another look later this week. Quick question in the mean time: to which ruleset will this sniff be added ? And shouldn't that change be included with this PR ?

grappler · 2017-01-30T14:57:59Z

We added the other deprecated sniffs just to Extra. I can add that to the PR.

jrfnl · 2017-01-30T15:13:39Z

I wouldn't be adverse to both this sniff as well as the one for WP.DeprecatedFunctions being added to Core as - obviously - core shouldn't be using methods and parameters which it has deprecated, but others might have a different opinion.

grappler · 2017-02-01T15:20:48Z

I have used the new addMessage() utility function. I have added the sniff to the extra ruleset. ~~Will do a separate PR to add it to the core ruleset.~~ Started a discussion in #576

grappler · 2017-02-09T19:56:20Z

Does anyone else have any thoughts on this PR? Don't worry it is not as big as my last one.

JDGrimes

👍 Looks great, just a couple small tweaks I'd suggest.

JDGrimes · 2017-02-09T20:14:28Z

WordPress/Sniffs/WP/DeprecatedParametersSniff.php

+				continue;
+			}
+
+			$is_dynamic_parameter = $this->phpcsFile->findNext( array( T_CONSTANT_ENCAPSED_STRING, T_ARRAY, T_FALSE, T_TRUE, T_NULL, T_LNUMBER, T_WHITESPACE ), $parameters[ $position ]['start'], ( $parameters[ $position ]['end'] + 1 ), true, null, true );


This function call could really benefit from being multi-lined.

JDGrimes · 2017-02-09T20:15:12Z

WordPress/Sniffs/WP/DeprecatedParametersSniff.php

+
+			if ( $is_dynamic_parameter ) {
+				$this->phpcsFile->addWarning(
+					'The dynamic parameter [%s] at possition #%s of %s() could possible by deprecated.',


s/possible by/possibly be/

GaryJones

Another couple of typos, but looks good otherwise.

GaryJones · 2017-02-09T23:42:46Z

WordPress/Sniffs/WP/DeprecatedParametersSniff.php

+
+			if ( $is_dynamic_parameter ) {
+				$this->phpcsFile->addWarning(
+					'The dynamic parameter [%s] at possition #%s of %s() could possibly be deprecated.',


Typo: position

GaryJones · 2017-02-09T23:43:18Z

WordPress/Sniffs/WP/DeprecatedParametersSniff.php

+				continue;
+			}
+
+			$message = 'The parameter [%s] at possition #%s of %s() has been deprecated since WordPress version %s.';


Typo: position

JDGrimes · 2017-02-25T13:56:46Z

@jrfnl @GaryJones It looks like @grappler has fixed all of the issues you pointed out, would you like to take one more look and approve before merging?

jrfnl · 2017-02-26T06:38:27Z

@JDGrimes Yup, this one is high on my priority list, will get to it soon.

GaryJones · 2017-02-26T10:08:55Z

WordPress/Sniffs/WP/DeprecatedParametersSniff.php

+	 *
+	 * @since 0.11.0
+	 *
+	 * @var array Multi-dimentional array with parameter details.


Oh, the typo came originally from https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/blob/develop/WordPress/Sniff.php#L1596

GaryJones · 2017-02-26T10:11:23Z

WordPress/Sniffs/WP/DeprecatedParametersSniff.php

+	 *                   }
+	 *               }
+	 */
+	protected $target_functions = array(


Could we get these in alphabetical order of function name? Makes it easier to check in the future.

They are currently sorted in order of version deprecated. @jrfnl @JDGrimes What do you think?

Hmmm... I generally prefer things in alphabetical order. Like @GaryJones says it makes it easier to check if a function is already there (though of course you could also search). Having them in the order deprecated is a nice feature, but does it actually provide benefit for users/developers? Only things I can think of, is that as we add new functions to the sniff over time, we'll always be adding them at the end (which isn't necessarily any better/easier than adding them in alphabetical order, I guess). But I guess it does make it easier for somebody who is using the sniff to see which functions are going to be flagged based on what WordPress version they support. I can see the potential benefit of that, so I'm fine leaving the list like it is. But I don't have a strong opinion either way.

I'm fine with the current order then (I hadn't spotted it was in WP version order), but at least add a comment before the array saying what the order is to clarify for future readers and contributors.

Makes sense, I have done that.

Just adding my two pennies to this discussion:

In most cases, either alphabetical order or version order would be fine. In this particular case, I think alphabetical order has a clear advantage.

Consider the following example: function something has four parameters, parameter 2 gets deprecated in WP 4.0, parameter 4 in WP 4.8. As the function already has a previously deprecated parameter, it is located under the WP 4.0 section, but someone adding deprecations for WP 4.8 might easily miss that if they are adding the newly deprecated parameters. In that case, the latter function array will overrule the earlier and the WP 4.0 deprecation of parameter 2 will never be reported anymore.

I think all things considered, we should probably just have this list be in alphabetical order. I don't really see a compelling case for putting it in order of version number, so for simplicity and consistency alphabetical order seems best.

grappler · 2017-07-12T20:46:44Z

Thank you @jrfnl for your review. I have updated most of the items. Once we have clarified the last few items and the new changes look good I will make another commit.

jrfnl · 2017-07-12T21:15:24Z

WordPress/Sniffs/WP/DeprecatedParametersSniff.php

+	 *                'version' => (int) The WordPress version when deprecated.
+	 *            )
+	 *         )
+	 *    );
 	 *


PHPCS throws a notice for a superfluous empty comment line at the end here.

grappler · 2017-07-16T19:27:31Z

@jrfnl I have made a few more changes and add some replies to your comments.

jrfnl

I'm ok with merging it as it is now.

The whole "token walking" vs checking of the raw parameter value issue can be addressed if/when bugs reports about this come in.

@JDGrimes @GaryJones As the PR has changed quite a lot, do you still want to have another look or can this be merged ?

GaryJones · 2017-07-18T07:04:27Z

WordPress/Sniffs/WP/DeprecatedParametersSniff.php

+	 * Minimum WordPress version.
+	 *
+	 * This variable allows changing the minimum supported WP version used by
+	 * this sniff by setting a property in a custom ruleset xml file.


GaryJones · 2017-07-18T07:06:45Z

WordPress/Sniffs/WP/DeprecatedParametersSniff.php

+				'version' => '2.7.0',
+			),
+		),
+		'get_delete_post_link' => array(


If this list is meant to be in alphabetical order, then get_delete_post_link should come after get_category_parents.

GaryJones · 2017-07-18T07:07:51Z

WordPress/Sniffs/WP/DeprecatedParametersSniff.php

+				'version' => '2.5.0',
+			),
+		),
+		'update_user_status' => array(


The order of these three update_* keys is not alphabetical.

GaryJones · 2017-07-18T07:10:42Z

3 minor fixes if possible, but otherwise good to go.

grappler · 2017-07-18T07:42:13Z

Thanks for spotting those. I have updated the PR.

GaryJones · 2017-07-18T12:48:27Z

WordPress/Sniffs/WP/DeprecatedParametersSniff.php

+		'wp_get_http_headers' => array(
+			2 => array(
+				'value'   => false,
+				'version' => '2.7',


Does this value require being exactly 2.7 (as opposed to 2.7.0)? If so, maybe worth adding a comment so it's not accidentally "fixed" in the future?

Not that I think so, it should be 2.7.0

GaryJones · 2017-07-18T12:53:21Z

WordPress/Sniffs/WP/DeprecatedParametersSniff.php

+	 *
+	 * @var string WordPress version.
+	 */
+	public $minimum_supported_version = 4.5;


Each new WP release will require that this is updated, if the "last four major versions" gap is to be maintained.

Do we have other code that is based on a value of "$release - X"?

Yes, all of the deprecated Sniffs work with the calculation.

I wonder if there's value in having a centralised $current_version, and then having the properties for the relevant Sniffs be calculated from there (not needed for this PR - just thinking aloud).

a centralised $current_version would a good idea.

GaryJones · 2017-07-18T12:55:28Z

WordPress/Tests/WP/DeprecatedParametersUnitTest.inc

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


Are these groups in order of release? i.e. functions with deprecated params in 4.9 can be added to the end of the relevant groups? If so, maybe add a comment here to remind future contributors.

Yes the idea would be for future functions to be added at the end.

OK, so just a commented needed here then please.

(I was tempted to suggest putting these functions in alphabetical order, but then remembered seeing discussion about the better order - a comment would help tell others what the logic is.)

I have ordered the functions in the test in alphabetical order.

I have ordered the functions in the test in alphabetical order.

That wasn't my intent here - future functions, unless they start with z, will not be at the end. The grouped nature means that new functions added in the first group will naturally means all of the error line numbers below need to be updated anyway though.

JDGrimes · 2017-07-18T16:02:15Z

WordPress/Sniffs/WP/DeprecatedParametersSniff.php

+				break;
+			}
+
+			// The list will need to updated if the default value is not supported.


This comment should be moved down, right about the switch statement. It is confusing where it is.

@grappler Didn't you remove this condition earlier based on a previous comment I made ? It's superfluous. (And removing it would sort the comment out as well ;-) )

Yes, I removed it. It slipped in again. Sorry.

jrfnl · 2017-07-19T22:23:38Z

@JDGrimes I think all concerns have been addressed. Would like to take another look or can I dismiss your earlier review ?

jrfnl · 2017-07-19T22:30:38Z

WordPress/Sniffs/WP/DeprecatedParametersSniff.php

+				continue;
+			}
+
+			$message = 'The parameter "s%" at position #%s of %s() has been deprecated since WordPress version %s.';


s% should be %s

jrfnl · 2017-07-19T23:40:00Z

🎉 🍾 🌮

GaryJones · 2017-07-20T09:37:00Z

Well done and thank you @grappler :-)

jrfnl requested changes Jan 28, 2017

View reviewed changes

grappler force-pushed the feature/576-deprecated-parameters branch 2 times, most recently from 0ca5c3b to 18cd5c3 Compare January 29, 2017 22:16

jrfnl reviewed Jan 29, 2017

View reviewed changes

grappler force-pushed the feature/576-deprecated-parameters branch from 18cd5c3 to d602d0a Compare January 30, 2017 07:25

grappler force-pushed the feature/576-deprecated-parameters branch from d602d0a to 394b4b5 Compare February 1, 2017 15:17

grappler mentioned this pull request Feb 1, 2017

Add sniffs for deprecated WP features #576

Open

grappler force-pushed the feature/576-deprecated-parameters branch from 0087fd6 to d281489 Compare February 9, 2017 19:50

JDGrimes suggested changes Feb 9, 2017

View reviewed changes

JDGrimes approved these changes Feb 9, 2017

View reviewed changes

JDGrimes added Status: Review ready Type: Enhancement labels Feb 9, 2017

JDGrimes added this to the 0.11.0 milestone Feb 9, 2017

GaryJones requested changes Feb 9, 2017

View reviewed changes

grappler force-pushed the feature/576-deprecated-parameters branch 2 times, most recently from 70bd3a0 to 18dd89a Compare February 10, 2017 19:25

GaryJones reviewed Feb 26, 2017

View reviewed changes

grappler force-pushed the feature/576-deprecated-parameters branch 2 times, most recently from 1878050 to 002e7b4 Compare February 27, 2017 05:41

GaryJones approved these changes Feb 27, 2017

View reviewed changes

jrfnl reviewed Jul 12, 2017

View reviewed changes

jrfnl approved these changes Jul 18, 2017

View reviewed changes

GaryJones reviewed Jul 18, 2017

View reviewed changes

grappler force-pushed the feature/576-deprecated-parameters branch from 4e230af to 2d1226c Compare July 18, 2017 07:41

GaryJones reviewed Jul 18, 2017

View reviewed changes

JDGrimes suggested changes Jul 18, 2017

View reviewed changes

grappler force-pushed the feature/576-deprecated-parameters branch 3 times, most recently from 27df536 to 32a17ac Compare July 19, 2017 22:22

jrfnl requested changes Jul 19, 2017

View reviewed changes

Check for deprecated parameters

532839e

grappler force-pushed the feature/576-deprecated-parameters branch from 32a17ac to 532839e Compare July 19, 2017 22:54

JDGrimes approved these changes Jul 19, 2017

View reviewed changes

jrfnl approved these changes Jul 19, 2017

View reviewed changes

jrfnl merged commit 8eddc26 into WordPress:develop Jul 19, 2017

grappler deleted the feature/576-deprecated-parameters branch July 20, 2017 06:40

jrfnl mentioned this pull request Aug 7, 2017

Allow for setting the minimum supported WP version from the command line #1094

Merged

1 task

grappler mentioned this pull request Jun 4, 2018

Check for deprecated parameter values #1366

Merged

GaryJones removed the Status: Review ready label Aug 21, 2018

		wp_new_user_notification( '', function_name() );


		// all will give an WARNING.

Check for deprecated parameters #826

Check for deprecated parameters #826

Conversation

grappler commented Jan 28, 2017

jrfnl commented Jan 28, 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

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 Jan 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl commented Jan 30, 2017

grappler commented Jan 30, 2017

jrfnl commented Jan 30, 2017

grappler commented Feb 1, 2017 • edited Loading

grappler commented Feb 9, 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

GaryJones left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JDGrimes commented Feb 25, 2017

jrfnl commented Feb 26, 2017

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

grappler commented Jul 12, 2017

Choose a reason for hiding this comment

grappler commented Jul 16, 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

GaryJones commented Jul 18, 2017

grappler commented Jul 18, 2017

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 Jul 18, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl commented Jul 19, 2017

Choose a reason for hiding this comment

jrfnl commented Jul 19, 2017

GaryJones commented Jul 20, 2017

grappler commented Feb 1, 2017 •

edited

Loading

GaryJones Jul 18, 2017 •

edited

Loading