We should avoid Superglobals when possible

[wonderboymusic]

We can probably add some helper functions that complete common tasks around Superglobal access

Examples of accessing here:
​https://codeclimate.com/github/WordPress/WordPress/wp-admin/edit-comments.php

Something like wp_verify_action( $action ) could replace the many instances of things like:
isset( $_REQUEST['action'] ) && 'upload-attachment' == $_REQUEST['action']

Without having to architect something like Symfony/HttpFoundation, we can make accessing them more rare.

[wonderboymusic]

This is also to work around the fact that we can't rely on filter_input( INPUT_GET, ... ) et al

[wonderboymusic]

33837.diff​ introduces wp_validate_action() and gets rid of 27 uses of $_REQUEST (14 now, down from 41) in wp-admin, ground zero for this

[ocean90]

Can we name it wp_validate_request_action() to avoid confusion with actions/hooks? Also what's the benefit of this change since we're still accessing/using the superglobal?

[wonderboymusic]

In 34059:

Introduce wp_validate_action( $action = '' ), a helper function that checks $_REQUEST for action and returns it, or empty string if not present. If $action is passed, it checks to make sure they match before returning it, or an empty string. Strings are always returned to avoid returning multiple types.

Implementing this removes 27 uses of direct superglobal access in the admin.

For more reading:
​https://codeclimate.com/github/WordPress/WordPress/wp-admin/edit-comments.php

See #33837.

[wonderboymusic]

Replying to ocean90:

Also what's the benefit of this change since we're still accessing/using the superglobal?

Every PHP analysis tool complains about superglobal access - basically means that access is supposed to be wrapped up in a framework. Doesn't mean they're Never accessed, just rarely. So, being present in the function counts as 1, and removes 27, which is better.

[jdgrimes]

One place that superglobals often have to be accessed is around nonce verification. FWIW, I have ​a helper function for that in one of my plugins.

[kitchin]

The commit calls it wp_validate_action() but it's really more of a wp_raw_action(). Here's a followup patch for less important stuff. I didn't rename the function, instead I addressed other little issues, and simplified !(empty()) for a string. See no reason to copy the potentially big $_REQUEST array to a new array, unless PHP compilation is smart about it.

[kitchin]

More stringy. Not tested.

[afercia]

Replying to ocean90:

Can we name it wp_validate_request_action() to avoid confusion with actions/hooks?

Makes sense :) Maybe worth considering before it's too late.

[kitchin]

It does not validate though (with the default argument). So maybe wp_raw_request_action().

[johnbillion]

It'd be nice if changes like this soaked for longer than an hour between the ticket being opened and the change being made.

Issues:

• wp_validate_action() is a very confusing function name because it doesn't really validate anything, especially if the $action parameter isn't passed, and it doesn't deal with WordPress actions.
• Why is an empty string returned on failure instead of boolean false?

[dd32]

Even if static analysis tools complain, I feel that the function should at a minimum not create a copy of $_REQUEST just to avoid their warnings, it feels and looks worse than using the super globals directly for what they're designed for.

I'd have probably suggested going with a simple get admin request action method instead, and used if ( 'adduser' == get admin request action ).

[kitchin]

I'm working on a follow-up patch. The original commit has one actual bug:

-		if ( isset( $_REQUEST['action'] ) && isset( $_REQUEST['delete_tags'] ) && ( 'delete' == $_REQUEST['action'] || 'delete' == $_REQUEST['action2'] ) )
+		$action = wp_validate_action();
+		if ( $action && isset( $_REQUEST['delete_tags'] ) && ( 'delete' == $action || 'delete' == $_REQUEST['action2'] ) )

The isset( $_REQUEST['action'] ) is just there to detect the first bulk <select>. It can be empty, as long as the second <select> is truthy.

[kitchin]

Stringy or null. Passes unit tests fwiw. Fixes bulk-delete bug in commit. Renames function, makes it more general.

[wonderboymusic]

