Replace usage of strpos with str_starts_with

[spacedmonkey]

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

[spacedmonkey]

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

[costdev]

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

[desrosj]

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.

[jorbin]

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.

[TobiasBg]

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

[SergeyBiryukov]

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.

[ayeshrajans]

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)

Trac ticket: https://core.trac.wordpress.org/ticket/58012

[spacedmonkey]

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

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.

[spacedmonkey]

Updated PR so bundled theme and libraries are not updated.

@sabernhardt Removed changes to themes.

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.

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

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

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!

[mukesh27]

@spacedmonkey Left some feedback on PR.

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.

[mukesh27]

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

Add commit keyword.

[spacedmonkey]

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

[SergeyBiryukov]

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.

Thanks for the PR! Merged in r55703.

[azaozz]

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

[flixos90]

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?

[dd32]

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)

[dd32]

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.

[dd32]

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.

[spacedmonkey]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

Trac ticket: https://core.trac.wordpress.org/ticket/58012

[spacedmonkey]

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

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.

[johnbillion]

Related: #57548

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.

[azaozz]

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.

[azaozz]

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

[flixos90]

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.

[SergeyBiryukov]

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.

Closing as it is fixed.

Trac ticket: https://core.trac.wordpress.org/ticket/58012

Trac ticket: https://core.trac.wordpress.org/ticket/58012

[spacedmonkey]

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

[SergeyBiryukov]

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.

Thanks for the PR! Merged in r55987.

[milana_cap]

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