Theme Restricted Parameter #80

khacoder · 2016-08-29T22:52:14Z

Initial PR
Issue #72

grappler · 2016-09-18T14:39:15Z

WordPress/Sniffs/Theme/DeprecatedArgsSniff.php

+
+		foreach ( $checks as $key => $check ) {
+			if ( trim( $token['content'], '\"\'"' ) === $key ) {
+				$nextStackPtr = $stackPtr;


Is this line needed?

grappler · 2016-09-18T14:46:23Z

WordPress/Sniffs/Theme/DeprecatedArgsSniff.php

+		foreach ( $checks as $key => $check ) {
+			if ( trim( $token['content'], '\"\'"' ) === $key ) {
+				$nextStackPtr = $stackPtr;
+				$nextStackPtr = $phpcsFile->findNext( $types , $nextStackPtr + 1 );


Perhaps just place array( T_CONSTANT_ENCAPSED_STRING ) directly in here.

grappler · 2016-09-18T14:47:37Z

WordPress/Sniffs/Theme/DeprecatedArgsSniff.php

+			if ( trim( $token['content'], '\"\'"' ) === $key ) {
+				$nextStackPtr = $stackPtr;
+				$nextStackPtr = $phpcsFile->findNext( $types , $nextStackPtr + 1 );
+				foreach ( $check as $key2 => $arg ) {


What about $args_key instead?

Are numbers as bad as globals :)
Will fix in the next update.

grappler · 2016-09-29T06:18:16Z

WordPress/Sniffs/Theme/DeprecatedArgsSniff.php

+		);
+
+		foreach ( $checks as $key => $check ) {
+			if ( trim( $token['content'], '\"\'"' ) === $key ) {


Functions in PHP can be uppercase too. So BlogInfo() would work too. The check would not catch that.

Actually, functions in PHP are case-insensitive, no matter how the function was declared and whether it is a native PHP function or a user-land function.

So any comparison for function names should be done in a case-insensitive way.

function my_function() { // do something } my_function(); // works MY_function(); // works MY_FUNCTION(); // works My_FuncTion(); // works

jrfnl · 2016-09-29T14:34:38Z

I suggest pausing work on this one in anticipation of the AbstractFunctionCall base class as discussed in Slack.

I intend to add an AbstractFunctionCall base class to WPCS which will first process to check if something a) is a function call and b) is in the list of functions you're looking for and will only pass off the processing to the child class once that has been established

Once in the child class, you can then use for instance the (upcoming) getFunctionCallParameter( $offset ) method to get the exact parameter to check or if you only wanted to check whether a function is used, you could just throw an error straight away

jrfnl · 2016-10-22T17:49:44Z

WordPress/Sniffs/Theme/DeprecatedArgsSniff.php

+
+		if ( isset( $this->functions[ $token['content'] ] ) ) {
+			$function      = $this->functions[ $token['content'] ];
+			$parameter_arg = $this->getFunctionCallParameter( $phpcsFile, $stackPtr, 1 );


Quick remark: I would make the parameter position part of the array above as for different functions, the parameter position might be different and hardcoding it to 1 here does not allow for that flexibility.

See these example sniffs which also uses this utility function:

https://github.com/wimg/PHPCompatibility/blob/master/Sniffs/PHP/NewFunctionParametersSniff.php

https://github.com/wimg/PHPCompatibility/blob/master/Sniffs/PHP/RemovedFunctionParametersSniff.php

https://github.com/wimg/PHPCompatibility/blob/master/Sniffs/PHP/RequiredOptionalFunctionParametersSniff.php

The array for WP would look differently, but you can probably get some inspiration from the array as used there.

grappler · 2016-11-13T19:10:32Z

Not all of the parameters are actually deprecated. So I am checking if there is a reason why they were not supposed to be used. https://wordpress.slack.com/archives/core/p1479063392002489

Also it seems like feed_url is also not a valid parameter.

justintadlock · 2016-11-13T23:38:57Z

Some of the URL ones are not technically deprecated. However, they are inconsistent. Just look at this mess:

case 'stylesheet_url':
    $output = get_stylesheet_uri();
    break;
case 'stylesheet_directory':
    $output = get_stylesheet_directory_uri();
    break;
case 'template_directory':
case 'template_url':
    $output = get_template_directory_uri();
    break;

IIRC, once core WP added getter functions for the URLs, we decided to make them required for consistency sake.

I'd actually like to see core WP drop the entire function and separate it into multiple functions. It might be worth exploring putting a core ticket for that together as a team.

grappler · 2016-11-14T08:53:42Z

Ok, I move the parameters that are not deprecated to a restricted parameters sniff so that there is a clear distinction.

I'd actually like to see core WP drop the entire function and separate it into multiple functions. It might be worth exploring putting a core ticket for that together as a team.

Sounds like a good idea.

pattonwebz · 2018-05-31T09:56:27Z

Verify restrictions and alts against TRT handbook.

jrfnl · 2018-05-31T09:57:36Z

I will review the actual logic in the process_parameter() method and let you know if I have feedback.

jrfnl · 2018-05-31T09:58:58Z

I will also rebase the PR & update for PHPCS 3.x.

khacoder · 2018-05-31T18:17:45Z

I would like to finish this one if I could please.

khacoder · 2018-06-02T02:22:06Z

Looks like this one has had some changes, I will let you set it up for 3.x. If it needs any additional attention let me know.

Initial Commit Issue #72

jrfnl · 2018-06-02T13:22:46Z

@khacoder Could you give me the rights to push to your fork of the repo ? I've made the PHPCS 3.x update, but my push gets rejected.

jrfnl · 2018-06-02T18:33:59Z

@khacoder Saw the invite. Thanks for that. I've just (force) pushed the updated branch. You will need to reset your local branch to the remote version to get access to the changes.

As you asked to continue with the sniff yourself, I've only done a rebase and added a commit which sorts out the PHPCS 3.x compatibility and some minor updates for consistency with WPCS.

Would you like me to review the logic in the process_parameter() method at this time or do you want to have a look yourself first ?

khacoder · 2018-06-02T20:09:09Z

Sure I can take a look.

…WPCS

khacoder · 2018-06-03T04:57:03Z

Just going over the logic and reason for this sniff. It was initially set up to replicate the more-deprecated.php sniff in the theme check plugin. The theme check is currently unchanged from when this sniff was initially developed.

So why has this sniff changed? Many parameters have been added that are not deprecated, and parameters have been removed that are deprecated.

So why are Error's being generated for non deprecated parameters? I would think that they should be Warnings ?

Edit: Many of the alts are output by the functions, so I was kind of wondering why they are even in the sniff?

khacoder · 2018-06-04T15:46:32Z

Hey @jrfnl

I think I'll let you finish this off, sorry about the above comments.

Just a heads up, the error message needs some love. Also check the bloginfo() alts as bloginfo echos get bloginfo().

grappler · 2018-06-04T17:47:15Z

As @khacoder prompted me to look closer at the PR I realised I had worked on a sniff to check for deprecated parameter values that should be merged into WPCS itself.

grappler/WordPress-Coding-Standards@61897bf

For that reason I think this PR should be marked as wait for upstream for the moment till that sniff it added to WPCS and then this Sniff could extend that.

jrfnl · 2018-06-04T17:52:18Z

@grappler 👍 Sounds good to me. Go for it! (and let me know if you want me to take a look in advance)

jrfnl · 2018-06-05T01:57:28Z

and then this Sniff could extend that.

@grappler Having just reviewed the upstream sniff, if you want to be able to extend that one, it will need some changes in the setup.

justintadlock · 2018-06-12T18:39:21Z

The team decided that this should mirror Theme Check and only catch the stylesheet and directory URLs. @khacoder can fill in the details if necessary.

Meeting: https://wordpress.slack.com/archives/C02RP4Y3K/p1528822802001090

jrfnl · 2018-07-25T23:36:02Z

PR #165 has been pulled which will include the WPCS deprecated parameter values sniff as created by @grappler. Once that PR has been merged, this PR can be adjusted to extend the upstream WPCS sniff.

grappler reviewed Sep 18, 2016

View reviewed changes

grappler added the pr: needs review label Sep 27, 2016

grappler force-pushed the feature/theme-review-sniffs branch 2 times, most recently from 563cd1d to 6671823 Compare September 27, 2016 12:28

grappler reviewed Sep 29, 2016

View reviewed changes

grappler removed the pr: needs review label Sep 29, 2016

grappler added the pr: wait for upstream label Oct 13, 2016

grappler self-assigned this Oct 13, 2016

grappler added pr: needs update and removed pr: needs update labels Oct 19, 2016

jrfnl reviewed Oct 22, 2016

View reviewed changes

grappler added the pr: needs update label Oct 23, 2016

This was referenced Dec 18, 2016

Add sniffs for the theme checks WordPress/WordPress-Coding-Standards#578

Closed

[New sniff] Incorrect function parameter usage #96

Open

grappler mentioned this pull request Dec 25, 2016

New Abstract for restricting arguments in functions WordPress/WordPress-Coding-Standards#745

Closed

grappler changed the title ~~Deprecated Args~~ Theme Restricted Parameter Jan 30, 2017

grappler added pr: wait for upstream and removed pr: wait for upstream labels Jan 30, 2017

grappler removed their assignment May 31, 2018

grappler removed the pr: wait for upstream label May 31, 2018

Deprecated Args

c87bf06

Initial Commit Issue #72

Check only for restricted parameter values

3d5a9b2

RestrictedParameterValues: update for PHPCS 3.x and consistency with …

9b23867

…WPCS

jrfnl added the pr: wait for upstream label Jun 4, 2018

This was referenced Jul 28, 2018

Add sniff: WordPress.WP.DeprecatedParameterValues #171

Closed

Meeting Notes 2018-07-28 #172

Open

jrfnl closed this Sep 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Theme Restricted Parameter #80

Theme Restricted Parameter #80

khacoder commented Aug 29, 2016

grappler Sep 18, 2016

grappler Sep 18, 2016

grappler Sep 18, 2016

khacoder Sep 27, 2016

grappler Sep 29, 2016

jrfnl Sep 29, 2016

jrfnl commented Sep 29, 2016

jrfnl Oct 22, 2016

grappler commented Nov 13, 2016

justintadlock commented Nov 13, 2016

grappler commented Nov 14, 2016

pattonwebz commented May 31, 2018

jrfnl commented May 31, 2018

jrfnl commented May 31, 2018

khacoder commented May 31, 2018

khacoder commented Jun 2, 2018

jrfnl commented Jun 2, 2018

jrfnl commented Jun 2, 2018

khacoder commented Jun 2, 2018

khacoder commented Jun 3, 2018 •

edited

Loading

khacoder commented Jun 4, 2018 •

edited

Loading

grappler commented Jun 4, 2018

jrfnl commented Jun 4, 2018

jrfnl commented Jun 5, 2018

justintadlock commented Jun 12, 2018

jrfnl commented Jul 25, 2018

Theme Restricted Parameter #80

Theme Restricted Parameter #80

Conversation

khacoder commented Aug 29, 2016

grappler Sep 18, 2016

Choose a reason for hiding this comment

grappler Sep 18, 2016

Choose a reason for hiding this comment

grappler Sep 18, 2016

Choose a reason for hiding this comment

khacoder Sep 27, 2016

Choose a reason for hiding this comment

grappler Sep 29, 2016

Choose a reason for hiding this comment

jrfnl Sep 29, 2016

Choose a reason for hiding this comment

jrfnl commented Sep 29, 2016

jrfnl Oct 22, 2016

Choose a reason for hiding this comment

grappler commented Nov 13, 2016

justintadlock commented Nov 13, 2016

grappler commented Nov 14, 2016

pattonwebz commented May 31, 2018

jrfnl commented May 31, 2018

jrfnl commented May 31, 2018

khacoder commented May 31, 2018

khacoder commented Jun 2, 2018

jrfnl commented Jun 2, 2018

jrfnl commented Jun 2, 2018

khacoder commented Jun 2, 2018

khacoder commented Jun 3, 2018 • edited Loading

khacoder commented Jun 4, 2018 • edited Loading

grappler commented Jun 4, 2018

jrfnl commented Jun 4, 2018

jrfnl commented Jun 5, 2018

justintadlock commented Jun 12, 2018

jrfnl commented Jul 25, 2018

khacoder commented Jun 3, 2018 •

edited

Loading

khacoder commented Jun 4, 2018 •

edited

Loading