Modularizing the discouraged functions #633

Merged
merged 8 commits into from Dec 30, 2016

Projects

None yet

6 participants

@grappler
Contributor
grappler commented Jul 25, 2016 edited

This is an intial start of fixing #582 & #576

Do you think I am on the right path?

TODO

  • Complete the list of deprecated functions.
  • Finish modularising WordPress.VIP.RestrictedFunctions
  • Find a place for the forbidden Theme Review functions. WPTRT#9 WPTRT#10
  • Update the unit tests to include the new functions.
  • Update the rulesets with the changes
@jrfnl jrfnl and 1 other commented on an outdated diff Jul 27, 2016
WordPress/Sniffs/PHP/DeprecatedFunctionsSniff.php
+ * @category PHP
+ * @package PHP_CodeSniffer
+ * @link https://make.wordpress.org/core/handbook/best-practices/coding-standards/
+ */
+
+/**
+ * WordPress_Sniffs_PHP_DeprecatedFunctionsSniff.
+ *
+ * Restricts usage of deprecated PHP functions.
+ *
+ * @category PHP
+ * @package PHP_CodeSniffer
+ * @author Shady Sharaf <shady@x-team.com>
+ * @author Ulrich Pogson <ulrich@pogson.ch>
+ */
+class WordPress_Sniffs_PHP_DeprecatedFunctionsSniff extends WordPress_AbstractFunctionRestrictionsSniff {
@jrfnl
jrfnl Jul 27, 2016 Contributor

FYI: This whole file can be removed as this is covered in #608.

@grappler
grappler Jul 27, 2016 Contributor

Thanks, I will rebase once it is merged.

@grappler grappler referenced this pull request in WPTRT/WordPress-Coding-Standards Jul 29, 2016
Open

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

@grappler
Contributor

As I am going through DiscouragedFunctionsSniff I see a check for register_globals() but I cannot find any reference to that function in WordPress other than WP::register_globals. Is that what the sniff is trying to catch?

@grappler
Contributor

A quick update. I have separated the different functions. Currently Deprecated function are not done yet. Waiting for a response from #576 (comment)

Travis is failing becuase of another issue. My unit tests are passing.

This list of functions is still in WordPress_Sniffs_VIP_RestrictedFunctionsSniff. I think it should be moved out so that others have access to it but I am not sure where.

            'runtime_configuration' => array(
                'type'      => 'error',
                'message'   => '%s is prohibited, changing configuration at runtime is not allowed on VIP Production.',
                'functions' => array(
                    'dl',
                    'error_reporting',
                    'ini_alter',
                    'ini_restore',
                    'ini_set',
                    'magic_quotes_runtime',
                    'set_magic_quotes_runtime',
                    'apache_setenv',
                    'putenv',
                    'set_include_path',
                    'restore_include_path',
                ),
            ),

We also have a list of functions that are blocked by the theme review team that we could maybe add to the same sniff. https://github.com/WPTRT/WordPress-Coding-Standards/pull/10/files#diff-75f603ed5bb071632d90df9854203926R29

Would love some feedback! 😄

@JDGrimes
Contributor

RE register_globals(): maybe that was a reference to this?

@grappler
Contributor

So register_globals is a deprecated PHP ini config. I think it is best left of the check. Do we need to document this in the class changelog?

@jrfnl
Contributor
jrfnl commented Sep 28, 2016

So register_globals is a deprecated PHP ini config. I think it is best left of the check. Do we need to document this in the class changelog?

As the WordPress_AbstractFunctionRestrictionsSniff class which is used as the base only checks for function calls, this should probably be removed - or better yet: be moved to a separate sniff which checks for ini directives.

@grappler
Contributor

The check is already included in PHPCompatibility DeprecatedIniDirectivesSniff.php

What do you think of including the TRT restricted PHP functions in the PHP folder?

@jrfnl
Contributor
jrfnl commented Sep 29, 2016

The check is already included in PHPCompatibility DeprecatedIniDirectivesSniff.php

The PHPCompatibility sniffs are a separate ruleset which does not get shipped with WPCS, so unless people add that to their custom ruleset, they won't get the benefit of it.

What do you think of including the TRT restricted PHP functions in the PHP folder?

You mean including the array from the TR sniff in the WordPress_Sniffs_PHP_RestrictedFunctionsSniff ? Possibly, but only if it doesn't impact any of the other WP rulesets.
I'd be more inclined to add the function groups each in a separate sniff file which could then go in the PHP folder (so it is easier to include them in the rulesets).

For that matter - the splitting out of the files as you're doing in this PR will also affect some rulesets, so you may want to add an action item to review & adjust the existing rulesets for the changes you are making.

@grappler
Contributor

The PHPCompatibility sniffs are a separate ruleset which does not get shipped with WPCS, so unless people add that to their custom ruleset, they won't get the benefit of it.

I am not that keen on duplicating checks just so that we have backwards compatibility. I expect in most WP projects there is no php.ini file. This check was looking for a function called register_globals() and not an ini directive.

I'd be more inclined to add the function groups each in a separate sniff file which could then go in the PHP folder (so it is easier to include them in the rulesets).

This is what I meant but I am not sure do we want to create a separate sniff for each item or if we can bundle a few together.

so you may want to add an action item to review & adjust the existing rulesets for the changes you are making.

Thought of that, I added an action item. 😄

@jrfnl
Contributor
jrfnl commented Sep 29, 2016

The PHPCompatibility sniffs are a separate ruleset which does not get shipped with WPCS, so unless people add that to their custom ruleset, they won't get the benefit of it.
I am not that keen on duplicating checks just so that we have backwards compatibility. I expect in most WP projects there is no php.ini file. This check was looking for a function called register_globals() and not an ini directive.

So the original sniff was wrong, but that doesn't mean it shouldn't be sniffed for. register_globals is evil and should not be touched.
Whether a project has a php.ini file is less relevant as a) those won't be parsed by PHPCS anyway and b) register_globals could - when it was still in PHP - be changed via ini_set() AFAICR which is what should be sniffed for.
Maybe leave it out of this PR, but open a new issue for this ?

@grappler
Contributor

The rulesets should be should checking the same things are were doing before the functions were modularized.

I have place all of the restricted functions in WordPress.PHP.RestrictedFunctions. It is better to have all of the restricted functions together. If a ruleset does not need a certain check then it can still be excluded like so:

<rule ref="WordPress.WP.AlternativeFunctions">
    <properties>
        <property name="exclude" value="curl,file_get_contents"/>
    </properties>
</rule>

The only remaining thing are the deprecated functions which are being discussed in #576 (comment)

@grappler
Contributor

I have added a start for a check for deprecated based on WPTRT#77

@JDGrimes Would be interested on your inital feedback on the deprecated functions check. It is just a start.

