Modularizing the discouraged functions

[grappler]

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. [New Sniff] Forbidden functions WPTT/WPThemeReview#9 Add Theme forbidden PHP functions sniff + unit tests. WPTT/WPThemeReview#10
• Update the unit tests to include the new functions.
• Update the rulesets with the changes

[jrfnl]

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

[grappler]

Thanks, I will rebase once it is merged.

[grappler]

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]

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]

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

[grappler]

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]

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]

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]

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]

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]

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]

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]

I have added a start for a check for deprecated based on WPTT/WPThemeReview#77

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

[JDGrimes]

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

[grappler]

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]

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