[Update] Documentation for sniff WordPress.PHP.DiscouragedPHPFunctions #2584
Conversation
|
Added more details with code obfuscation examples in the final standard. |
jrfnl
changed the title
rodrigoprimo
added
Status: Awaiting feedback
and removed
Status: Awaiting feedback
labels
There was a problem hiding this comment.
Thanks for creating this PR, @jasonkenison!
I left some comments with suggestions. Let me know if you have questions.
| </code> | ||
| <code title="Invalid: Changing configuration at runtime"> | ||
| <![CDATA[ | ||
| error_reporting( 0 ); |
There was a problem hiding this comment.
This block and most of the blocks below are missing <em> tags.
| <code title="Valid: Using JSON for serialized data."> | ||
| <![CDATA[ | ||
| $serialized = <em>json_encode</em>( $array ); | ||
| $serialized = <em>wp_json_encode</em>( $array ); |
There was a problem hiding this comment.
I think that just a single valid example encoding $array is enough. I don't see a reason for two. Maybe keep this line and remove the one above?
| ]]> | ||
| </standard> | ||
| <code_comparison> | ||
| <code title="Valid: Using JSON for serialized data."> |
There was a problem hiding this comment.
I don't have a specific suggestion, but I think this title can be improved. Maybe Using JSON to encode data or JSON encoding data. As a non-English native speaker, I'm not sure, but I don't think the examples below are about Using JSON for serialized data as the data is not necessarily serialized before it is passed to the JSON functions. Makes sense?
| <code_comparison> | ||
| <code title="Valid: Using functions for benign reasons."> | ||
| <![CDATA[ | ||
| base64_encode( $string ); |
There was a problem hiding this comment.
Please double-check, but I believe the examples provided here actually trigger the sniff and thus are invalid. If I'm correct, the sniff triggers a warning if one of those functions is used, regardless of how it is used. It is up to a developer to determine if the function is used for benign purposes if they want to keep it. Let me know if you need help testing the sniff.
| </code> | ||
| <code title="Invalid: Changing configuration at runtime"> | ||
| <![CDATA[ | ||
| error_reporting( 0 ); |
There was a problem hiding this comment.
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?
| <code title="Invalid: Using serialized data strings."> | ||
| <![CDATA[ | ||
| $serialized = <em>serialize</em>( $array ); | ||
| $unserialized = <em>unserialize</em>( $array ); |
There was a problem hiding this comment.
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.
| </code_comparison> | ||
| <standard> | ||
| <![CDATA[ | ||
| URLs should now be encoded using rawurlencode(). Only legacy applications should use urlencode(). |
There was a problem hiding this comment.
Maybe it is not necessary to include the second sentence? The sniff will trigger anyway if it finds urlencode(). If there is a reason to include the second sentence, maybe it makes sense to add another sentence explaining what determines the cases where legacy applications should use urlencode(). I don't remember off the top of my head.
| > | ||
| <standard> | ||
| <![CDATA[ | ||
| Use JSON instead of serialized data, which has known vulnerability problems with object injection. |
There was a problem hiding this comment.
Here and in all other <standard> blocks, the description should be indented four spaces.
| Use JSON instead of serialized data, which has known vulnerability problems with object injection. | |
| Use JSON instead of serialized data, which has known vulnerability problems with object injection. |
| // Configuration not changed at runtime. | ||
| ]]> | ||
| </code> | ||
| <code title="Invalid: Changing configuration at runtime"> |
There was a problem hiding this comment.
Missing period
| <code title="Invalid: Changing configuration at runtime"> | |
| <code title="Invalid: Changing configuration at runtime."> |
| </code_comparison> | ||
| <standard> | ||
| <
Related to #1722
Continuing requested edits from #2494
Updated indentation, added tags and a line break at the end of the file.
Closes #2494