Add GlobalVariablesOverride standard doc #2586
Conversation
There was a problem hiding this comment.
Thanks for working on this, @paulopmt1!
I left a few comments. Please let me know if you have any questions.
| <code title="Invalid: Global variable overridden."> | ||
| <![CDATA[ | ||
| <em>global $wp_query;</em> | ||
| <em>$wp_query = new WP_Query( $args );</em> |
There was a problem hiding this comment.
For this code example, I would suggest highlighting with the <em> tag only the $wp_query variable instead of both lines of the code example.
| <code_comparison> | ||
| <code title="Valid: Different variable name."> | ||
| <![CDATA[ | ||
| $my_query = new WP_Query( $args ); |
There was a problem hiding this comment.
Following the pattern that I suggested below, I think that here you can highlight the $my_query variable. Typically, the <em> tags are used to highlight both the "dos" as well as the "don'ts".
| <code_comparison> | ||
| <code title="Valid: Custom Superglobal variable."> | ||
| <![CDATA[ | ||
| $GLOBALS['my_data'] = array( 'key' => 'value' ); |
There was a problem hiding this comment.
See my remarks above regarding the <em> tag.
| the `global` statement within functions, variables assigned via the `$GLOBALS` superglobal array, | ||
| and variables assigned in the global namespace. | ||
|
|
||
| A few select WordPress global variables like `$content_width` and `$wp_cockneyreplace` are |
There was a problem hiding this comment.
Since those are the two only WP global variables that can be overriden, I think this paragraph could be rephrased to remove the "like".
| > | ||
| <standard> | ||
| <![CDATA[ | ||
| WordPress global variables should not be overridden. This includes variables imported with |
There was a problem hiding this comment.
In my opinion, just saying "WordPress global variables should not be overridden." is enough and second phrase is not necessary, but other reviewers might disagree with me, so feel free to leave it for now if you prefer.
| </code> | ||
| </code_comparison> | ||
| <code_comparison> | ||
| <code title="Valid: Custom Superglobal variable."> |
There was a problem hiding this comment.
The superglobal is $GLOBALS and not $GLOBALS['my_data']. I believe the titles in this code_comparison block need to be rephrased. Alternatively, I suggest considering using just a single code_comparison block and presenting two code examples. One with a global override using global $wp_variable_name and another usigin $GLOBALS['wp_variable_name'].
| <code_comparison> | ||
| <code title="Valid: Custom Superglobal variable."> | ||
| <![CDATA[ | ||
| $GLOBALS['my_data'] = array( 'key' => 'value' ); |
There was a problem hiding this comment.
This line has 49 characters and the maximum is 48.
There was a problem hiding this comment.
Thanks for addressing the points in my first review, @paulopmt1! I think this is much better now, I left three more small comments with a few things that occurred to me only after checking the new version of the documentation.
| <em>$GLOBALS['my_data']</em> = "some data"; | ||
| ]]> | ||
| </code> | ||
| <code title="Invalid: Global variable overridden."> |
There was a problem hiding this comment.
| <code title="Invalid: Global variable overridden."> | |
| <code title="Invalid: WordPress global variable overridden."> |
I think it is important to mention that this is about WordPress globals and not any globals.
| ]]> | ||
| </standard> | ||
| <code_comparison> | ||
| <code title="Valid: Different variable name."> |
There was a problem hiding this comment.
I'm not sure about this title. Maybe Valid: Using custom variable/key names instead of WordPress globals. or Valid: Using custom variable/key names. is a bit better and more descriptive? Happy to hear other suggestions you might have.
The concern that I have with Valid: Different variable name. is that it reads a bit vaguely and doesn't clearly communicate what makes the examples below valid.
| <em>global $wp_query;</em> | ||
| <em>$wp_query</em> = new WP_Query( $args ); | ||
|
|
||
| <em>$GLOBALS['wp_query']</em> = new WP_Query( $args ); |
There was a problem hiding this comment.
| <em>$GLOBALS['wp_query']</em> = new WP_Query( $args ); | |
| <em>$GLOBALS['post']</em> = get_post( 1 ); |
Nitpick: maybe use a different global here to illustrate one more WP global that is flagged by this sniff?
Related to #1722
We're adding a new xml standard file to GlobalVariablesOverrideSniff