- );
+ 'urlencode' => array(
+ 'type' => 'warning',
+ 'message' => '%s() should only be used when dealing with legacy applications rawurlencode should now de used instead. See http://php.net/manual/en/function.rawurlencode.php and http://www.faqs.org/rfcs/rfc3986.html',
@JDGrimes
JDGrimes Oct 11, 2016 Contributor

Small typo here, "de" instead of "be".

+ */
+ public function getGroups() {
+ foreach ( $this->depreacted_functions as $depreacted_function => $data ) {
+ if ( intval( $data['version'] < $this->minimum_supported_version ) ) {
@JDGrimes
JDGrimes Oct 11, 2016 Contributor

I don't think intval() is needed here, or maybe you didn't intend for it to wrap the entire comparison?

@westonruter
westonruter Oct 11, 2016 Member

Shouldn't version_compare() be used?

@JDGrimes
Contributor

The deprecated functions check looks like it is going in the right direction. The main thing that I think could be improved is the minimum WP version. Just so that we're all on the same page, the purpose of having the minimum WP version would seem to be this:

Say that you have a plugin that supports WordPress 4.2-4.6. But a function was deprecated in 4.3. In WordPress 4.3+, you want to avoid that function. But in 4.2, it isn't deprecated, and you may actually have to use it, because the alternative may not have been available yet.

The ability to define the lowest version that is supported lets us continue to use such a function in such a project, without getting errors for it (currently the sniff would still give a warning though).

In general I think this is a good feature, but I do think that it could be even better. For example, sometimes when a function is deprecated, the alternative may have already existed. So using the above example, if the deprecated function had an alternative, and that alternative was available in 4.2, then there is no reason that we shouldn't use it. Admittedly though, I'm not sure how common that situation is. If we find that it is at all common, we could check the minimum version against the version that the alternative function was introduced, instead of against the version that the deprecated function was deprecated.

However, a more general thing that we could do is check that whenever a deprecated function does need to be used on older versions of WordPress, that it would be surrounded by a WP version check, and only used on those older versions where it isn't deprecated yet. Of course, that feature could be added to the sniff later, I guess, it doesn't have to be in the initial version, especially since a warning will still be given.

@grappler
Contributor

The ability to define the lowest version that is supported lets us continue to use such a function in such a project, without getting errors for it (currently the sniff would still give a warning though).

Yes that is right. We need the warning system for the theme review so that theme authors know they have to switch the deprected function out in the future.

sometimes when a function is deprecated, the alternative may have already existed.

This may be the case but I have not come across this.

However, a more general thing that we could do is check that whenever a deprecated function does need to be used on older versions of WordPress, that it would be surrounded by a WP version check, and only used on those older versions where it isn't deprecated yet.

There are few different ways to do the check depending on the function so this would be a bit more involved. It would make sense to add support for somthing like this till a maximum of version 3.0. Some check if the new function exists and otherwise use the depreacted functions. Here is an example which checks for a function introduced in the version but is not the alternative function that should be used. https://make.wordpress.org/core/2015/10/20/document-title-in-4-4/

Thank you for the vote of confidence. I will get this finished up soon.

@grappler
Contributor

I have completed the WordPress Deprecated functions sniff. Somehow the CURL functions check is failing when using PHPCS 2.7. I don't think it is due to my PR as I just moved the code around.

@grappler grappler referenced this pull request in WPTRT/WordPress-Coding-Standards Oct 13, 2016
Open

No PHP File Functions #79

@grappler
Contributor

I have worked out why travis test is failing with PHPCS 2.7. I have excluded two function groupd in the VIP ruleset as they don't apply there. When running the Unit Test in PHPCS 2.7 it expects an notice but there is non becuase the function groups have been exluded.

I wonder if there is a way to check when ruleset is being used and then change the expected value for the Unit Test.

In WordPress_Sniffs_WP_AlternativeFunctionsSniff the notice is set to Warning but for the Theme Review sniff we will need them to be Errors. Is there any reason not to set them to errors? I kept them as Warnings as that is what they were in the VIP sniff.

@jrfnl
Contributor
jrfnl commented Oct 14, 2016

The unit tests test the sniff, not the ruleset, so in that case, the unit tests need to be corrected. They should test the complete sniff. See also #696.

@jrfnl
Contributor
jrfnl commented Oct 14, 2016

In WordPress_Sniffs_WP_AlternativeFunctionsSniff the notice is set to Warning but for the Theme Review sniff we will need them to be Errors.

That can be changed from the (Theme) ruleset anyway, so the sniff itself does not need to be changed for that.

@grappler
Contributor

The unit tests test the sniff, not the ruleset, so in that case, the unit tests need to be corrected. They should test the complete sniff.

The unit test is testing the complete sniff as that is why travis is passing when using PHPCS master but it is failing when using PHPCS 2.7. This is the reverse ofhttps://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/696

@jrfnl
Contributor
jrfnl commented Oct 14, 2016

@grappler Hmm.. I'd need to look in more detail. Haven't got time at the moment, will try next week. if it's still needed by then.

@jrfnl
Contributor
jrfnl commented Oct 27, 2016

@grappler I've had a look at this and found a way to solve it. I've send in a PR to this branch in your fork to sort it out. grappler#1

@grappler
Contributor

Nearly there, now the travis test is failing because eval is being used in VIP/CronIntervalSniff.php. I don't think WordPress_AbstractFunctionRestrictionsSniff supports whitelisting comments.

@jrfnl
Contributor
jrfnl commented Oct 28, 2016 edited

Nearly there, now the travis test is failing because eval is being used in VIP/CronIntervalSniff.php

Not sure how the others feel about using this, but you could get the build to pass by adding // @codingStandardsIgnoreLine to the line.

So the line would becomes something like this (not sure how well the comment works in combination with the original comment, but a quick test should tell you quickly enough):

    $interval = eval( "return ( $value );" ); // @codingStandardsIgnoreLine - No harm here.

Ref: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Advanced-Usage#ignoring-parts-of-a-file

@grappler
Contributor

The build is now passing. Can the label Status: Review Ready be added?

@grappler
Contributor
grappler commented Nov 8, 2016

Any chance of a review? 😄

@westonruter
Member

@jrfnl if you're good with this then I am good with it.

@jrfnl
Contributor
jrfnl commented Dec 5, 2016

@westonruter I'll try and do a thorough review later this week

@jrfnl

Before I say anything else: thank you so much for all your hard work on this @grappler!

The "problem" with this PR is that it effectively addresses three different issues:

  • move around existing functionality - which addresses the original issue it set out to solve
  • introduce new functions & remove some others within existing functionality - including a number which could be considered controversial and should be discussed
  • introduce new functionality (deprecated functions sniff with version compare)

This made this PR very hard and time consuming to review as these three things need to be reviewed in different ways.
If the PR would have been split into three subsequent PRs - each addressing one of these distinct issues, it would have been much easier.

Having said that, most of the proposed changes are great and very welcome. Kudos!

I do feel that the "silently introduced" functionality needs to be made explicit so that more people can give their opinion (and to make it a lot easier to write the changelog for this later), so based on my review, please find the below summary.
Regarding the code itself, I've left comments inline. Some comments on sentence structure and such might not be directly related to this PR, but as the lines were touched now anyway, we might as well get it right.

Significant changes to take note of based on the current state of the PR:

extra ruleset

The following "functions" previously being sniffed for have been removed:

  • find_base_dir() (method)
  • get_base_dir() (method)
  • register_globals (ini directive)

The following functions which were previously not sniffed for are being newly introduced into the extra ruleset:

  • WordPress.PHP.DevelopmentFunctions

    • section error_log: error_log(), trigger_error(), set_error_handler(), debug_backtrace(), wp_debug_backtrace_summary()
    • section prevent_path_disclosure: error_reporting(), phpinfo()
  • WordPress.PHP.DiscouragedFunctions

    • create_function()
    • serialize(), unserialize()
    • urlencode()
  • WordPress.PHP.RestrictedFunctions

    • section eval: eval()
    • section runtime_configuration: dl, error_reporting, ini_alter, ini_restore, ini_set, magic_quotes_runtime, set_magic_quotes_runtime, apache_setenv, putenv, set_include_path, restore_include_path
    • section system_calls: exec, passthru, proc_open, shell_exec, system, popen
    • section obfuscation: base64_decode, base64_encode, convert_uudecode, convert_uuencode, str_rot13
  • WordPress.WP.DeprecatedFunctions

    • Lots of new functions, too many to list
  • WordPress.WP.AlternativeFunctions

    • curl_*()
    • parse_url()
    • file_get_contents()
    • section file_system_read: readfile, fopen, fsockopen, pfsockopen, fclose, fread, fwrite

VIP ruleset

The following functions which were previously not sniffed for are being newly introduced into the VIP ruleset:

  • WordPress.PHP.DevelopmentFunctions

    • section error_log: var_export(), debug_backtrace(), debug_print_backtrace(), wp_debug_backtrace_summary()
  • WordPress.PHP.RestrictedFunctions

    • section system_calls: exec, passthru, proc_open, shell_exec, system, popen
    • section obfuscation: base64_decode, base64_encode, convert_uudecode, convert_uuencode, str_rot13
  • WordPress.WP.AlternativeFunctions

    • json_encode() 👍
    • section file_system_read: readfile, fopen, fsockopen, pfsockopen, fclose, fread, fwrite

Also note that the WordPress.WP.DeprecatedFunctions sniff has not been added to the VIP ruleset.

@westonruter @JDGrimes @GaryJones Please have a look through the above change summary and leave an opinion on this.

My personal opinion about the changes to the extra ruleset:

  • The two removed methods should be brought back - see inline comment.

  • WordPress.PHP.DevelopmentFunctions / section error_log: For a number of these functions there are perfectly valid reasons to use them in production. Think: setting an error handler which logs errors instead of displaying them and includes a backtrace.

  • WordPress.PHP.DevelopmentFunctions / section prevent_path_disclosure: Looking at the functions included, I believe this group should be moved back to the VIP.RestrictedFunctions or else, the error level should be lowered to warning. Both are perfectly valid functions to use in plugins. Think: a security plugin which turns error reporting off for badly configured servers to prevent sensitive information leaking out.

  • WordPress.PHP.DiscouragedFunctions / create_function: create_function() used to be discouraged only for the VIP platform, but has now been introduced to the extra ruleset as well. IMHO it should be excluded from extra. This can be done from the ruleset.
    VIP controls the platform the code is run on and therefore encouraging PHP 5.3+ code is perfectly fine, however code which is not specifically targetted at VIP, will - more often than not - want to comply with the WP core minimum requirements. As WP core (still sigh) is not willing to drop the PHP 5.2 minimum version requirement, it would be irresponsible and misleading to encourage developers otherwise.

  • WordPress.PHP.RestrictedFunctions / section eval: 👍

  • WordPress.PHP.RestrictedFunctions / section runtime_configuration: error_reporting() - see above.

  • WordPress.PHP.RestrictedFunctions / section runtime_configuration: ini_(alter|restore|set) used to be forbidden for the VIP platform, but have now been introduced to the extra ruleset as well. IMHO these should be excluded from extra. For this to be possible the runtime_configuration section would need to be split up into two or more sections.
    There are perfectly valid reasons to use ini_set() in plugins, think: attempting to raise the PHP memory limit for a backup plugin.
    If they are to be included in the extra ruleset, the error level for these functions should be warning, not error. In that case, for VIP, the error level can be raised to error via the ruleset config.

  • WordPress.PHP.RestrictedFunctions / section system_calls: No documentation about these functions being forbidden exists AFAIK, so either documentation should be created or else, the error level should be warning.

  • WordPress.PHP.RestrictedFunctions / section obfuscation: While I agree that most of the time these functions are used for nefarious reasons, there are - again - perfectly valid reasons for using these. For instance in plugins dealing with mail, logging or image creation/conversion. So again: if these are to be added to extra, at the very least the error level should be lowered to warning.

@@ -57,7 +57,7 @@ public function getGroups() {
'split' => array(
'type' => 'error',
- 'message' => '%s has been deprecated since PHP 5.3 and removed in PHP 7.0, please use explode(), str_split() or preg_split() instead.',
+ 'message' => '%s() has been deprecated since PHP 5.3 and removed in PHP 7.0, please use explode(), str_split() or preg_split() instead.',
@jrfnl
jrfnl Dec 12, 2016 Contributor

Only one space needed after the ()

WordPress-VIP/ruleset.xml
@@ -14,6 +14,27 @@
<!-- https://vip.wordpress.com/documentation/code-review-what-we-look-for/#commented-out-code-debug-code-or-output -->
<rule ref="Squiz.PHP.CommentedOutCode" />
+ <!-- https://vip.wordpress.com/documentation/vip/code-review-what-we-look-for/#eval-and-create_function -->
+ <!--https://vip.wordpress.com/documentation/vip/code-review-what-we-look-for/#settings-alteration -->
@jrfnl
jrfnl Dec 12, 2016 Contributor

Missing a space before https

WordPress-VIP/ruleset.xml
@@ -14,6 +14,27 @@
<!-- https://vip.wordpress.com/documentation/code-review-what-we-look-for/#commented-out-code-debug-code-or-output -->
<rule ref="Squiz.PHP.CommentedOutCode" />
+ <!-- https://vip.wordpress.com/documentation/vip/code-review-what-we-look-for/#eval-and-create_function -->
+ <!--https://vip.wordpress.com/documentation/vip/code-review-what-we-look-for/#settings-alteration -->
+ <rule ref="WordPress.PHP.RestrictedFunctions" />
@jrfnl
jrfnl Dec 12, 2016 Contributor

The links justify adding this sniff for the eval and runtime_configuration sections.
Additional links to the VIP documentation should be added for the newly introduced system_calls and obfuscation sections. If these are not documented, they should be excluded from the ruleset or added to the VIP documentation.

@grappler
grappler Dec 12, 2016 Contributor

None of us can update the VIP documentation. There is a check in the VIP scanner that does the same thing that could backup including the two new sections. https://github.com/Automattic/vip-scanner/blob/master/vip-scanner/checks/ForbiddenPHPFunctionsCheck.php

WordPress-VIP/ruleset.xml
+
+ <!-- https://vip.wordpress.com/documentation/vip/code-review-what-we-look-for/#use-wp_parse_url-instead-of-parse_url -->
+ <!-- https://vip.wordpress.com/documentation/vip/code-review-what-we-look-for/#use-wp_json_encode-over-json_encode -->
+ <rule ref="WordPress.WP.AlternativeFunctions">
@jrfnl
jrfnl Dec 12, 2016 Contributor

The links justify adding this sniff for the parse_url and json_encode sections.
Additional links to the VIP documentation should be added for the newly introduced file_system_read section. If this is not documented, the section should be excluded from the ruleset or added to the VIP documentation.

WordPress-VIP/ruleset.xml
+ <properties>
+ <!-- VIP recommends other functions -->
+ <property name="exclude" value="curl,file_get_contents"/>
+ </properties>
@jrfnl
jrfnl Dec 12, 2016 Contributor

The exclude property looks to me to be superseded by the fact that the sniff now uses modular error codes.
We should probably remove (or rather deprecate) the $exclude property in the AbstractFunctionRestrictionsSniff altogether, but that's for another PR.

All the same, the property line can be replaced by the more common modular error code syntax:

		<exclude name="WordPress.WP.AlternativeFunctions.curl" />
		<exclude name="WordPress.WP.AlternativeFunctions.file_get_contents" />
+ * The checks for `parse_url()` hs been moved to the stand-alone sniff
+ * WordPress_Sniffs_WP_AlternativeFunctionsSniff.
+ * The checks for `eval()` hs been moved to the stand-alone sniff
+ * WordPress_Sniffs_PHP_RestrictedFunctionsSniff.
@jrfnl
jrfnl Dec 12, 2016 Contributor

Spelling: 2 x hs should be has
Grammer: 2x The checks for => The check for (last two lines)

@@ -37,31 +46,13 @@ public function getGroups() {
// @link https://vip.wordpress.com/documentation/vip/code-review-what-we-look-for/#switch_to_blog
'switch_to_blog' => array(
'type' => 'error',
- 'message' => '%s is not something you should ever need to do in a VIP theme context. Instead use an API (XML-RPC, REST) to interact with other sites if needed.',
+ 'message' => '%s() is not something you should ever need to do in a VIP theme context. Instead use an API (XML-RPC, REST) to interact with other sites if needed.',
@jrfnl
jrfnl Dec 12, 2016 Contributor

Consistency: You're changing the message to %s() 👍 for nearly all groups here, but missing two.
I would suggest changing the others as well to be consistent.

  • get_page_by_title
  • get_category_by_slug
// @link https://vip.wordpress.com/documentation/vip/code-review-what-we-look-for/#use-wp_safe_redirect-instead-of-wp_redirect
'wp_redirect' => array(
'type' => 'warning',
- 'message' => '%s Using wp_safe_redirect(), along with the allowed_redirect_hosts filter, can help avoid any chances of malicious redirects within code. It’s also important to remember to call exit() after a redirect so that no other unwanted code is executed.',
+ 'message' => '%s() Using wp_safe_redirect(), along with the allowed_redirect_hosts filter, can help avoid any chances of malicious redirects within code. It’s also important to remember to call exit() after a redirect so that no other unwanted code is executed.',
@jrfnl
jrfnl Dec 12, 2016 Contributor

As the line is being changed anyway, we might as well fix the other issue(s):

  • The first bit isn't in sentence form - %s() could be removed or suffixed with : or found.
  • ASCII: It’s also important => It's also important (the apostrophe is currently a Unicode character which doesn't display well in some (most?) command line interfaces)
@@ -286,49 +245,12 @@ public function getGroups() {
// @link https://vip.wordpress.com/documentation/vip/code-review-what-we-look-for/#mobile-detection
'wp_is_mobile' => array(
'type' => 'error',
- 'message' => '%s When targeting mobile visitors, jetpack_is_mobile() should be used instead of wp_is_mobile. It is more robust and works better with full page caching.',
+ 'message' => '%s() When targeting mobile visitors, jetpack_is_mobile() should be used instead of wp_is_mobile. It is more robust and works better with full page caching.',
@jrfnl
jrfnl Dec 12, 2016 Contributor

As the line is being changed anyway, we might as well fix the other issue(s):

  • The first bit isn't in sentence form - %s() could be removed or suffixed with : or found.
@@ -0,0 +1,22 @@
+<?php
+// Make sure no groups are being excluded. Shouldn't be needed, but is due to some PHPCS test weirdness.
+// @codingStandardsChangeSetting WordPress.WP.AlternativeFunctions exclude false
@jrfnl
jrfnl Dec 12, 2016 Contributor

Maybe add a note that the setting change comment can be remove once the minimum required PHPCS version has gone up to v 2.7.1 (in which this has been fixed)

+ *
+ * @var string WordPress versions.
+ */
+ public $minimum_supported_version = 4.3;
@jrfnl
jrfnl Dec 12, 2016 Contributor

Maybe add a note that this can be overruled via the ruleset (as it is a public property) and include an example of how to do this ?

Aside: Would be awesome if at some point this could be auto-detected for plugins and themes based on what is set as the minimum version in the readme file / style.css file, but that's another matter.

+ /**
+ * List of deprecated functions with alternative when available.
+ *
+ * To be updated after every major release.
@jrfnl
jrfnl Dec 12, 2016 Contributor

I would suggest to add a note which documents when the list was last updated and on which version / commit the last update was based, so the next person updating it knows where to start from.

+ *
+ * @var array
+ */
+ private $depreacted_functions = array(
@jrfnl
jrfnl Dec 12, 2016 Contributor

Spelling: depreacted => deprecated (also a number of times below in the function)

+ *
+ * @var array
+ */
+ private $depreacted_functions = array(
@jrfnl
jrfnl Dec 12, 2016 Contributor

For future consideration:
There are situations where the preferred alternative function was introduced in a later version than the version in which the original function was deprecated. In that case, it might be useful to add the version number to the alternative.
Something like:

'function_name' => array(
	'alt'     => 'alt_function() (WP 4.3+)',
	'version' => '4.2',
),
+ 'alt' => 'wp_get_attachment_image()',
+ 'version' => '2.5',
+ ),
+ 'get_attachment_innerhtml' => array(
@jrfnl
jrfnl Dec 12, 2016 Contributor

I'd suggest defining this using the same case as the function was in the WP code get_attachment_innerHTML - the abstract class handling the check already ensures that the actual comparison of the function name is done in a case-insensitive manner.

+ *
+ * @return array
+ */
+ public function getGroups() {
@jrfnl
jrfnl Dec 12, 2016 Contributor

You should initialize the $groups variable like $groups = array(); before starting the loop to prevent notices.

+ * @return array
+ */
+ public function getGroups() {
+ foreach ( $this->depreacted_functions as $depreacted_function => $data ) {
@jrfnl
jrfnl Dec 12, 2016 Contributor

Spelling ( see above) + 4 more below

+ $type = 'warning';
+ }
+ if ( empty( $data['alt'] ) ) {
+ $message = $depreacted_function . '() has been deprecated since version ' . $data['version'] . '.';
@jrfnl
jrfnl Dec 12, 2016 Contributor

Instead of $depreacted_function - use %s as the detected function name is passed to the error message function anyway.

And just to be clear that we're talking WP versions, I'd suggest changing since version to since WP both here as well as in the line below.

+ if ( empty( $data['alt'] ) ) {
+ $message = $depreacted_function . '() has been deprecated since version ' . $data['version'] . '.';
+ } else {
+ $message = $depreacted_function . '() has been deprecated since version ' . $data['version'] . '. Use ' . $data['alt'] . ' instead.';
@jrfnl
jrfnl Dec 12, 2016 Contributor

dito

+ 'type' => $type,
+ 'message' => $message,
+ 'functions' => array(
+ $depreacted_function
@jrfnl
jrfnl Dec 12, 2016 Contributor

Missing , behind the variable

+// DEPRECATED WORDPRESS FUNCTIONS.
+
+WP_Filesystem_Base::find_base_dir(); // Bad, use WP_Filesystem::abspath instead.
+WP_Filesystem_Base::get_base_dir(); // Bad, use WP_Filesystem::abspath instead.
@jrfnl
jrfnl Dec 12, 2016 Contributor

AFAICS, the above two methods are no longer sniffed for, nor tested.

They should be added somewhere, especially as they were previously already being sniffed for.
As these are methods, not functions, I can imagine we might need a separate AbstractMethodsSniff / DeprecatedMethodsSniff to deal with this or alternatively, the existing AbstractFunctionRestrictionsSniff could be adjusted to make the distinction between methods and functions.

For now - to prevent a regression - I suggest adding them as if they were functions with a comment (and accompanying issue) that this will need to be addressed later in another PR.

+get_the_attachment_link();
+get_attachment_icon_src();
+get_attachment_icon();
+get_attachment_innerhtml();
@jrfnl
jrfnl Dec 12, 2016 Contributor

I'd suggest defining this using the case same as the function was in the WP code get_attachment_innerHTML - the abstract class handling the check already takes care of the doing a case-insensitive comparison of the function name.

/**
- * Discourages the use of various functions and suggests (WordPress) alternatives.
+ * Discourages the use of various functions and suggests alternatives.
@jrfnl
jrfnl Dec 12, 2016 Contributor

various functions => various native PHP functions

+ * stand-alone sniff WordPress_Sniffs_PHP_DevelopmentFunctionsSniff.
+ * The check for the `register_globals` has been removed as there is no such
+ * function. To check for `register_globals` ini directive use
+ * PHPCompatibility_Sniffs_PHP_DeprecatedIniDirectivesSniff.
@jrfnl
jrfnl Dec 12, 2016 edited Contributor

Pointing to another standard which isn't shipped with PHPCS nor WPCS is neither here nor there.

Also: I'd suggest moving this one down.

*/
-class WordPress_Sniffs_PHP_DiscouragedFunctionsSniff extends Generic_Sniffs_PHP_ForbiddenFunctionsSniff {
+class WordPress_Sniffs_PHP_DiscouragedFunctionsSniff extends WordPress_AbstractFunctionRestrictionsSniff {
@jrfnl
jrfnl Dec 12, 2016 Contributor

This is now a completely different sniff from before, i.e. all the checks which it contained before have been moved elsewhere + it extends a different sniff as well.

(Re-)Using the file for a new sniff breaks backward compatibility in that there may well be people who have excluded this sniff in their custom ruleset based on the "old" version, but who would now inadvertently end up excluding completely different checks.

I'd suggest changing the file + class name to DiscouragedPHPFunctions to avoid this issue.
In that case, the @since would become 0.11.0 and the changelog should be removed as it can (and should) be considered a new sniff.
N.B. Don't forget that the same changes (file + class name + @since tag) need to be applied to the unit test file for this sniff too and that the rulesets and (comments in) other sniffs which refer to this sniff will need to be adjusted as well.

- 'get_attachment_innerHTML' => 'wp_get_attachment_image',
+ 'serialize' => array(
+ 'type' => 'warning',
+ 'message' => '%s() Serialized data has <a href=\'https://www.owasp.org/index.php/PHP_Object_Injection\'>known vulnerability problems</a> with Object Injection. JSON is generally a better approach for serializing data.',
@jrfnl
jrfnl Dec 12, 2016 Contributor

The output is plain text, so no HTML tags should be used in the message. The URL might be better placed in an FAQ item in the wiki maybe ?

- );
+ 'urlencode' => array(
+ 'type' => 'warning',
+ 'message' => '%s() should only be used when dealing with legacy applications rawurlencode should now be used instead. See http://php.net/manual/en/function.rawurlencode.php and http://www.faqs.org/rfcs/rfc3986.html',
@jrfnl
jrfnl Dec 12, 2016 Contributor

Consistency: rawurlencode => rawurlencode()

- );
+ 'urlencode' => array(
+ 'type' => 'warning',
+ 'message' => '%s() should only be used when dealing with legacy applications rawurlencode should now be used instead. See http://php.net/manual/en/function.rawurlencode.php and http://www.faqs.org/rfcs/rfc3986.html',
@jrfnl
jrfnl Dec 12, 2016 Contributor

The URLs might be better placed in an FAQ item in the wiki maybe ?

+ return array(
+ 'query_posts' => array(
+ 'type' => 'warning',
+ 'message' => '%s is discouraged. Use WP_Query instead.',
@jrfnl
jrfnl Dec 12, 2016 Contributor

Consistency: (nearly) everywhere else, the message has been changed to %s()

+
+ 'wp_reset_query' => array(
+ 'type' => 'warning',
+ 'message' => '%s is discouraged. Use the wp_reset_postdata instead.',
@jrfnl
jrfnl Dec 12, 2016 Contributor

Consistency: (nearly) everywhere else, the message has been changed to %s(), I'd also suggest adding the () behind the function alternative too

-// NEVER PUSH DEBUG CALLS.
-// ----------------------
+add_action( 'widgets_init', create_function( '', // Warning.
+ 'return register_widget("time_more_on_time_widget");'
@jrfnl
jrfnl Dec 12, 2016 Contributor

Lead by example: "time_more_on_time_widget" should be surrounded by spaces

@@ -1,24 +1,20 @@
<?php
@jrfnl
jrfnl Dec 12, 2016 Contributor

I'm missing unit tests for the following functions which are covered by the sniff (not directly related to the PR, but noticed it while reviewing):

  • setcookie
  • get_user_meta
  • update_user_meta
  • delete_user_meta
  • add_user_meta
-attachment_url_to_postid( $url ); // Bad.
+attachment_url_to_postid( $url ); // Error.
@jrfnl
jrfnl Dec 12, 2016 Contributor

Duplicate: it is also being tested for a few lines below.

+ public function getGroups() {
+ return array(
+ 'error_log' => array(
+ 'type' => 'error',
@jrfnl
jrfnl Dec 12, 2016 Contributor

BC-break: for the functions in this list which were previously contained within the extra ruleset (DiscouragedFunctions), the error level used to be warning. For those which used to be in the VIP ruleset (RestrictedFunctions), the error level used to be error.

IMHO there is no reason to raise the error level for the check in the extra ruleset, so the level should be set to warning. For VIP, it can be selectively raised to error via the ruleset.

+ return array(
+ 'error_log' => array(
+ 'type' => 'error',
+ 'message' => '%s() Debug code is not to be used in Production',
@jrfnl
jrfnl Dec 12, 2016 Contributor

The first bit isn't in sentence form - %s() => %s() found.

+ return array(
+ 'error_log' => array(
+ 'type' => 'error',
+ 'message' => '%s() Debug code is not to be used in Production',
@jrfnl
jrfnl Dec 12, 2016 Contributor

Also, IMHO, the message is worded a too strongly in this case. For a number of these functions there are perfectly valid reasons to use them in production. Think: setting an error handler which logs errors instead of displaying them and includes a backtrace.

+ ),
+ ),
+
+ 'prevent_path_disclosure' => array(
@jrfnl
jrfnl Dec 12, 2016 Contributor

This group is new to the extra ruleset. With that in mind and looking at the functions included, I believe this group should be moved back to the VIP.RestrictedFunctions or else, the error level should be lowered to warning (for VIP it can be changed back to error from the ruleset).

The error message itself is also worded too strongly for usage in extra as AFAICS there is no documentation that these functions are forbidden in plugins or core.

@grappler
grappler Dec 12, 2016 Contributor

I am fine with changing it to warning but I believe they should belong in the category of development functions. We could always remove it from the extra ruelset. I don't think every plugin needs to use these functions and they definitely should not be used in themes.

By having the development functions together it can be easily enabled or disabled.

@mundschenk-at
mundschenk-at Jan 14, 2017

Both trigger_error and set_error_handler would appear to me to be legitimate functions for production code (especially when integrating third-party PHP libraries).

+ ),
+
+ 'runtime_configuration' => array(
+ 'type' => 'error',
@jrfnl
jrfnl Dec 12, 2016 Contributor

This group of functions is new to the extra ruleset. No documentation about these functions being forbidden in plugins or core exists AFAICS, so either documentation should be created or else, the error level should be warning.
The message text is also too strongly worded in my opinion.

For VIP which previously already included these, the error level can be raised to error via the ruleset and if needed, the error message could also be changed from the ruleset.

+ ),
+
+ 'system_calls' => array(
+ 'type' => 'error',
@jrfnl
jrfnl Dec 12, 2016 Contributor

The message sound more like a warning than an error. Also: no documentation about these functions being forbidden exists, so either documentation should be created or else, the error level should be warning.

+
+ 'parse_url' => array(
+ 'type' => 'warning',
+ 'message' => '%s() is discouraged due to a lack for backwards-compatibility in PHP versions; use wp_parse_url() instead.',
@jrfnl
jrfnl Dec 12, 2016 Contributor

Grammer/clarity:
is discouraged due to a lack for backwards-compatibility in PHP versions =>
is discouraged because of inconsistency in the output across PHP versions

+
+ 'file_system_read' => array(
+ 'type' => 'warning',
+ 'message' => 'File operations should use the WP_Filesystem methods instead of direct PHP filesystem calls %s()',
@jrfnl
jrfnl Dec 12, 2016 Contributor

Sentence structure:
File operations should use the WP_Filesystem methods instead of direct PHP filesystem calls %s() =>
File operations should use WP_Filesystem methods instead of direct PHP filesystem calls. Found: %s()

+set_include_path(); // Error.
+restore_include_path(); // Error.
+
+// Obfuscation and evil functions.
@jrfnl
jrfnl Dec 12, 2016 Contributor

All the functions below miss the error/warning/bad indicator.

@grappler
Contributor

Thank you @jrfnl for the impressive review. The initial struggle was to work out how to separate the different checks. The reason I ended up having this huge PR is that if I was moving checks around I was missing functionality. I suppose I should have done the PR is stages. It is easier to see the structure in hindsight.

I have started to make changes according to the inline comments.

I am fine changing the notice fromerror to warning where ever as PHPCS is configurable that it can be changed in the ruleset. If the notice is changed then we will need to move the functions from RestrictedFunctions to DiscouragedFunctions.

@sboisvert @david-binda could you check that the changes being made to the VIP ruleset is OK? They should be OK but they are not fully documented. It would be good if they are OK that they are documented too. Thanks! :) See the section "VIP ruleset" in the comment by @jrfnl

@JDGrimes
Contributor

I disagree with @jrfnl regarding most of the functions that he's suggested should be removed from extra/downgraded to a warning (except for create_function, of course). While there may be perfectly valid use-cases for these functions, I don't think that is so much the point. I think that those are really special cases, that can be handled via a custom config for a project. Generally speaking, plugins that are using them are very probably _doing_it_wrong(). And in some cases hosts disable some of these functions anyway, and so as along as that is a somewhat common practice, those functions shouldn't be used without first checking if they are available.

Upon reflection, I guess maybe a warning makes some sense as opposed to an error for some of the cases, but I've always just turned all off the warnings off when I run PHPCS (I don't remember exactly why, maybe I should just stop doing this), so when I think of a warning, I think "most people will never see this or won't pay any attention to it." Which maybe is a completely wrong assumption. In which case, yeah, a warning is fine. But I still wouldn't support removing them from extra entirely.

@sboisvert
Contributor

Thanks for the amazing PR and all the work that went into it!

The one thing VIP wouldn't necessarily want to sniff for is under section obfuscation: base64_decode, base64_encode. There are enough valid use cases for base64 encoding that I don't think this should be included. As for the others under obfuscation I don't have a strong opinion but if we do leave the sniffs in I think they should be warnings. @david-binda might have other suggestions.

As a sidenote I support the suggested modifications from @jrfnl.

@grappler
Contributor
grappler commented Dec 17, 2016 edited

@jrfnl I have made all of the changes as requested. I removed the duplicate curl and file_get_contents checks and just changed the message instead.

The Unit Test is failing in PHPCS version 2.7 due to the notice being changed from warning to error. To prove this I have temporarily increased the version in the .travis.yml file. To fix this I have created a separate PR #733 which should be merged before this PR. Once PR #733 is merged I can rebase this PR and the tests should pass.

@jrfnl
jrfnl requested changes Dec 26, 2016 edited View changes

Nearly there 👍

I've reviewed the latest changes and only got a few remarks left - see inline.

And one question:
The WordPress.PHP.DiscouragedFunctions sniff has now been replaced by other sniffs.
Currently the file has been removed. I'm wondering if this could break backward-compatibility if people reference that sniff from their rulesets (will PHPCS start throwing errors because a sniff is referenced which no longer exists in the standard?).
Should the file be left in place, but with a @deprecated annotation + the list of what was moved where and no actual functionality in the sniff itself ?

+<?php
+// Make sure no groups are being excluded. Shouldn't be needed, but is due to some PHPCS test weirdness.
+// Remove once the minimum required PHPCS version goes up to v 2.7.1.
+// @codingStandardsChangeSetting WordPress.WP.AlternativeFunctions exclude false
@jrfnl
jrfnl Dec 26, 2016 Contributor

As you are no longer excluding part of the sniff from VIP, but just changing the message, this setting change is no longer needed and all three comment lines can be removed.

(I've tested it already: https://travis-ci.org/jrfnl/WordPress-Coding-Standards/builds/186780652 )

+class WordPress_Sniffs_WP_DeprecatedFunctionsSniff extends WordPress_AbstractFunctionRestrictionsSniff {
+
+ /**
+ * Mimimum WordPress version.
@jrfnl
jrfnl Dec 26, 2016 Contributor

Spelling: Mimimum => Minimum

+ * Mimimum WordPress version.
+ *
+ * This public variable allows the mimimum WordPress version to be
+ * overruled via the ruleset via a property in the phpcs.xml config file.
@jrfnl
jrfnl Dec 26, 2016 Contributor

This sentence seems a bit clucky, what about :
This sniff will throw an error when usage of deprecated functions is detected if the function was deprecated before the minimum supported WP version; a warning otherwise.
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.

+ $message = '%s() has been deprecated since WordPress version ' . $data['version'] . '.';
+ } else {
+ $message = '%s() has been deprecated since WordPress version ' . $data['version'] . '. Use ' . $data['alt'] . ' instead.';
+ }
@jrfnl
jrfnl Dec 26, 2016 Contributor

This if/else could be simplified:

$message = '%s() has been deprecated since WordPress version ' . $data['version'] . '.';
if ( ! empty( $data['alt'] ) ) {
	$message .= ' Use ' . $data['alt'] . ' instead.';
}
-urlencode(); // Warning.
-rawurlencode(); // Ok.
+
+setcookie("cookie[three]", "cookiethree"); // Waarning.
@jrfnl
jrfnl Dec 26, 2016 Contributor
  • Spelling: Warning
  • Lead by example: spaces + single quotes.
+
+ 'runtime_configuration' => array(
+ 'type' => 'warning',
+ 'message' => '%s() found. Changing configuration at runtime should not be done.',
@jrfnl
jrfnl Dec 26, 2016 edited Contributor

Error message is worded too strongly for a warning - suggest: Changing configuration at runtime is rarely necessary.

+ 'putenv',
+ 'set_include_path',
+ 'restore_include_path',
+ // This alias was DEPRECATED in PHP 5.3.0, and REMOVED as of PHP 7.0.0.
@jrfnl
jrfnl Dec 26, 2016 Contributor

Are these last three functions included here because they are deprecated/removed of because they touch the runtime_configuration ?
Should they (also) be in a separate WordPress.PHP.DeprecatedFunctions sniff ?

@grappler
grappler Dec 26, 2016 Contributor

They are included because they touch the runtime. For PHP compatibility we can use the PHPcompatibility sniffs.

+
+ 'obfuscation' => array(
+ 'type' => 'warning',
+ 'message' => '%s() can be used to obfuscate code.',
@jrfnl
jrfnl Dec 26, 2016 Contributor

This message feels incomplete. What about: %s() can be used to obfuscate code which is strongly discouraged. Please verify that the function is used for benign reasons. ? Alternative suggestions for the phrasing welcome.

WordPress-VIP/ruleset.xml
@@ -14,6 +14,41 @@
<!-- https://vip.wordpress.com/documentation/code-review-what-we-look-for/#commented-out-code-debug-code-or-output -->
<rule ref="Squiz.PHP.CommentedOutCode" />
+ <!-- https://vip.wordpress.com/documentation/vip/code-review-what-we-look-for/#eval-and-create_function -->
+ <!-- https://vip.wordpress.com/documentation/vip/code-review-what-we-look-for/#settings-alteration -->
@jrfnl
jrfnl Dec 26, 2016 Contributor

This link no longer applies to this sniff. It should be moved down to the rule inclusion of WordPress.PHP.DiscouragedPHPFunctions / WordPress.PHP.DiscouragedPHPFunctions.runtime_configuration

WordPress-VIP/ruleset.xml
+ <!-- https://vip.wordpress.com/documentation/vip/code-review-what-we-look-for/#eval-and-create_function -->
+ <!-- https://vip.wordpress.com/documentation/vip/code-review-what-we-look-for/#serializing-data -->
+ <!-- https://vip.wordpress.com/documentation/vip/code-review-what-we-look-for/#encoding-values-used-when-creating-a-url-or-passed-to-add_query_arg -->
+ <rule ref="WordPress.PHP.DiscouragedPHPFunctions" />
@jrfnl
jrfnl Dec 26, 2016 Contributor

A link to documentation is still needed to cover the system_calls section. I'm happy for it to be the link to the VIP scanner which you previously posted - https://github.com/Automattic/vip-scanner/blob/master/vip-scanner/checks/ForbiddenPHPFunctionsCheck.php

WordPress-VIP/ruleset.xml
+ <!-- https://vip.wordpress.com/documentation/vip/code-review-what-we-look-for/#serializing-data -->
+ <!-- https://vip.wordpress.com/documentation/vip/code-review-what-we-look-for/#encoding-values-used-when-creating-a-url-or-passed-to-add_query_arg -->
+ <rule ref="WordPress.PHP.DiscouragedPHPFunctions" />
+ <exclude name="WordPress.PHP.DiscouragedPHPFunctions.obfuscation" />
@jrfnl
jrfnl Dec 26, 2016 Contributor

XML syntax - I believe this should be like:

<rule ref="WordPress.PHP.DiscouragedPHPFunctions">
	<exclude name="WordPress.PHP.DiscouragedPHPFunctions.obfuscation" />
</rule>
+ <type>error</type>
+ </rule>
+
+ <!-- https://vip.wordpress.com/documentation/code-review-what-we-look-for/#commented-out-code-debug-code-or-output -->
@jrfnl
jrfnl Dec 26, 2016 Contributor

This link should probably also be added to cover the prevent_path_disclosure section: https://vip.wordpress.com/documentation/vip/code-review-what-we-look-for/#settings-alteration

'functions' => array(
'attachment_url_to_postid',
),
),
- // @link https://vip.wordpress.com/documentation/vip/code-review-what-we-look-for/#remote-calls
- 'wp_remote_get' => array(
@jrfnl
jrfnl Dec 26, 2016 Contributor

This section looks to be removed without it being brought back anywhere... Looks like a mistake ?

WordPress-Extra/ruleset.xml
+ <rule ref="WordPress.PHP.DevelopmentFunctions"/>
+ <rule ref="WordPress.PHP.DiscouragedPHPFunctions"/>
+ <!-- WP core still supports PHP 5.2+ -->
+ <exclude name="WordPress.WP.DiscouragedFunctions.create_function" />
@jrfnl
jrfnl Dec 26, 2016 Contributor

XML syntax - I believe this should be like:

<rule ref="WordPress.PHP.DiscouragedPHPFunctions">
	<!-- WP core still supports PHP 5.2+-->
	<exclude name="WordPress.WP.DiscouragedFunctions.create_function" />
</rule>
@grappler
Contributor
grappler commented Dec 26, 2016 edited

I have fixed the last few issues. The travis test is now failing as I removed the commit to update PHPCS to 2.7.1. Once #733 is merged I will rebase and then this will be ready to be merged.

EDIT: Forgot to add the blank file will do it later

@jrfnl
Contributor
jrfnl commented Dec 26, 2016 edited

@grappler Thanks! Ping me once the blank file has been added (and maybe the last open comment has been addressed too #633 (comment) ) and I'll approve the changes.

(reviewed the last batch of changes and all ok, except for ⬆️ )

+ * function. To check for `register_globals` ini directive use
+ * PHPCompatibility_Sniffs_PHP_DeprecatedIniDirectivesSniff from wimg/PHPCompatibility.
+ */
+class WordPress_Sniffs_PHP_DiscouragedFunctionsSniff extends WordPress_AbstractFunctionRestrictionsSniff {
@jrfnl
jrfnl Dec 26, 2016 edited Contributor

If you remove the extends WordPress_AbstractFunctionRestrictionsSniff, you can remove everything else in the class too, making it even clearer that it's deprecated.

@jrfnl
jrfnl approved these changes Dec 26, 2016 View changes
@grappler
Contributor

Thank you @jrfnl !

@jrfnl jrfnl modified the milestone: 0.11.0 Dec 28, 2016
@jrfnl
Contributor
jrfnl commented Dec 29, 2016

@grappler As #733 has been merged, could you please rebase the PR to allow the unit tests to pass & solve the merge conflicts caused by #752 (my apologies!) ? Thanks 😘

@grappler
Contributor

Updated Summary of the PR

extra ruleset

The following "functions" previously being sniffed for have been removed:

  • register_globals (ini directive)

The following sniffs has been deprecated and the checks moved to other sniffs.

WordPress.PHP.DiscouragedFunctions
  • The checks for the PHP development functions have been replaced by the stand-alone sniff WordPress_Sniffs_PHP_DevelopmentFunctionsSniff.
  • The checks for the WP deprecated functions have been replaced by the stand-alone sniff WordPress_Sniffs_WP_DeprecatedFunctionsSniff.
  • The checks for the PHP functions which have a WP alternative has been replaced by the stand-alone sniff WordPress_Sniffs_WP_AlternativeFunctionsSniff.
  • The checks for the WP discouraged functions have been replaced by the stand-alone sniff WordPress_Sniffs_WP_DiscouragedFunctionsSniff.
  • The checks for the PHP discouraged functions have been replaced by the stand-alone sniff WordPress_Sniffs_WP_DiscouragedPHPFunctionsSniff.
  • The check for the register_globals has been removed as there is no such function. To check for register_globals ini directive use PHPCompatibility_Sniffs_PHP_DeprecatedIniDirectivesSniff from wimg/PHPCompatibility.

The following functions which were previously not sniffed for are being newly introduced into the extra ruleset:

  • WordPress.PHP.DevelopmentFunctions

    • section error_log: error_log(), trigger_error(), set_error_handler(), debug_backtrace(), wp_debug_backtrace_summary()
    • section prevent_path_disclosure: error_reporting(), phpinfo()
  • WordPress.PHP.DiscouragedPHPFunctions

    • create_function() - Excluded from the Extra ruleset
    • serialize(), unserialize()
    • urlencode()
  • WordPress.PHP.RestrictedFunctions

    • section eval: eval()
    • section runtime_configuration: dl, error_reporting, ini_alter, ini_restore, ini_set, magic_quotes_runtime, set_magic_quotes_runtime, apache_setenv, putenv, set_include_path, restore_include_path
    • section system_calls: exec, passthru, proc_open, shell_exec, system, popen
    • section obfuscation: base64_decode, base64_encode, convert_uudecode, convert_uuencode, str_rot13
  • WordPress.WP.DeprecatedFunctions

    • Lots of new functions, too many to list. By default, it is set to presume that a project will support the current WP version and up to three releases before.
  • WordPress.WP.AlternativeFunctions

    • curl_*()
    • parse_url()
    • file_get_contents()
    • section file_system_read: readfile, fopen, fsockopen, pfsockopen, fclose, fread, fwrite

VIP ruleset

The following functions which were previously not sniffed for are being newly introduced into the VIP ruleset:

  • WordPress.PHP.DevelopmentFunctions

    • section error_log: var_export(), debug_backtrace(), debug_print_backtrace(), wp_debug_backtrace_summary()
  • WordPress.PHP.RestrictedFunctions

    • section system_calls: exec, passthru, proc_open, shell_exec, system, popen
  • WordPress.WP.AlternativeFunctions

    • json_encode()
    • section file_system_read: readfile, fopen, fsockopen, pfsockopen, fclose, fread, fwrite

Changes to WordPress.VIP.RestrictedFunctions

  • The checks for create_function(), serialize()/unserialize() and urlencode have been moved to the stand-alone sniff WordPress_Sniffs_PHP_DiscouragedPHPFunctionsSniff.
    The checks for PHP developer functions, error_reporting and phpinfohave moved to the stand-alone sniff WordPress_Sniffs_PHP_DevelopmentFunctionsSnif.
  • The check for parse_url() and curl_* have been moved to the stand-alone WordPress_Sniffs_WP_AlternativeFunctionsSniff.
  • The check for eval() has been moved to the stand-alone sniff WordPress_Sniffs_PHP_RestrictedFunctionsSniff.

Also note that the WordPress.WP.DeprecatedFunctions sniff has not been added to the VIP ruleset.

@jrfnl
Contributor
jrfnl commented Dec 29, 2016

Anyone else still wants to have a look at this before merging ? Please let me know, if not, I'll merge this tomorrow.

@jrfnl jrfnl merged commit a92b51b into WordPress-Coding-Standards:develop Dec 30, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@grappler grappler deleted the grappler:feature/discouraged-functions branch Dec 31, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment