Make WordPress Core

Opened 19 months ago

Closed 17 months ago

Last modified 16 months ago

#58220 closed enhancement (fixed)

Replace usage of substr with str_starts_with and str_ends_with

Reported by: soean's profile Soean Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.3 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch commit add-to-field-guide
Focuses: performance Cc:

Description

Replace $needle === substr( $string, 0, $length ) with str_starts_with( $haystack, $needle ). $length should be the length of $needle.

Replace $needle === substr($string, $offset) where $offset is negative with str_ends_with( $haystack, $needle ). The absolute value of $offset should be the length of $needle.

It makes the code more readable and use a PHP native function ( which may have a performance benefit ).

Change History (28)

#2 follow-up: @Clorith
19 months ago

These functions were introduced with PHP 8, so before this can be properly considered, WordPress core would need a minimum PHP version of 8.0, which I think is a bit premature still, when looking at the overall PHP version stats.

Alternatively, core would need to create a polyfill for the functions, although possible, that feels like the wrong direction to me. Of course, if there's some substantial performance gains for those running on PHP 8 already, then it would be considered, but this would need to be well documented first :)

#3 in reply to: ↑ 2 @ocean90
19 months ago

  • Type changed from defect (bug) to enhancement

Replying to Clorith:

Alternatively, core would need to create a polyfill for the functions

Polyfills are already available as of [52040].

#5 @spacedmonkey
18 months ago

  • Milestone changed from Awaiting Review to 6.3

#6 @azaozz
18 months ago

  • Keywords 2nd-opinion added

This seems perhaps premature. See https://core.trac.wordpress.org/ticket/58206#comment:12 and https://core.trac.wordpress.org/ticket/58012#comment:24.

Also, would be good to get some performance testing on PHP 7.4.

#7 @azaozz
18 months ago

  • Focuses performance added

@spacedmonkey commented on PR #4395:


18 months ago
#8

@Soean There seems to be a unit test failure for php 8.1

#9 @spacedmonkey
18 months ago

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

@SergeyBiryukov

@Soean commented on PR #4395:


18 months ago
#10

@spacedmonkey I merged trunk, now the tests look good.

#11 @spacedmonkey
17 months ago

  • Keywords commit added; 2nd-opinion removed

Looks good to me. Marking as ready for commit.

joelataccelm commented on PR #4395:


17 months ago
#12

wait, how are you doing this and also still claiming to support php 7.4? these aren't supported in php 7.4 and will fatal error.

joelataccelm commented on PR #4395:


17 months ago
#13

@spacedmonkey wait, how are you doing this and also still claiming to support php 7.4? these aren't supported in php 7.4 and will fatal error.

@spacedmonkey commented on PR #4395:


17 months ago
#14

@joelataccelm A backward compatibility shim was added https://github.com/WordPress/wordpress-develop/commit/3cc8f1237a737ccf895682029afacafd7305d420 ( in WP 5.9.0 ). These are already in core in many many places. WordPress currently supports down to PHP 5.6.

joelataccelm commented on PR #4395:


17 months ago
#15

@joelataccelm A backward compatibility shim was added 3cc8f12 ( in WP 5.9.0 ). These are already in core in many many places. WordPress currently supports down to PHP 5.6.

got it, sorry. I just spotted that in the latest compat file. the version I noticed this fatal erroring on doesn't have the str_ functions in the compat file, so I just need to upgrade.

@sabernhardt commented on PR #4395:


17 months ago
#16

According to the Hello Dolly plugin page, it should be compatible with WordPress 4.6 or higher. The str_starts_with function would require either PHP8 or WP5.9.

#17 follow-up: @spacedmonkey
17 months ago

Performance testing:

Run on PHP 7.4 - 2020 theme and theme test data imported, trunk vs PR. Runned 500 times

Trunk PR
Response Time (median) 85.6 84.93
wp-before-template (median) 20.81 20.68
wp-before-template-db-queries (median} 2.99 2.97
wp-template (median) 55.52 55.22
wp-total (median) 76.42 75.98
wp-template-db-queries (median) 5.83 5.77

Blackfire profiles show a similar story. https://blackfire.io/profiles/compare/73d894ed-7057-44aa-840e-eed31d7a5085/graph

This change is a net benefit. @azaozz

#18 @SergeyBiryukov
17 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 55990:

Code Modernization: Replace usage of substr() with str_starts_with() and str_ends_with().

str_starts_with() and str_ends_with() were introduced in PHP 8.0 to perform a case-sensitive check indicating if the string to search in (haystack) begins or ends with the given substring (needle).

