Make WordPress Core

Opened 11 months ago

Closed 9 months ago

Last modified 8 months ago

#58012 closed enhancement (fixed)

Replace usage of strpos with str_starts_with

Reported by: spacedmonkey's profile spacedmonkey Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.3 Priority: normal
Severity: normal Version:
Component: General Keywords: good-first-bug has-patch 2nd-opinion add-to-field-guide
Focuses: Cc:

Description

Replace usage of 0 === strpos or 0 !== strpos with str_starts_with. It makes the code more readable and use a PHP native function ( which may have a performance benefit ).

Attachments (1)

58012.diff (1.0 KB) - added by SergeyBiryukov 10 months ago.

Download all attachments as: .zip

Change History (49)

#1 follow-up: @spacedmonkey
11 months ago

@SergeyBiryukov This is a code quality thing, I would love your thoughts on this.

#2 @costdev
11 months ago

+1

We've been trying to use str_starts_with() (and similar) for some time, making replacements here and there in existing code. Any remaining instances should be checked and changed to str_starts_with() and ! str_starts_with() as appropriate.

#3 @desrosj
11 months ago

Just a few notes to provide full context.

The str_starts_with() function is only available on PHP 8.0+. The function was polyfilled into Core in 5.9 along with str_ends_with() in [52040], so we can safely update the codebase, but it's likely sites running PHP 5.x & 7.x won't see the likely performance improvement sites using 8.x will.

str_contains() was also polyfilled in [52039].

Seems there was an adhoc instances of these new functions being added in [52326], but seems that was the only case where this was intentionally updated.

I think this is a good change to make provided we have tests in all the right places. It also seems like a reasonable assumption that sites running PHP 8.0+ would see some form of a performance increase, but would be great to have some benchmarking to know the real impact of this change.

#4 @jorbin
11 months ago

I ran a quick test - https://3v4l.org/dUbZ9 which shows that strpos is slightly faster before PHP8 and across 1,000,000 runs, the time saved is only about 3.5e-8 seconds. As we have less than 1000 instances in code that would change, this doesn't seem like it will move the performance needle substantially.

To me, this feels like a micro-optimization that is best done as a part of other code changes and not done on its own. After all, Code refactoring should not be done just because we can.

#5 @TobiasBg
11 months ago

A tiny bit of discussion on this is at https://core.trac.wordpress.org/ticket/56791#comment:8 as well.

#6 in reply to: ↑ 1 @SergeyBiryukov
11 months ago

Replying to spacedmonkey:

@SergeyBiryukov This is a code quality thing, I would love your thoughts on this.

Looks like I had some thoughts earlier in comment:8:ticket:56791 (thanks @TobiasBg!):

I am in favor of making a batch replacement to use str_contains(), str_starts_with(), and str_ends_with() where appropriate, for readability and consistency.

Replying to jorbin:

I ran a quick test - https://3v4l.org/dUbZ9 which shows that strpos is slightly faster before PHP8

Thanks for the test! True, but it also appears to show that str_starts_with() is twice as fast on PHP 8.0+. As we gradually move to modern PHP versions, this seems like a small but beneficial performance improvement.

#7 @ayeshrajans
11 months ago

I'm in favor of this change too. Even if the performance difference is minimal (and it is not like we run these functions millions of times anyway), I think the code readability alone is a strong enough motivation for this change.

Apart from strpos calls, I imagine we will have preg_match calls without /i flag that could also be replaced with str_(starts_with|contains|ends_with)

This ticket was mentioned in PR #4273 on WordPress/wordpress-develop by @spacedmonkey.


11 months ago
#8

  • Keywords has-patch added; needs-patch removed

#9 @spacedmonkey
11 months ago

I have created a PR for this. My early benchmarks are showing a small performance benefit.

@sabernhardt commented on PR #4273:


11 months ago
#10

