Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#32786 closed enhancement (fixed)

In get_edit_term_link: $term validation

Reported by: tmatsuur's profile tmatsuur Owned by: boonebgorges's profile boonebgorges
Milestone: 4.3 Priority: normal
Severity: normal Version:
Component: Taxonomy Keywords: has-patch
Focuses: Cc:

Description

$term_id is sometimes invalid, so isn't an error checking necessary?

Proposal:

$term = get_term( $term_id, $taxonomy );
if ( is_wp_error() )
	return;

Attachments (2)

32786.diff (659 bytes) - added by MikeHansenMe 9 years ago.
32786.2.diff (2.2 KB) - added by MikeHansenMe 9 years ago.

Download all attachments as: .zip

Change History (9)

@MikeHansenMe
9 years ago

#1 @MikeHansenMe
9 years ago

  • Keywords has-patch added

32786.diff checks term for WP_Error or null which are the 2 values get_term can return if it is not valid.

#2 @tmatsuur
9 years ago

Thank you for reply.
This has been careless. I think that your patch is better.

#3 @boonebgorges
9 years ago

  • Keywords needs-unit-tests added

Pretty lame that we return null from this function instead of a string, and pretty lame that we return null here but WP_Error objects in get_term_link(). We can't easily change the latter, I guess.

Can we get two unit tests demonstrating the null and WP_Error cases? Like, one that includes an empty $term and one that includes an invalid $taxonomy.

@MikeHansenMe
9 years ago

#4 @MikeHansenMe
9 years ago

  • Keywords needs-unit-tests removed

32786.2.diff has 4 unit tests.

  1. Valid Scenario
  2. Invalid Term ID
  3. Empty Term ID
  4. Bad Taxonomy

When writing the tests I realized that $tax had the same problem as $term. The fourth test checks this. The patch corrects this.

#5 @tmatsuur
9 years ago

When a consistency with source cord of the other functions is considered, isn't the next source cord appropriate?
(See: get_term_feed_link)

	if ( empty( $term ) || is_wp_error( $term ) )
		return false;

And, can't get_term be called first?

	$term = get_term( $term_id, $taxonomy );
	if ( empty( $term ) || is_wp_error( $term ) )
		return false;

	$tax = get_taxonomy( $taxonomy );
	if ( !current_user_can( $tax->cap->edit_terms ) )
		return;

I'm sorry, but I don't understand how to use the diff.
I hope that my intention is transmitted correctly.

#6 @boonebgorges
9 years ago

  • Milestone changed from Awaiting Review to 4.3
  • Version 4.2.2 deleted

Thanks all. As a rule, I think what's being encouraged here is not very good practice. Functions that are supposed to return a string (in this case, a URL) should not simply return null when bad input is provided. Ideally, they'd return a meaningful error object. But, in the absence of that, a PHP notice can be a helpful debugging tool for developers. If you're passing a bad term ID or a bad taxonomy name to get_edit_term_link(), you should know about it.

All this being said, the function is already odd in the sense that it bails with null when the cap check is not met. And we can't change the function to return WP_Error objects, as plugins (and WP itself) sometimes checks ! $link = get_edit_term_link() for failure. So, for the sake of consistency, it shouldn't do any harm to patch as suggested.

Last edited 9 years ago by boonebgorges (previous) (diff)

#7 @boonebgorges
9 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 32954:

Improve error checking in get_edit_term_link().

The function should not throw notices when an improper term or taxonomy is
passed.

Props tmatsuur, MikeHansenMe.
Fixes #32786.

Note: See TracTickets for help on using tickets.