Update needed to the WP global variables list

jrfnl commented 7 years ago

As discussed in https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/pull/904#pullrequestreview-30382018, the list with WP global variables needs to be updated.

The list is currently used by two sniffs:

Variables.GlobalVariables to check for overwriting WordPress native global variables.
NamingConventions.PrefixAllGlobals - to verifies whether a prefix is needed for variables.

For the NamingConventions.PrefixAllGlobals sniff the list needs to be complete. For the Variables.GlobalVariables we'll need to allow for a number of variables which plugins/themes are allowed to override.

we should probably have a small $overrule_allowed list in the Global Var Override sniff and do an array_diff() to prime the list for usage in that sniff.

@JDGrimes has created an initial diff between WP core and the list currently in WPCS which is a good starting point:

I've just checked over trunk and come up with a new list of WordPress globals, and there are some changes from the old list: https://www.diffchecker.com/jOhQZYHl

But take note of:

While searching for global and $GLOBALS gives us a good starting point, any variable created in the global namespace and not unset should be on the list.

Some more references about WP global vars (which may well be out of date):

mundschenk-at commented 5 years ago

Not sure if this is the right ticket, but $wp_cockneyreplace should be on the whitelist to be overridden (because that's what it's for). Of course this might be more or less dead code, I've not seen a plugin in the wild that actually puts something in that global (only some that honor it in the same way as wptexturize does).

dingo-d commented 5 years ago

Hi! I just got the report of a false positive for the core the_content string in the Theme Sniffer repo: https://github.com/WPTRT/theme-sniffer/issues/104

Error msg: Hook names invoked by a theme/plugin should start with the theme/plugin prefix. Found: "the_content".

Example code:

/**
 * Display post content.
 *
 * @since 1.0.0
 *
 * @param str $style  Current display post style.
 */
public function excerpt( $style ) {

    // Short circuit filter.
    $check = apply_filters( 'bayleaf_display_posts_excerpt', false, $style );
    if ( false !== $check ) {
        return;
    }

    $text = get_the_content( '' );
    $text = wp_strip_all_tags( strip_shortcodes( $text ) );

    /** This filter is documented in wp-includes/post-template.php */
    $text = apply_filters( 'the_content', $text );
    $text = str_replace( ']]>', ']]&gt;', $text );

    /**
     * Filters the number of words in an excerpt.
     *
     * @since 1.0.0
     *
     * @param int $number The number of words. Default 55.
     */
    $excerpt_length = apply_filters( 'bayleaf_dp_excerpt_length', 55, $style );

    // Generate excerpt teaser text and link.
    $exrpt_url   = esc_url( get_permalink() );
    $exrpt_text  = esc_html__( 'Continue Reading', 'bayleaf' );
    $exrpt_title = get_the_title();

    if ( 0 === strlen( $exrpt_title ) ) {
        $screen_reader = '';
    } else {
        $screen_reader = sprintf( '<span class="screen-reader-text">%s</span>', $exrpt_title );
    }

    $excerpt_teaser = sprintf( '<p class="dp-link-more"><a class="dp-more-link" href="%1$s">%2$s &rarr; %3$s</a></p>', $exrpt_url, $exrpt_text, $screen_reader );

    /**
     * Filters the string in the "more" link displayed after a trimmed excerpt.
     *
     * @since 1.0.0
     *
     * @param string $more_string The string shown within the more link.
     */
    $excerpt_more = apply_filters( 'bayleaf_dp_excerpt_more', ' ' . $excerpt_teaser, $style );
    $text         = wp_trim_words( $text, $excerpt_length, $excerpt_more );

    printf( '<div class="dp-excerpt">%s</div>', $text ); // WPCS xss ok.
}

The code that triggers it is $text = apply_filters( 'the_content', $text );.

Should this be added in the $whitelisted_core_hooks property? If so I can make a PR 🙂

mundschenk-at commented 5 years ago

@dingo-d I think this the wrong ticket for the filter hooks issue (it is about certain unprefixed global variables not being recognized, not the PrefixAllGlobals sniff in general).

dingo-d commented 5 years ago

This seemed like an appropriate ticket, as I didn't want to open a new one for no reason. I can move it to some other ticket :)

jrfnl commented 5 years ago

@dingo-d @mundschenk-at is right, this is not the correct ticket. This ticket is about the global variables list being out of date, your issue is about WP hooks.

dingo-d commented 5 years ago

Would issue 1043 be a better fit for this? I can delete the comment and move it there, or open a new ticket for it

jrfnl commented 5 years ago

@dingo-d No as that issue is still about variables, not hooks. Issue #1139 looks like the right issue, though it was previously closed.

jrfnl commented 4 years ago

Not sure if this is the right ticket, but $wp_cockneyreplace should be on the whitelist to be overridden (because that's what it's for).

@mundschenk-at PR #1773 should take care of that.

jrfnl commented 4 years ago

Ok, so I've made an attempt to retrieve the info needed to update this list.

I've basically made some small adjustments to the GlobalVariablesOverride sniff to allow it to generate the list and ran it over the Core src directory, excluding wp-content, like so: phpcs -p ./src/ --report=full,summary,source,info --standard=WordPress --extensions=php --ignore=*/src/wp-content/* --sniffs=WordPress.WP.GlobalVariablesOverride --basepath=./

Here are the results: 20190723-global-variables-list.txt

And this is the diff: https://www.diffchecker.com/4MKEDHNv

The results are discouraging....

825 variables which are currently not in our list.
23 variables which appear to no longer be declared by Core.

If anyone wants to help by doing some spot checks and verifying the list, that would be absolutely fabulous!

jrfnl commented 4 years ago

Some minor checks I've done myself for variables which appear to be no longer declared by Core:

wp_user_roles should not be removed from the list. While WP Core does not set it, it does check it in wp-includes/class-wp-roles.php. This is basically another one which should be whitelisted for the GlobalVariablesOverride sniff as it appears this is intended to be set by a (custom) plugin.
super_admins - same thing - see: https://developer.wordpress.org/reference/functions/get_super_admins/
avail_post_mime_types highlights a bug in the GlobalVariablesOverride list as it is set via the list() construct which that sniff does not take into account yet. I opened an issue for that #1774.
merged_filters - no longer in the codebase, can be removed.
opml - mentioned in the wp-admin/link_parse_opml.php file, but does not appear to be set anywhere. Will need further investigation as the call to xml_parse() at the bottom of the file does seem to use it.
post_default_category - appears to be used, but never set: https://developer.wordpress.org/reference/functions/xmlrpc_getpostcategory/

So, just based on these initial checks, I think a more thorough check of the new list is warranted.

jrfnl commented 4 years ago

Ok, updated diff which now takes assignments via list() into account: https://www.diffchecker.com/21CiRD0n

Summary:

837 variables which are currently not in our list.
24 variables which appear to no longer be declared by Core.

List still needs further verification. Any help with this would be greatly appreciated.

jrfnl commented 1 year ago

It would be really valuable if this action would finally get executed. If anyone wants to validate the list, give me a shout and I can generate an updated version.

WordPress / WordPress-Coding-Standards

Update needed to the WP global variables list #924