Please do not replace strpos in bundled themes. The themes should work with older versions of both PHP and WordPress (before the polyfill). The str_starts_with function was already suggested for all the $font_version variables. Maybe these could have a note to explain it, though.

#11 @SergeyBiryukov
11 months ago

  • Milestone changed from Awaiting Review to 6.3

#12 @spacedmonkey
11 months ago

Updated PR so bundled theme and libraries are not updated.

@spacedmonkey commented on PR #4273:


11 months ago
#13

@sabernhardt Removed changes to themes.

@SergeyBiryukov commented on PR #4273:


11 months ago
#14

Looks good based on a visual review, left one suggestion in wp-includes/class-simplepie.php.

As a follow-up to this, false !== strpos( ... ) can also be replaced with str_contains(), which is polyfilled in core as well.

This ticket was mentioned in PR #4367 on WordPress/wordpress-develop by lgadzhev.


11 months ago
#15

I have merged the latest truck and trunk and resolved the merge conflicts and also replaced the occurrence of str_pos in wp-includes/class-simplepie.php with str_contains

Trac ticket: Replace usage of strpos with str_starts_with

This ticket was mentioned in PR #4369 on WordPress/wordpress-develop by lgadzhev.


11 months ago
#16

I have merged the latest trunk branch into the current one and resolved the merge conflicts

Trac ticket: Replace usage of strpos with str_starts_with

@spacedmonkey commented on PR #4273:


10 months ago
#17

Thanks @spacedmonkey, Great start. left a few suggestions for removing extra round brackets.

