Skip to content
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

No Sanitize Callback #76

Closed
wants to merge 6 commits into from
Closed

No Sanitize Callback #76

wants to merge 6 commits into from

Conversation

khacoder
Copy link
Contributor

First commit
Issue #68

Copy link
Member

@grappler grappler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be interesting to compare the sanitization functions with a list of core functions and add a Warning if the author was using their own function.

* @package PHP_CodeSniffer
* @author khacoder
*/
class WordPress_Sniffs_Theme_NoSanitizeCallBackSniff implements PHP_CodeSniffer_Sniff
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As "callback" is one word I would make the B small.

$tokens = $phpcsFile->getTokens();
$token = $tokens[ $stackPtr ];
$types = array( T_STRING, T_CONSTANT_ENCAPSED_STRING, T_VARIABLE, T_LNUMBER, T_SEMICOLON );
if ( '$wp_customize' === $token['content'] ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not guaranteed. The developer does not need to use $wp_customize. The variable could be named anything. This has become a bit of a standard as that is what core used.

$phpcsFile->addError( 'Neither sanitize_callback or sanitize_js_callback were found for this option.', $stackPtr, 'SanitizeCallbackChecks' );
}
if ( false !== $sanitize_callback_found && false !== $empty_callback && true === $sanitize_js_callback_found && true === $empty_js_callback ) {
$phpcsFile->addError( 'Either sanitize_callback or sanitize_js_callback must not be empty.', $stackPtr, 'SanitizeCallbackChecks' );
Copy link
Member

@grappler grappler Sep 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a separate error message depending if the variables exist or not? What about "'sanitize_callback or sanitize_js_callback do not have a valid sanitization callback defined.'"

@grappler
Copy link
Member

I will be working to update the code for compatibility with PHPCS 3.x. Can use WordPress\AbstractFunctionRestrictionsSniff for this to reduce the amount of code needed.

@khacoder
Copy link
Contributor Author

I will try and pick this up again, if that's OK with everyone.

@grappler
Copy link
Member

@khacoder I was just working on this. Let me commit my changes and then you can continue.

- Extend WordPress\Sniff and use the utility functions from there
@grappler
Copy link
Member

@khacoder I have added my changes which make the code PHPCS 3.x compatible and uses the new utility functions from the WordPress/Sniff class.

The tests are failing because of $parameter_arg['end']. https://github.com/khacoder/WordPress-Coding-Standards/blob/1ab6a0ff07a65b8a9dcad2c73e9d75f83791b6a7/WordPress/Sniffs/Theme/NoSanitizeCallBackSniff.php#L95
At one point $parameter_arg is being defined as false. I am not sure why.

@grappler grappler assigned khacoder and unassigned grappler May 31, 2018
@jrfnl
Copy link

jrfnl commented Jun 1, 2018

@khacoder Welcome back! I'm happy to let you continue, however, be aware that extensive changes are needed to this sniff.

This sniff should extend the WPCS AbstractFunctionParameters sniff and a lot of the logic currently in the sniff can be removed. What's left should go into the process_parameters() method.

Once the basic principle of the sniff has been updated for the above, I will do a full review which will necessitate further changes.

Will you have the time to make these changes and process any feedback you receive before the end of next week ?
We would like to have this sniff ready & merged before WCEU.

@khacoder
Copy link
Contributor Author

khacoder commented Jun 1, 2018

I will try.

@khacoder
Copy link
Contributor Author

khacoder commented Jun 1, 2018

How can the AbstractFunctionParameters sniff be used if we are trying to get the parameters for the $wp_customize->add_setting method ?

kevinhaig and others added 2 commits June 1, 2018 16:33
Fixed the undefined index bug, modified the logic a bit, to ensure sanitize_callback is included.
Override `is_targetted_token()` so to be able to check for the method `$wp_customize->add_setting()`
@jrfnl jrfnl closed this Sep 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants