WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 8 months ago

Last modified 7 months ago

#19347 closed enhancement (wontfix)

Ugly alert: false !== strpos( $which_one_was_needle, $this_one )

Reported by: nbachiyski Owned by: westi
Milestone: Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch westi-likes 3.5-early
Focuses: Cc:

Description

In WordPress strpos() is used hundreds of times. Very large percentage of those calls are trying to accomplish one of these 3 goals:

  • Check if a string starts with another string
  • Check if a string contains another string
  • Check if a string ends with another string

The code for doing these 3 things is not very readable, ugly, and error-prone:

0 === strpos( $one_string, $other_string )
false !== strpos( $one_string, $other_string )
strpos( $one_string, $other_string ) == ( strlen( $one_string ) - strlen( $other_string )

I propose we introduce functions to hide the ugliness and the complexity behind these three common operations:

  • wp_startswith( $haystack, $needle )
  • wp_in( $needle, $haystack )
  • wp_endswith( $haystack, $needle )

The logic of the arguments order is consistent and easy to remember:

first argument verb second argument.

  • everything starts with a beginning translates to wp_startswith( "everything", "a beginning" )
  • goat is in distress translates to wp_in( "goat", "distress" )
  • movie ends with happy end translates to wp_endswith( "movie", "happy end" )

These three functions are happily used in GlotPress and WordPress.com.

The implementations are attached.

Attachments (1)

string-helpers.php (752 bytes) - added by nbachiyski 4 years ago.

Download all attachments as: .zip

Change History (18)

@nbachiyski4 years ago

comment:1 @westi4 years ago

  • Keywords westi-likes 3.4-early added
  • Milestone changed from Awaiting Review to Future Release
  • Owner set to westi
  • Status changed from new to reviewing

comment:2 follow-up: @nacin4 years ago

Remembering haystack/needle order is a pain and something that all still venture to php.net for on a regular basis.

However, wp_in() still ends up being reversed from startswith() and endswith(). Sure, it's natural with the whole verb thing, but it requires one to switch contexts and think about it a little more than "oh, the haystack is always going to be first." So I don't know how much I like this. I feel like adding new functions on top of this will only end up making it more difficult to follow code, not less.

comment:3 @merty4 years ago

What about renaming wp_in() to wp_has(), so we can both use the word in the function and not cause any confusions?

/**
 * Checks if $haystack has $needle
 * 
 * @param string $haystack
 * @param string $needle
 * @return bool
 */
function wp_has( $haystack, $needle ) {
	return false !== strpos( $haystack, $needle );
}

comment:4 in reply to: ↑ 2 ; follow-up: @johnbillion4 years ago

Replying to nacin:

Remembering haystack/needle order is a pain and something that all still venture to php.net for on a regular basis.

For array functions and string functions, there is actually consistency. Once you've successfully recalled the correct function name, that is.

However, wp_in() still ends up being reversed from startswith() and endswith().

These are all string functions so they should accept $haystack, $needle. merty's suggestion of wp_has() works, as does wp_contains().

comment:5 @daniloercoli4 years ago

strpos( $one_string, $other_string ) == ( strlen( $one_string ) - strlen( $other_string )

The code above doesn't work if there are multiple occurrences of '$other_string' in '$one_string'. I think the best way to test the 'EndWith' condition is using something like the code below:

strpos(  strrev( $one_string  ), strrev( $other_string  )  ) ===  0 

comment:6 @daniloercoli4 years ago

  • Cc ercoli@… added

comment:7 in reply to: ↑ 4 @merty4 years ago

These are all string functions so they should accept $haystack, $needle. merty's suggestion of wp_has() works, as does wp_contains().

Yes, "contains" is so much better. "has" sounds like it's an array-related thing.

comment:8 @mitchoyoshitaka4 years ago

  • Cc mitcho@… added

+1 to all of this. wp_contains and standardizing on $haystack, $needle makes sense to me.

comment:9 @hd-J4 years ago

  • Cc jeremy@… added

+1 to this. And I'd personnally vote for wp_contains instead of wp_has.

comment:10 @nbachiyski4 years ago

For me wp_in was good enough, because consistent narrative naming has a lot higher priority in my brain compared to consistent argument. And it is much shorter.

But I can see your point and I agree wp_contains() is good.

comment:11 follow-up: @nacin3 years ago

I don't actually see the true usefulness in these wrappers. Before committing there should be a patch against core instances of strpos(), to evaluate it.

comment:12 @tollmanz3 years ago

  • Cc zack@… added

comment:13 in reply to: ↑ 11 @nbachiyski3 years ago

Replying to nacin:

I don't actually see the true usefulness in these wrappers. Before committing there should be a patch against core instances of strpos(), to evaluate it.

Nacin, if you commit it, I will make sure to patch everything :-)

comment:14 @nbachiyski3 years ago

  • Keywords 3.5-early added; 3.4-early removed

comment:15 @nbachiyski3 years ago

And I'd like to propose another a last-minute change. In the spirit of the WordPress naming scheme, I think the best names would be:

  • wp_starts_with()
  • wp_ends_with()
  • wp_contains()

The arguments and their order should be obvious.

comment:16 @wonderboymusic8 months ago

  • Milestone Future Release deleted
  • Resolution set to maybelater
  • Status changed from reviewing to closed

I think we are good without this, correct me if I am wrong

comment:17 @johnbillion7 months ago

  • Resolution changed from maybelater to wontfix

Agreed

Note: See TracTickets for help on using tickets.