if ( strpos( haystack, needle ) === 0 ) { why this type of check is not consider in this PR? I found many instance in core for same.

List of files:

/src/wp-admin/load-styles.php
/src/wp-admin/includes/credits.php
/src/wp-admin/includes/template.php
/src/wp-includes/block-template.php
/src/wp-includes/bookmark-template.php
/src/wp-includes/class-wp.php
/src/wp-includes/functions.php
/src/wp-includes/l10n.php
/src/wp-includes/pluggable.php
/src/wp-includes/rest-api.php
/src/wp-includes/rewrite.php
/src/wp-includes/rest-api/class-wp-rest-server.php
/src/wp-includes/widgets/class-wp-widget-block.php

Good patch. I did a regex search and found more!

#18 @mukesh27
10 months ago

  • Keywords changes-requested added

@spacedmonkey Left some feedback on PR.

@spacedmonkey commented on PR #4273:


10 months ago
#19

Thanks @spacedmonkey, Thanks for the updates. Left final feedback.

I found some instances in files that are missing:

https://github.com/spacedmonkey/wordpress-develop/blob/str_starts_with/src/wp-includes/class-simplepie.php#L3227
https://github.com/spacedmonkey/wordpress-develop/blob/str_starts_with/src/wp-includes/functions.php#L7434
https://github.com/spacedmonkey/wordpress-develop/blob/str_starts_with/src/wp-includes/rest-api/class-wp-rest-server.php#L651

Good catches. Simple pie was left by design, It is an external library. Fixed now.

#20 @mukesh27
10 months ago

  • Keywords commit added; changes-requested removed
  • Owner set to spacedmonkey
  • Status changed from new to assigned

Thanks @spacedmonkey, Changes look good now and PR got two approval.

Add commit keyword.

#21 @spacedmonkey
10 months ago

  • Owner changed from spacedmonkey to SergeyBiryukov

Reassigning to @SergeyBiryukov to commit, as I don't have time to commit this.

#22 @SergeyBiryukov
10 months ago

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

In 55703:

Code Modernization: Replace usage of strpos() with str_starts_with().

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

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

This commit replaces 0 === strpos( ... ) with str_starts_with() in core files, making the code more readable and consistent, as well as improving performance.

While strpos() is slightly faster than the polyfill on PHP < 8.0, str_starts_with() is noticeably faster on PHP 8.0+, as it is optimized to avoid unnecessarily searching along the whole haystack if it does not find the needle.

Follow-up to [52039], [52040], [52326].

Props spacedmonkey, costdev, sabernhardt, mukesh27, desrosj, jorbin, TobiasBg, ayeshrajans, lgadzhev, SergeyBiryukov.
Fixes #58012.

@SergeyBiryukov commented on PR #4273:


10 months ago
#23

Thanks for the PR! Merged in r55703.

#24 follow-up: @azaozz
10 months ago

  • Focuses performance added
  • Keywords 2nd-opinion added; commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Same considerations as #58206. This seems premature.

Currently only ~20% of the WP installs are on PHP 8+. The question is: how much slower is the polyfill on PHP 7.4?

A quick test shows it at about 50% slower:

2.439975ms
0 === strpos(), 100000 times

3.588421ms
str_starts_with(), 100000 times

It's not as bad as the str_contains() polyfill but will still slow down 80% of the WP sites a little. Perhaps better to "refactor" this when at least 50% of WP is on PHP 8+.

Last edited 10 months ago by azaozz (previous) (diff)

#25 follow-up: @flixos90
10 months ago

Completely unrelated to the point above whether we should do this or not, it seems this commit broke my WP core dev environment 🤔

My WP Admin shows completely unstyled since [55703], but without that commit (i.e. when running [55702]) it shows up correctly. It may have unexpectedly broken something?

#26 in reply to: ↑ 25 @dd32
10 months ago

Replying to flixos90:

it seems this commit broke my WP core dev environment 🤔
[...] It may have unexpectedly broken something?

This is because script-loader.php is used by wp-admin/load-styles.php which doesn't load the compat functions.

( ! ) Fatal error: Uncaught Error: Call to undefined function str_starts_with() in wp-admin/load-styles.php on line 77

(load-styles.php sets a error_reporting( 0 ) which means you probably won't see the fatal)

Last edited 10 months ago by dd32 (previous) (diff)

This ticket was mentioned in Slack in #meta by dd32. View the logs.


10 months ago

#28 in reply to: ↑ 24 ; follow-ups: @dd32
10 months ago

Replying to azaozz:

Same considerations as #58206. This seems premature.

I'm on the other side of the fence here, while this undoubtably makes WordPress slower for the majority of PHP installs (Making this change odd under the 'performance' tag), using more modern functions is beneficial overall for coding practices.

#29 @dd32
10 months ago

Additionally... Can someone look at adding a unit test to cover load-styles.php? This entry point has had many fatals over the years due to it not being used in development environments and not having the full kitchen-sink of function dependencies being loaded.

#30 follow-up: @spacedmonkey
10 months ago

script-loader.php Seems to have used since [55658]. Before this commit. This file needs those compatibility functions.

Regarding performance on PHP 8.0+ is a little bit faster. AS for PHP 8.0-, the compablity shim from str_starts_with has a check for if the needle is, meaning 0 === strpos() is not exactly the same as str_starts_with. Having error handling on needle is a code quality thing IMO.

#31 @SergeyBiryukov
10 months ago

In 55710:

General: Restore strpos() check in wp-admin/load-styles.php.

This resolves a fatal error on PHP < 8.0, as wp-includes/compat.php is not loaded in this file, so str_starts_with() may not be available.

Follow-up to [55703].

Props dd32, flixos90, DigTek.
Fixes #58244. See #58012.

#32 in reply to: ↑ 30 @SergeyBiryukov
10 months ago

Replying to spacedmonkey:

script-loader.php Seems to have used since [55658]. Before this commit. This file needs those compatibility functions.

Indeed, str_starts_with() was already used in a few places in wp-includes/script-loader.php, see [52695], [52754], [53282], [55658].

In this case, however, it appears that the issue is not in that file but in wp-admin/load-styles.php itself. It already loads a few theme files as of [51056] and [52054], so maybe it should load compat.php as well?

In the meantime, [55710] should resolve the fatal error.

#34 follow-up: @spacedmonkey
10 months ago

I have a fix on #4418.

This loads in the compat file. That seems to have fixed the issue. Mind taking a look at the PR.

@SergeyBiryukov

@SergeyBiryukov commented on PR #4418:


10 months ago
#35

Thanks! Yeah, that's what I was leaning towards in 58012.diff as well 🙂

The str_starts_with() call was replaced in r55710, let's reinstate it in this PR too.

@azaozz commented on PR #4418:


10 months ago
#37

Sorry but including that file is "Doing it wrong". load-scripts.php and load-styles.php are (were?) designed to run outside of WordPress. They use script-loader.php as a whitelist of URLs to scripts and styles. None of the functions there are supposed to run.

#38 in reply to: ↑ 34 @azaozz
10 months ago

Replying to spacedmonkey:

I have a fix on #4418.

Sorry but that is the wrong way to do this. If you really cannot stand seeing the "legacy" way for testing whether a string starts with something, I'd suggest adding the polyfill to wp-admin/includes/noop.php. This is the proper place for it.

#39 in reply to: ↑ 28 @azaozz
10 months ago

Replying to dd32:

this undoubtably makes WordPress slower for the majority of PHP installs (Making this change odd under the 'performance' tag)

Heheh, exactly, pretty odd for the performance team to want to degrade performance :)

#40 in reply to: ↑ 28 ; follow-up: @flixos90
10 months ago

  • Focuses performance removed

Replying to dd32:

I'm on the other side of the fence here, while this undoubtably makes WordPress slower for the majority of PHP installs (Making this change odd under the 'performance' tag), using more modern functions is beneficial overall for coding practices.

How do you know that it makes WordPress sites slower? Are you referring to most WP sites being on PHP<8? That would make sense to me. That said, in the long term this will probably be beneficial for performance by using a more targeted native PHP function.

Anyway, I agree this change has barely anything to do with performance. We can be nit-picking as much as we want in either direction, but this is about code patterns, like following the latest available PHP best practices.

Maybe 1 of the 2 is an extremely tiny bit faster or slower than the other, but in reality this will make close to zero difference for overall site performance (the same way that for example removing 100 if clauses somewhere is great, but still is too small of a change to actually impact performance. And when I say close to zero, I don't mean 1% (which would still be significant), more like a fraction of a percent.

I'll remove the performance focus as this is about code patterns, not performance.

Last edited 10 months ago by flixos90 (previous) (diff)

#41 in reply to: ↑ 40 @SergeyBiryukov
10 months ago

Replying to flixos90:

this is about code patterns, like following the latest available PHP best practices.

Yeah, that's how I see this ticket too. We already use str_starts_with() in a few places in core (a bit randomly), so might as well do that in a consistent way to improve code quality.

While strpos() has to search the entire string, str_starts_with() is optimized for the particular use case of only checking the beginning, so in the long run in will be a minor performance improvement as well.

@spacedmonkey commented on PR #4418:


10 months ago
#42

Closing as it is fixed.

#45 @spacedmonkey
9 months ago

After [55959] by @oandregal. There need to fix the code as this commit contains a strpos.

I have created a PR https://github.com/WordPress/wordpress-develop/pull/4663

#46 @SergeyBiryukov
9 months ago

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

In 55987:

Code Modernization: Use str_starts_with() in WP_Theme_JSON class methods.

This aims to make the code more readable and consistent, as the function is already used extensively in core files.

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

Follow-up to [55703], [55959].

Props spacedmonkey.
Fixes #58012.

@SergeyBiryukov commented on PR #4663:


9 months ago
#47

Thanks for the PR! Merged in r55987.

#48 @milana_cap
8 months ago

  • Keywords add-to-field-guide added

Should be mentioned in the Field guide, together with https://core.trac.wordpress.org/ticket/58206

Version 1, edited 8 months ago by milana_cap (previous) (next) (diff)
Note: See TracTickets for help on using tickets.