WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 3 months ago

#33837 assigned enhancement

We should avoid Superglobals when possible

Reported by: wonderboymusic Owned by: wonderboymusic
Milestone: Priority: normal
Severity: normal Version:
Component: General Keywords: 2nd-opinion has-patch
Focuses: Cc:

Description

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.

Attachments (3)

33837.diff (8.2 KB) - added by wonderboymusic 4 years ago.
33837.string.diff (2.3 KB) - added by kitchin 4 years ago.
More stringy. Not tested.
33837.stringish.diff (9.2 KB) - added by kitchin 4 years ago.
Stringy or null. Passes unit tests fwiw. Fixes bulk-delete bug in commit. Renames function, makes it more general.

Download all attachments as: .zip

Change History (28)

#1 @wonderboymusic
4 years ago

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

@wonderboymusic
4 years ago

#2 @wonderboymusic
4 years ago

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

#3 follow-ups: @ocean90
4 years ago

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?

#4 @wonderboymusic
4 years ago

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.

#5 in reply to: ↑ 3 @wonderboymusic
4 years ago

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.

#6 @jdgrimes
4 years ago

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.

#7 @kitchin
4 years ago

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
4 years ago

More stringy. Not tested.

#8 @wonderboymusic
4 years ago

  • Owner set to wonderboymusic
  • Status changed from new to assigned

#9 in reply to: ↑ 3 @afercia
4 years ago

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.

#10 @kitchin
4 years ago

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

#11 @johnbillion
4 years ago

  • Keywords needs-patch added
  • Version trunk deleted

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?

#12 @dd32
4 years ago

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 ).

#13 follow-up: @kitchin
4 years ago

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
4 years ago

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

#14 @wonderboymusic
4 years ago

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.

#15 @kitchin
4 years ago

  • Keywords 2nd-opinion added
  • Severity changed from normal to major

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

#16 in reply to: ↑ 13 @SergeyBiryukov
4 years ago

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.

#17 follow-up: @wonderboymusic
4 years ago

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

#18 in reply to: ↑ 17 @SergeyBiryukov
4 years ago

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().

#19 @kitchin
4 years ago

  • Severity changed from major to normal

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.

#20 @kitchin
4 years ago

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;
		}
	}
}

#21 @kitchin
4 years ago

  • Keywords has-patch added; needs-patch removed

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 );

#22 follow-up: @helen
4 years ago

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.

#23 @helen
4 years ago

In 34265:

Superglobals: Revert [34059] until further notice.

see #33837.

#24 in reply to: ↑ 22 @westi
4 years ago

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.

#25 @wonderboymusic
4 years ago

  • Milestone changed from 4.4 to Future Release

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

Note: See TracTickets for help on using tickets.