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

Add Theme forbidden PHP functions sniff + unit tests. #10

Closed

Conversation

jrfnl
Copy link

@jrfnl jrfnl commented Jul 11, 2016

Initial setup for the forbidden functions sniff as described in issue #9.

[Edit] Changed over to use WordPress_Sniffs_Functions_FunctionRestrictionsSniff as the base class rather than Generic_Sniffs_PHP_ForbiddenFunctionsSniff.
This allows for better differentiation of the error messages, keeping it more in line with what people have come to expect from Theme Check.

@jrfnl jrfnl force-pushed the feature/issue-9-forbidden-functions branch from 4d3d110 to 47976eb Compare July 11, 2016 12:04
@jrfnl jrfnl changed the title Add Theme forbidden functions sniff + unit tests. Add Theme forbidden PHP functions sniff + unit tests. Jul 11, 2016
@jrfnl jrfnl force-pushed the feature/issue-9-forbidden-functions branch 2 times, most recently from f84cf0f to c6fd313 Compare July 12, 2016 13:36
@grappler grappler force-pushed the feature/theme-review-sniffs branch from b8a5e09 to 5dd4758 Compare July 14, 2016 12:51
@jrfnl jrfnl force-pushed the feature/issue-9-forbidden-functions branch 3 times, most recently from 7249019 to d312c6b Compare July 19, 2016 11:33
@jrfnl jrfnl force-pushed the feature/issue-9-forbidden-functions branch from d312c6b to fd3ae5b Compare July 27, 2016 03:39
@jrfnl
Copy link
Author

jrfnl commented Jul 27, 2016

Regarding the travis failure: this has to do with a double fix upstream in WPCS and in PHPCS. Once the up to date version of WPCS develop has been merged in, I'll rebase and the build will pass.

@ernilambar
Copy link
Member

I propose adding create_function to the forbidden list. If not error then at least warning.

@grappler
Copy link
Member

I would like to fully modularize the discouraged functions upstream before we merge this in. Ref: WordPress/WordPress-Coding-Standards#633

create_function is still a recommended method for registering widgets on Codex https://codex.wordpress.org/Widgets_API

Also https://twitter.com/grapplerulrich/status/759126711929798660

@Pross
Copy link

Pross commented Jul 29, 2016

Its drafted for deprecation in 7.1
https://wiki.php.net/rfc/deprecations_php_7_1

@jrfnl
Copy link
Author

jrfnl commented Jul 29, 2016

But it's the only alternative to anonymous functions in PHP 5.2 and as WP is still not dropping 5.2 and the adoption of WP on 5.2 > than WP on 7.1....
In other words - yes, create_function() should be discouraged, but only when WP drops support for PHP 5.2 somewhere in the future.

@jrfnl
Copy link
Author

jrfnl commented Jul 29, 2016

Also: AFAICS looks like no vote has taken place yet and as PHP 7.1 is already in RC Beta phase, I doubt it will make it in in time, so deprecation will probably not happen in PHP 7.1.

@ernilambar
Copy link
Member

When something hooked using create_function, that cannot be unhook/removed/modified from child theme. Thus making theme not fully child theme ready (which is against TRT guideline).

@justintadlock
Copy link

Are there any use cases for create_function() in a theme?

Like @ernilambar said, themes must be child-theme ready. Something like this is not child-theme compatible:

add_action(
    'wp_head',
    create_function( '', 'return some_function();' )
);

However, this is child theme compatible because the widget can be deregistered:

add_action(
    'widgets_init',
    create_function( '', 'return register_widget("My_Widget");')
);

At the very least, this should be a warning. More times than not, it's going to be an indication of some issue. Reviewers and theme authors should definitely be made aware of it.

@grappler grappler force-pushed the feature/theme-review-sniffs branch 2 times, most recently from 563cd1d to 6671823 Compare September 27, 2016 12:28
@jrfnl jrfnl force-pushed the feature/issue-9-forbidden-functions branch from fd3ae5b to ffebe73 Compare September 27, 2016 12:45
@ernilambar
Copy link
Member

paginate_links() is almost all the time used in the place where it can be replaced by the_posts_pagination(). I know paginate_links() is capable of paginating anything. But I cannot think any other use cases. May be we can add paginate_links() in discouraged functions list.
Any other use cases?

@justintadlock
Copy link

The two most likely use cases for paginate_links() are:

  1. When you need to create pagination that the core the_posts_pagination() is not capable of.
  2. When you're building special pagination for a plugin that your theme supports.