WordPress core includes a polyfill for these functions on PHP < 8.0 as of WordPress 5.9.

This commit uses str_starts_with() and str_ends_with() in core files where appropriate:

  • $needle === substr( $string, 0, $length ), where $length is the length of $needle, is replaced with str_starts_with( $haystack, $needle ).
  • $needle === substr( $string, $offset ), where $offset is negative and the absolute value of $offset is the length of $needle, is replaced with str_ends_with( $haystack, $needle ).

This aims to make the code more readable and consistent, as well as better aligned with modern development practices.

Follow-up to [52039], [52040], [52326], [55703], [55710], [55987], [55988].

Props Soean, spacedmonkey, Clorith, ocean90, azaozz, sabernhardt, SergeyBiryukov.
Fixes #58220.

@SergeyBiryukov commented on PR #4395:


17 months ago
#19

Thanks for the PR! Merged in r55990.

#20 @SergeyBiryukov
17 months ago

In 56014:

Code Modernization: Use str_ends_with() in a few more places.

str_ends_with() was introduced in PHP 8.0 to perform a case-sensitive check indicating if the string to search in (haystack) ends with the given substring (needle).

WordPress core includes a polyfill for str_ends_with() on PHP < 8.0 as of WordPress 5.9.

Follow-up to [55990].

See #58220.

#21 @SergeyBiryukov
17 months ago

In 56015:

General: Replace substr_compare() usage in the str_ends_with() polyfill.

This avoids a warning on PHP < 7.2.18 if haystack is an empty string:

Warning: substr_compare(): The start position cannot exceed initial string length

Follow-up to [52040], [55158], [55990], [56014].

See #58220.

#22 @SergeyBiryukov
17 months ago

In 56016:

General: Return early from str_ends_with() polyfill if both haystack and needle are empty.

Prior to PHP 7.0, substr( '', -0, 0 ) returns false instead of an empty string, so the strict comparison further in the function did not work as expected.

This commit addresses a test failure on PHP < 7.0, making the function consistently return true if both haystack and needle are an empty string.

Follow-up to [52040], [56014], [56015].

See #58220.

#23 @SergeyBiryukov
17 months ago

In 56017:

Coding Standards: Use Yoda condition in str_ends_with().

This resolves a WPCS error:

Use Yoda Condition checks, you must.

Follow-up to [56015], [56016].

See #58220.

#24 @SergeyBiryukov
17 months ago

In 56019:

Code Modernization: Use str_starts_with() and str_ends_with() in a few more places.

str_starts_with() and str_ends_with() were introduced in PHP 8.0 to perform a case-sensitive check indicating if the string to search in (haystack) begins or ends with the given substring (needle).

WordPress core includes a polyfill for these functions on PHP < 8.0 as of WordPress 5.9.

Follow-up to [55990], [56014].

See #58220.

#25 @SergeyBiryukov
17 months ago

In 56020:

Code Modernization: Use str_starts_with() and str_ends_with() in a few more places.

str_starts_with() and str_ends_with() were introduced in PHP 8.0 to perform a case-sensitive check indicating if the string to search in (haystack) begins or ends with the given substring (needle).

WordPress core includes a polyfill for these functions on PHP < 8.0 as of WordPress 5.9.

Follow-up to [55990], [56014], [56019].

See #58220.

#26 @SergeyBiryukov
17 months ago

In 56021:

Code Modernization: Use str_contains() in a few more places.

str_contains() was introduced in PHP 8.0 to perform a case-sensitive check indicating if the string to search in (haystack) contains the given substring (needle).

WordPress core includes a polyfill for str_contains() on PHP < 8.0 as of WordPress 5.9.

This commit replaces false !== strpos( ... ) with str_contains() in core files, making the code more readable and consistent, as well as better aligned with modern development practices.

Follow-up to [55988].

Props spacedmonkey.
See #58220.

#27 in reply to: ↑ 17 @azaozz
17 months ago

Replying to spacedmonkey:

Performance testing:

Run on PHP 7.4 - 2020 theme and theme test data imported, trunk vs PR. Runned 500 times
...
This change is a net benefit. @azaozz

Mmmmm, these results are really really interesting :)

So running the PHP native strpos() is slower than calling a (polifill) function with the exact same strpos() and with a conditional before it? Will keep it in mind but sounds unbelievable! :)

#28 @milana_cap
16 months ago

  • Keywords add-to-field-guide added

Should be mentioned in the Field guide, together with #58206 and #58012

Note: See TracTickets for help on using tickets.