Skip to content

[Update] Documentation for sniff WordPress.PHP.DiscouragedPHPFunctions #2584

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.

Sign up for GitHub

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

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

[Update] Documentation for sniff WordPress.PHP.DiscouragedPHPFunctions #2584

Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b91fb4f
adds documentation for sniff
tikifez Sep 17, 2024
d12e715
updated formatting, indentation, add <em> tags
jasonkenison Aug 26, 2025
46f08e3
update indentation, add <em>, linebreak end of document
jasonkenison Aug 26, 2025
a8fffd4
include more details in obfuscate code standard
jasonkenison Aug 26, 2025
b5a5872
requested changes
jasonkenison Sep 14, 2025
013e0d0
fix spelling error
jasonkenison Sep 14, 2025
5613680
requested updates
jasonkenison Oct 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 113 additions & 0 deletions WordPress/Docs/PHP/DiscouragedPHPFunctionsStandard.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
<?xml version="1.0"?>
<documentation xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="https://phpcsstandards.github.io/PHPCSDevTools/phpcsdocs.xsd"
title="Discouraged PHP Functions"
>
<standard>
<![CDATA[
Use JSON instead of serialized data, which has known vulnerability problems with object injection.
]]>
</standard>
<code_comparison>
<code title="Valid: Using JSON for serialized data.">
<![CDATA[
$serialized = <em>json_encode</em>( $array );
$serialized = <em>wp_json_encode</em>( $array );
$unserialized = <em>json_decode</em>( $array );
]]>
</code>
<code title="Invalid: Using serialized data strings.">
<![CDATA[
$serialized = <em>serialize</em>( $array );
$unserialized = <em>unserialize</em>( $array );
Copy link
Collaborator

@rodrigoprimo rodrigoprimo Sep 4, 2025

Choose a reason for hiding this comment

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

Maybe it is better to use a variable name other than $array here as this function takes an string as its first parameter? The same applies to the json_decode() example above.

Please check if other variables could be renamed as well for the same reason in the other examples.

Copy link
Collaborator

@rodrigoprimo rodrigoprimo Oct 27, 2025

Choose a reason for hiding this comment

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

Apologies if my previous message was not clear. I can see you changed the variable name from $array to $my_array for all the function calls in this <code_comparison> block, but that is not exactly what I meant.

I think it is fine to use $array (or $my_array) for the functions that accept an array as the first parameter (wp_json_encode() and serialize()). But I don't think we should use $array (or $my_array) for functions that don't accept an array as the first parameter (json_decode() and unserialize()). $string (or something similar) would be fine in those two cases from my perspective. Do you agree?

]]>
</code>
</code_comparison>
<standard>
<![CDATA[
URLs should now be encoded using rawurlencode(). Only legacy applications should use urlencode().
]]>
</standard>
<code_comparison>
<code title="Valid: Encoding a url using rawurlencode().">
<![CDATA[
<em>rawurlencode</em>( get_site_url() );
]]>
</code>
<code title="Invalid: Encoding a url using urlencode().">
<![CDATA[
<em>urlencode</em>( get_site_url() );
]]>
</code>
</code_comparison>
<standard>
<![CDATA[
Avoid using functions which change configuration values at runtime.
]]>
</standard>
<code_comparison>
<code title="Valid: Not changing configuration at runtime.">
<![CDATA[
// Configuration not changed at runtime.
]]>
</code>
<code title="Invalid: Changing configuration at runtime">
<![CDATA[
error_reporting( 0 );
Copy link
Collaborator

@rodrigoprimo rodrigoprimo Aug 26, 2025

Choose a reason for hiding this comment

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

This block and most of the blocks below are missing <em> tags.

Copy link
Collaborator

@rodrigoprimo rodrigoprimo Oct 27, 2025

Choose a reason for hiding this comment

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

In some cases the <em> tag is used to highlight just the function name and in other cases it is used to highlight the whole function call. Is there a reason for that? If not, I suggest highlighting just the function name in all examples.

Copy link
Collaborator

@rodrigoprimo rodrigoprimo Sep 4, 2025

Choose a reason for hiding this comment

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

I'm unsure (here and in other <code_comparison> blocks) if it is necessary to include an example for every single function that triggers the sniff. Please check other sniff XML docs if you can, but I believe it is more common to just include one example (if all the others are the same except for the function name). Then, if the list is not too long, you can consider mentioning all the functions in the corresponding <standard> description. What do you think?

Copy link
Collaborator

@rodrigoprimo rodrigoprimo Oct 27, 2025

Choose a reason for hiding this comment

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

I can see you addressed this point for most <standard> blocks except for the one related to functions used to obfuscate code. Is there a reason for that? Would it make sense to list the functions that trigger this warning in the <standard> description as you did for the <standard> blocks?

ini_restore( $option );
apache_setenv( $variable, $value );
putenv( $assignment );
set_include_path( $include_path );
restore_include_path();
magic_quotes_runtime( $new_setting );
set_magic_quotes_runtime( $new_setting );
dl( $extension_filename );
]]>
</code>
</code_comparison>
<standard>
<![CDATA[
Do not use PHP system calls. They are often disabled by server admins.
]]>
</standard>
<code_comparison>
<code title="Valid: Not using PHP system calls.">
<![CDATA[
// Avoiding using PHP system calls.
]]>
</code>
<code title="Invalid: Using PHP system calls.">
<![CDATA[
exec( $command );
passthru( $command );
proc_open( 'php', $desc, $pipes, $cwd, $env );
shell_exec( $command );
system( $command );
popen( $command, $mode );
]]>
</code>
</code_comparison>
<standard>
<![CDATA[
Functions often used for obfuscating code are strongly discouraged. Make sure the function is used for benign reasons.
]]>
</standard>
<code_comparison>
<code title="Valid: Using functions for benign reasons.">
<![CDATA[
base64_encode( $string );
base64_decode( $encrypted_string );
convert_uuencode( $string );
convert_uudecode( $encrypted_string );
str_rot13( $string );
]]>
</code>
<code title="Invalid: Using functions to obfuscate code.">
<![CDATA[
<em>eval( </em>base64_decode( $code_str )<em> )</em>;
Copy link
Collaborator

@rodrigoprimo rodrigoprimo Oct 27, 2025

Choose a reason for hiding this comment

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

I believe eval() is not flagged by the sniff so it should not be highlighted. More broadly, I'm not sure if it needs to be included as the sniff will trigger anyway for base64_decode() regardless of whether eval() is used.

<em>eval( </em>convert_uudecode( $uuencoded )<em> )</em>;
<em>eval( </em>str_rot13( $rot13_encoded )<em> )</em>;
]]>
</code>
</code_comparison>
</documentation>
Loading