the conversation is great - I am married to none of this. a bunch of us have commit: change the name of the func, change the internal bits, revert the change - all of it can change. Let's move forward.

[kitchin]

I'm bumping the severity up because the commit disabled the second bulk drop-down, see comment:13. Need to revert or fix.

[SergeyBiryukov]

Replying to kitchin:

I'm working on a follow-up patch. The original commit has one actual bug:

-		if ( isset( $_REQUEST['action'] ) && isset( $_REQUEST['delete_tags'] ) && ( 'delete' == $_REQUEST['action'] || 'delete' == $_REQUEST['action2'] ) )
+		$action = wp_validate_action();
+		if ( $action && isset( $_REQUEST['delete_tags'] ) && ( 'delete' == $action || 'delete' == $_REQUEST['action2'] ) )

The isset( $_REQUEST['action'] ) is just there to detect the first bulk <select>. It can be empty, as long as the second <select> is truthy.

It's not empty though, the default value is -1. I could not reproduce the issue,

I would suggest wp_is_request_value( $field, $value = null ) for the function name. Otherwise, 33837.stringish.diff​ looks good to me.

[wonderboymusic]

why not wp_current_action(), which is just like WP_List_Table::current_action()?

[SergeyBiryukov]

Replying to wonderboymusic:

why not wp_current_action(), which is just like WP_List_Table::current_action()?

Too similar to current_action() or current_filter().

[kitchin]

Then wp_current_request_action? I'm cooking up a patch, modulo the name.

And d'oh on -1! Still better not to trust the user agent screen reader, form filler, whatever though.

[kitchin]

Ack, 33837.stringish.diff is the best I have for behavior. I looked at making it more like WP_List_Table::current_action() in how it works, but the goal is to replace $_REQUEST generally, so this still seems best to me, whatever the name:

function wp_raw_request_value( $field, $value = null ) {
	if ( is_string( $field ) && isset( $_REQUEST[ $field ] ) ) {
		$raw = $_REQUEST[ $field ];
		if ( is_string( $raw ) ) {
			if ( $value !== null ) {
				return $value === $raw;
			}
			return $raw;
		}
	}
}

[kitchin]

By the way, the committed name, wp_validate_action was mentioned in "Week in Core: Aug. 31 - Sept. 12, 2015." Somebody wrap this thing up! Despite -1 in <select>, the commit is not quite right in my opinion because it depends on user agents doing things right. Patch 33837.stringish.diff is ready to go if you want it and still applied to trunk last time I tried with svn.

Then we can open part 2. Look how much fun we'll have with these:

wp-admin/edit.php (1 hit)
	$done = bulk_edit_posts($_REQUEST);
wp-admin/includes/ajax-actions.php (1 hit)
	_wp_ajax_menu_quick_search( $_POST );
wp-admin/includes/bookmark.php (2 hits)
	return wp_update_link( $_POST );
	return wp_insert_link( $_POST );
wp-admin/includes/class-wp-media-list-table.php (1 hit)
	list( $post_mime_types, $avail_post_mime_types ) = wp_edit_attachments_query( $_REQUEST );
wp-admin/includes/post.php (1 hit)
	$post_ID = wp_insert_post( $_POST );

[helen]

I'm reverting this until there's been more discussion about what the right approach is for us now and moving forward. Naming it wp_validate_action() is a definite no.

[helen]

In 34265:

Superglobals: Revert [34059] until further notice.

see #33837.

[westi]

Replying to helen:

I'm reverting this until there's been more discussion about what the right approach is for us now and moving forward. Naming it wp_validate_action() is a definite no.

Personally I think wrapping up superglobals like this is over obfuscation of functionality, it just makes the code more complex not less complex.

[wonderboymusic]

I'm going back to the drawing board here. Don't want to waste everyone's cycles on such an arbitrary issue.

[Mrasnika]

Almost 10 years later, perhaps we can have a fresh look at this again. Perhaps the issues around near-naming collisions can be addressed using a class with static methods, or with namespaces?