But, legit use cases are few and far between.

@grappler grappler force-pushed the feature/issue-9-forbidden-functions branch from 5951a76 to cbeb66e Compare January 22, 2017 18:43
@grappler grappler force-pushed the feature/issue-9-forbidden-functions branch from cbeb66e to cd34a9c Compare January 30, 2017 12:25
@grappler
Copy link
Member

grappler commented Jun 3, 2017

@jrfnl I think this can be merged? I don't see any reason not to.

public function getGroups() {
return array(

'eval' => array(
Copy link
Author

Choose a reason for hiding this comment

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

eval should be removed here as it's a language construct, not a function (the dedicated token has since the original PR been removed from the parent sniff as well).
An upstream Squiz sniff should be added instead.
See: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/blob/develop/WordPress-Extra/ruleset.xml#L73-L77

*
* @since 0.xx.0
*/
class WordPress_Sniffs_Theme_RestrictedPHPFunctionsSniff extends WordPress_Sniffs_Functions_FunctionRestrictionsSniff {
Copy link
Author

Choose a reason for hiding this comment

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

This WPCS has since the original PR been deprecated. The WordPress\AbstractFunctionRestrictionsSniff should be used instead.

*/

/**
* Forbids usage of certain fuctions and recommends alternatives.
Copy link
Author

Choose a reason for hiding this comment

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

fuctions => functions

),
),

'system_calls' => array(
Copy link
Author

Choose a reason for hiding this comment

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

This group should be removed as it's already covered in the PHP\DiscouragedPHPFunctions sniff.
If the error level & message needs changing, this can be done from the ruleset.

/**
* Groups of functions to restrict
*
* Example: groups => array(
Copy link
Author

Choose a reason for hiding this comment

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

Example should be removed (as it belong with the parent class) or updated.

),
),

'obfuscation' => array(
Copy link
Author

Choose a reason for hiding this comment

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

This group should be removed as it's already covered in the PHP\DiscouragedPHPFunctions sniff.
If the error level & message needs changing, this can be done from the ruleset.

*
* @category Theme
* @package PHP_CodeSniffer
* @author Juliette Reinders Folmer <wpplugins_nospam@adviesenzo.nl>
Copy link
Author

Choose a reason for hiding this comment

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

Alignment + comment style should be updated to reflect upstream changes ;-)

@@ -89,6 +89,12 @@
<!-- Prohibit the use of the backtick operator. -->
<rule ref="Generic.PHP.BacktickOperator"/>

<!-- Discourage a number of functions for usage in a theme. -->
<rule ref="WordPress.PHP.DiscouragedFunctions"/>
Copy link
Author

Choose a reason for hiding this comment

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

This sniff is now deprecated.

The WordPress.PHP.PHPDiscouragedFunctions sniff covers at least part of it.

@jrfnl
Copy link
Author

jrfnl commented Jul 25, 2017

I think this can be merged? I don't see any reason not to.

Had a quick look and sorry, no, this can't be merged as is. Code is very out of date compared to upstream.

@jrfnl
Copy link
Author

jrfnl commented May 31, 2018

  • I will rebase this PR shortly & update for PHPCS 3.x
  • Need input for what functions need to be sniffed for, what should be discouraged, what forbidden.

@jrfnl jrfnl force-pushed the feature/issue-9-forbidden-functions branch from 7726530 to 1bd5a98 Compare May 31, 2018 11:44
jrfnl and others added 6 commits May 31, 2018 13:44
`eval()` is a language construct, not a function, so cannot and should not be in this sniff.

The ruleset already contains a separate sniff which checks for it - see 121 -, so removing does not devaluate what's being checked by WPTRT-CS.
@jrfnl jrfnl closed this Sep 23, 2018
@jrfnl
Copy link
Author

jrfnl commented Sep 23, 2018

For history:

This PR was closed when we replaced the master/develop branches with the re-organized stand-alone repo.

It will not be re-opened for the following reasons:

  1. This PR as it stands now would need to be split into several different ones anyway, think:
    • Add upstream WPCS sniffs to the ruleset.
    • Add sniff for PHP functions forbidden/discouraged for Themes.
    • Add sniff for WP functions forbidden/discouraged for Themes or, more likely, add to the existing PluginTerritory\ForbiddenFunctions sniff.
  2. There is - at this moment - no clear list of which functions should be forbidden in the first place, so unless and until that becomes clearer, this PR would just be open for eternity.

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

5 participants