Make WordPress Core

Opened 13 months ago

Closed 10 months ago

Last modified 4 months ago

#58206 closed enhancement (fixed)

Replace usage of strpos with str_contains

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

Description (last modified by SergeyBiryukov)

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

Follow on from #58012.

Attachments (2)

Screenshot 2023-06-21 at 18.25.31.png (51.1 KB) - added by spacedmonkey 11 months ago.
58206.diff (891 bytes) - added by Presskopp 11 months ago.

Download all attachments as: .zip

Change History (70)

#1 @spacedmonkey
13 months ago

Pinging @SergeyBiryukov

#2 follow-up: @dingo_d
13 months ago

str_starts_with is PHP 8+ syntax, so you cannot just replace strpos checks with it until the minimum PHP version is raised to PHP 8 if I'm not mistaken...

#3 in reply to: ↑ 2 @SergeyBiryukov
13 months ago

  • Description modified (diff)
  • Milestone changed from Awaiting Review to 6.3

Replying to dingo_d:

str_starts_with is PHP 8+ syntax, so you cannot just replace strpos checks with it until the minimum PHP version is raised to PHP 8 if I'm not mistaken...

WordPress core includes a polyfill for str_contains() as of [52039] / #49652, as well as str_starts_with() and str_ends_with() as of [52040] / #54377, so the change should be safe to make, see the discussion in #58012.

This ticket is about str_contains() though, updating the description to avoid confusion.

#4 @dingo_d
13 months ago

Thanks for the clarification @SergeyBiryukov. Wasn't aware of the polyfills 👍🏻

#5 @costdev
13 months ago

+1 on these tickets @spacedmonkey!

Patchers: Be careful of loose checks like if ( strpos() ) and if ( ! strpos() ). These should be evaluated for both str_contains() and str_starts_with() (or strpos() > 0) to maintain backward compatibility and prevent possible security regressions.

Last edited 13 months ago by costdev (previous) (diff)

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


13 months ago
#6

  • Keywords has-patch added

Replace usage of strpos with str_contains.

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

#7 follow-up: @Soean
13 months ago

I created a pull request for str_contains, I will create new tickets for str_starts_with and str_ends_with.

Version 0, edited 13 months ago by Soean (next)

#8 in reply to: ↑ 7 @spacedmonkey
13 months ago

Replying to Soean:

I created a pull request for str_contains, I will create new tickets for str_starts_with and str_ends_with.

There is already a ticket for str_starts_with. #58012

@spacedmonkey commented on PR #4394:


13 months ago
#9

@Soean Good start. I found some more places we could use this function.

https://github.com/WordPress/wordpress-develop/blob/6d7430cfe1f8853b75787e03effe8f16f9242ad8/src/wp-admin/includes/theme.php#L1165

https://github.com/WordPress/wordpress-develop/blob/6d7430cfe1f8853b75787e03effe8f16f9242ad8/src/wp-admin/includes/theme.php#L1167

https://github.com/WordPress/wordpress-develop/blob/f3e65b1e441146d19f650971e4c932b358a7b10e/src/wp-admin/includes/ajax-actions.php#L3825

https://github.com/WordPress/wordpress-develop/blob/f3e65b1e441146d19f650971e4c932b358a7b10e/src/wp-admin/includes/file.php#L2334

https://github.com/WordPress/wordpress-develop/blob/6c47ed1dc697a81755d3892e41cede7b9a318d5b/src/wp-admin/includes/post.php#L1110

https://github.com/WordPress/wordpress-develop/blob/6c47ed1dc697a81755d3892e41cede7b9a318d5b/src/wp-admin/includes/post.php#L1119

https://github.com/WordPress/wordpress-develop/blob/f3e65b1e441146d19f650971e4c932b358a7b10e/src/wp-admin/includes/ajax-actions.php#L3362

https://github.com/WordPress/wordpress-develop/blob/f3e65b1e441146d19f650971e4c932b358a7b10e/src/wp-includes/class-wp-query.php#L4414

https://github.com/WordPress/wordpress-develop/blob/f3e65b1e441146d19f650971e4c932b358a7b10e/src/wp-includes/class-wp-query.php#L4414

https://github.com/WordPress/wordpress-develop/blob/f3e65b1e441146d19f650971e4c932b358a7b10e/src/wp-includes/class-wp-query.php#L4524

https://github.com/WordPress/wordpress-develop/blob/e4419d727ca99efff002ed7dbb70500e5093ed53/src/wp-includes/rss.php#L115

https://github.com/WordPress/wordpress-develop/blob/a028958dd6f01427fcf2fda5dd74c3a3a543b44a/src/wp-includes/blocks/navigation-submenu.php#L266

https://github.com/WordPress/wordpress-develop/blob/e4419d727ca99efff002ed7dbb70500e5093ed53/src/wp-includes/rss.php#L115

https://github.com/WordPress/wordpress-develop/blob/a028958dd6f01427fcf2fda5dd74c3a3a543b44a/src/wp-includes/blocks/navigation-submenu.php#L266

https://github.com/WordPress/wordpress-develop/blob/f3e65b1e441146d19f650971e4c932b358a7b10e/src/wp-includes/link-template.php#L4430

https://github.com/WordPress/wordpress-develop/blob/f3e65b1e441146d19f650971e4c932b358a7b10e/src/wp-includes/media.php#L1300

https://github.com/WordPress/wordpress-develop/blob/dcd1ba9330c23d3745c154eb11069ac6288c0665/src/wp-includes/option.php#L1263

https://github.com/WordPress/wordpress-develop/blob/f3e65b1e441146d19f650971e4c932b358a7b10e/src/wp-includes/pluggable.php#L1236

https://github.com/WordPress/wordpress-develop/blob/f3e65b1e441146d19f650971e4c932b358a7b10e/src/wp-includes/post.php#L6106

https://github.com/WordPress/wordpress-develop/blob/e4419d727ca99efff002ed7dbb70500e5093ed53/src/wp-includes/rss.php#L115

https://github.com/WordPress/wordpress-develop/blob/6c47ed1dc697a81755d3892e41cede7b9a318d5b/src/wp-includes/user.php#L3061

https://github.com/WordPress/wordpress-develop/blob/f3e65b1e441146d19f650971e4c932b358a7b10e/src/wp-includes/rest-api/class-wp-rest-request.php#L309

@Soean commented on PR #4394:


13 months ago
#10

@spacedmonkey Thanks, I added the post.php line.

In the first step I only changed the strpos( ) === false and strpos( ) !== false, as described in the ticket . The others we have to look at very closely, because if a sting starts with a substing strpos returns 0 which is false and str_contains returns true.

{{{php
$string = '/wp-content'
strpos( $string, '/' ) 0 -> false
str_contains( $string, '/')
-> true
}}}

#11 @spacedmonkey
13 months ago

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

@Soean Can you rebase / merge trunk on PR. Core changes have added merge conflicts.

#12 @azaozz
13 months ago

  • Keywords 2nd-opinion changes-requested added

I have "mixed feelings" about this. Imho replacing false === strpos( ... ) and false !== strpos( ... ) with str_contains() is premature.

The problem is that only ~20% of the current WP sites are on PHP 8+. So 80% will have to use the polyfill. Then the question is: how much slower is the polyfill compared to the native PHP 7.4 code? I did a quick comparison and it seems about 300% slower:

2.350807ms
false !== strpos(), 100000 times

7.260107ms
str_contains(), 100000 times

So it seems this should be postponed until (at least) more than half of the WP installs are on PHP 8+ as it reduces performance.

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

#13 follow-up: @spacedmonkey
13 months ago

I run benchmarks against this change. I see a net posative.

500 runs of the benchmark - trunk ( 0746cc7324b4b025d1ad3f2afeeb2aadd7d1d19f ) - Theme tt1.

Trunk - PHP 7.4 PR - PHP 7.4 Trunk - PHP 8.0 PR - PHP 8.0
Response Time (median) 79.64 79.6 79.4 79.31
wp-before-template (median)32.88 32.93 32.39 32.24
wp-template (median)40.59 40.42 40.09 40.59
wp-total (median)73.72 73.37 72.57 72.77

There is not a notable improvement performance. As a code quality improvement, this is +1 from me. There are lots of parts of core that use str_contains. For consistency we should use it everywhere.

#14 @spacedmonkey
12 months ago

  • Keywords commit added; changes-requested removed

#15 @spacedmonkey
12 months ago

This change looks good to me. I think we are good to commit.

#16 in reply to: ↑ 13 @azaozz
12 months ago

Replying to spacedmonkey:

I run benchmarks against this change. I see a net posative.

Hmmmm, that's really strange? Why would the benchmark you're using miss the 300% slow-down of the PHP < 8 native code vs. the polyfill, and the PHP 8+ native code? Seems something's not right?

#17 @spacedmonkey
12 months ago

Maybe the performance lead, @flixos90 could take a look into this.

To be clear, if they’re a slight performance loss, I think this change is worth for the code quality improvement.

Worth noting, the performance team is currently working on docs on how we run our benchmarks. That way we could run the same benchmarks and we could see similar results.

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


12 months ago

#19 follow-up: @kirasong
11 months ago

Hi! I read through this ticket, and I noticed that there are two performance tests, with different results, and two different interpretations of the impact.

I wanted to check an assumption here: Is it correct that the test from @azaozz is on the specific function being changed, and the one from @spacedmonkey is a holistic / integration test that shows the total performance impact, including however often the polyfill / new function is used?

If my understanding is correct, and the total performance impact in practice is low / negligible, I like this change. It moves core in the direction of more modern PHP practices, and, in my opinion (I realize this is subjective), makes the resulting code more readable, too.

Would certainly be open to other performance tests as well, to verify assumptions, if anyone has the availability to do so.

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


11 months ago

#21 @spacedmonkey
11 months ago

@azaozz Can you provide more information on how you got these numbers. What software or tool was used to generate these numbers? What versions of PHP were used, was opcache enabled?

The process I used to benchmark the PR is documented here. I have run both benchmark and profiles on this change and seen positive results.

It worth noting that str_contains and false === strpos( ... ) are not exactly the same. The compatibility shim, in compat.php, does false === strpos( ... ) but also does a check to see if the $needle is an empty string. This is a little extra logic. That said, I see this as a positive as it hardens the code with better checks.

I think this change should be committed.

#22 in reply to: ↑ 19 @azaozz
11 months ago

Replying to mikeschroder:

Is it correct that the test from @azaozz is on the specific function being changed, and the one from @spacedmonkey is a holistic / integration test that shows the total performance impact, including however often the polyfill / new function is used?

Yes, that's my understanding too. That's why I was surprised to see that the results of the "benchmark test" not only missed the slow-down of using the polifill vs. using PHP 7.4 native code, but showed the polifill as slightly faster.

if they’re a slight performance loss, I think this change is worth it

Yep, agreed. Seems the slowdown when using the polifill is really small.

Can you provide more information on how you got these numbers. What software or tool was used to generate these numbers? What versions of PHP were used, was opcache enabled?

My testing was really simple: using wp_env (Docker, PHP 7.4), no opcache, simple for ( $i = 0; $i < 100000; $i++ ) loop with time start above and echo of passed time below (seems I've deleted the exact code but can recreate it if needed).

That code seems really fast, 100k loops are only ~7ms on my computer. Still I'm curious why the tests show different results.

#23 @spacedmonkey
11 months ago

I run the following script using query monitor to start and stop timers. Run using PHP 7.4 on docker environment.

add_action('init', function(){
        do_action( 'qm/start', 'strpos' );
        for ($x = 0; $x <= 100000; $x++) {
                (false === strpos( 'foo-bar', 'bar' ));
        }
        do_action( 'qm/stop', 'strpos' );

        do_action( 'qm/start', 'str_contains' );
        for ($x = 0; $x <= 100000; $x++) {
                (str_contains( 'foo-bar', 'bar' ));
        }
        do_action( 'qm/stop', 'str_contains' );
});

strpos - 0.0040
str_contains - 0.0086.

Meaning that is str_contains slow, but it is by a very small amount. Not so much that this should be a a blocker for this change.

To be fair, think kind of benchmarking, so not compare to real benchmarking of the change in core.

#24 @flixos90
11 months ago

Chiming in here due to the ongoing discussion: I believe what is being discussed here applies holistically not only to this ticket, but also #58012 and #58220.

I personally think all of these are not as much about performance as they are about code cleanup and using the modern PHP functions available for specific purposes. It is good that we're benchmarking performance, and while it's hard to say which of the two options is currently faster, I think it's fair to say that in any case based on the benchmarks done the overall performance impact is negligible.

I would advise that we make a decision decoupled from the performance & benchmarking comversation, simply because the difference is so little that due to variance sometimes it will seem it's faster while other times it will seem that it's slower. To me, the main discussion point is the tradeoff between using modern PHP best practices vs avoiding refactoring that is not critical.

From a performance perspective, just conceptually, I assume that sites on PHP 8+ will be faster when using the new functions while sites on PHP <8 will either see no difference or be slightly slower (due to using the polyfill). But again, I'd say we shouldn't focus on this point as much.

I personally don't have a strong opinion in either direction, though I'd argue that in the long term we should probably use the modern PHP functions natively available. But most importantly, I wanted to chime in so that we can hopefully avoid deep diving into a performance discussion on these tickets, which per the above is pretty much a moot point.

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

#25 @peterwilsoncc
11 months ago

I think this can go in as a way to encourage modern practices and making the code easier to read for developers.

I agree with Felix, this can be decoupled from a performance discussion. Any changes appear to be a rounding error. I suspect people are seeing different results because the difference is environmental and effected by other processes.

@spacedmonkey commented on PR #4394:


11 months ago
#26

@Soean Do you mind rebase from trunk. they are some new str_pos that have been added in newer commits.

#27 @SergeyBiryukov
11 months ago

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

In 55988:

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

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 [52039], [52040], [52326], [55703], [55710], [55987].

Props Soean, spacedmonkey, costdev, dingo_d, azaozz, mikeschroder, flixos90, peterwilsoncc, SergeyBiryukov.
Fixes #58206.

@SergeyBiryukov commented on PR #4394:


11 months ago
#28

Thanks for the PR! Merged in r55988.

#29 @spacedmonkey
11 months ago

@SergeyBiryukov I think there is one more place we need to change. See.

#30 @spacedmonkey
11 months ago

More functions using strpos
_wptexturize_pushpop_element
esc_url
get_the_permalink

#31 follow-up: @ryelle
11 months ago

@SergeyBiryukov I think this is causing trouble with load-styles.php — when I apply [55988] to WordPress.org, https://wordpress.org/wp-admin/load-styles.php?load=l10n,wp-auth-check returns an empty result. Could be related to [55710]/#58244?

#32 follow-up: @joedolson
11 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Seems to be a problem in install.php, as well.

#33 follow-up: @dd32
11 months ago

Similar to [55157], [55990] (#58220) + [55988] have again introduced an external dependencies to wpdb of compat.php or PHP8.

As mentioned above, [55988] has introduced the same thing as [55710] to load-styles.php - probably this change: https://core.trac.wordpress.org/changeset/55988/trunk/src/wp-includes/script-loader.php

I think it might just be worthwhile including compat.php in all of these alternate entry points and requiring that wpdb when used outside of WordPress either loads compat too or runs on PHP8+.

#34 @SergeyBiryukov
11 months ago

In 55994:

Database: Replace str_contains() and str_ends_with() usage in wpdb methods.

This avoids fatal errors on PHP < 8.0 if the file is included directly outside of WordPress core, e.g. by HyperDB.

While WordPress core does include polyfills for these functions, they are not directly loaded in the wpdb class.

Follow-up to [54384], [55157], [55158], [55988], [55990].

Props dd32, ryelle, joedolson.
See #58206.

#35 in reply to: ↑ 33 @SergeyBiryukov
11 months ago

Replying to dd32:

Similar to [55157], [55990] (#58220) + [55988] have again introduced an external dependencies to wpdb of compat.php or PHP8.

Thanks! Reverted the changes to wpdb for now, though including compat.php might be preferable in the long run.

Will look into the issues with script-loader.php and install.php in a few hours.

#36 @SergeyBiryukov
11 months ago

In 56002:

Script Loader: Replace str_contains() usage in wp-includes/script-loader.php.

This avoids fatal errors on PHP < 8.0 if the file is included via wp-admin/load-scripts.php or wp-admin/load-styles.php, in which case the polyfills from wp-includes/compat.php are not loaded.

Follow-up to [55703], [55710], [55988].

Props ryelle.
See #58206.

#37 in reply to: ↑ 31 @SergeyBiryukov
11 months ago

Replying to ryelle:

I think this is causing trouble with load-styles.php — when I apply [55988] to WordPress.org, https://wordpress.org/wp-admin/load-styles.php?load=l10n,wp-auth-check returns an empty result.

Thanks! This should be resolved now.

#38 in reply to: ↑ 32 @SergeyBiryukov
11 months ago

Replying to joedolson:

Seems to be a problem in install.php, as well.

Thanks! Yes, it looks like there's a similar issue with wp-load.php, triggered via wp-admin/install.php. In that instance, it might be preferable to include compat.php, as the site is not installed yet, so the overhead of including an extra file is negligible.

It might also be beneficial to load compat.php a bit earlier in wp-settings.php, so that a few more files could use the polyfills going forward.

#39 @SergeyBiryukov
11 months ago

In 56006:

Bootstrap/Load: Require wp-includes/compat.php in wp-load.php.

This allows for using polyfill functions if the site is not installed yet.

Follow-up to [28978], [55988].

Props joedolson, dd32.
See #58206.

#40 @SergeyBiryukov
11 months ago

In 56007:

Bootstrap/Load: Require wp-includes/compat.php earlier in wp-settings.php.

This allows for using polyfill functions in more files, including the advanced-cache.php drop-in.

Follow-up to [6108], [46183], [55988], [55990], [56006].

See #58206.

#41 @SergeyBiryukov
11 months ago

In 56031:

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], [56021].

See #58206.

#42 @SergeyBiryukov
11 months ago

In 56032:

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], [56021], [56031].

See #58206.

#43 @SergeyBiryukov
11 months ago

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

#44 follow-up: @westonruter
11 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

When running the wordpress-develop core development site in 7.4 I found a fatal error due to the changes here, specifically r56031. I get:

Fatal error:  Uncaught Error: Call to undefined function str_ends_with() in /var/www/src/wp-includes/load.php:76
Stack trace:
#0 /var/www/src/index.php(27): wp_fix_server_vars()
#1 {main}
  thrown in /var/www/src/wp-includes/load.php on line 76

To use this function here, it seems we'll need to load compat.php earlier.

#45 @Presskopp
11 months ago

Indeed in wp-settings.php first require ABSPATH . WPINC . '/load.php'; happens before require ABSPATH . WPINC . '/compat.php';

@Presskopp
11 months ago

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


11 months ago

#47 @joemcgill
11 months ago

Thanks, @Presskopp. 58206.diff looks right to me.

#48 @oglekler
11 months ago

@SergeyBiryukov please look at this one 🙏

#49 in reply to: ↑ 44 @SergeyBiryukov
11 months ago

Replying to westonruter:

When running the wordpress-develop core development site in 7.4 I found a fatal error due to the changes here, specifically r56031. I get:

Fatal error:  Uncaught Error: Call to undefined function str_ends_with() in /var/www/src/wp-includes/load.php:76
Stack trace:
#0 /var/www/src/index.php(27): wp_fix_server_vars()
#1 {main}
  thrown in /var/www/src/wp-includes/load.php on line 76

Thanks! It looks like the changes to wp-includes/load.php from [55990,56014,56031,56032] were reverted in [56065], so this should not be a blocker for Beta 1.

Unless I'm missing something, 58206.diff might not resolve this particular issue. When wp_fix_server_vars() runs in wp-settings.php, compat.php is already loaded, even without the patch. However, it is not loaded in src/index.php, which is the entry point here.

I would like to take a closer look at reintroducing these changes after making sure the polyfills are available for use in all of the known entry points, as suggested in comment:33, except maybe for wp-admin/load-(scripts|styles).php and script-loader.php, as per comment:37:ticket:58012.

#50 @azaozz
11 months ago

  • Keywords needs-patch added; has-patch commit removed
  • Severity changed from normal to blocker

Another place where it seems the polifills cannot be used is in update-core.php. The problem seems to be that when (auto)updating core this file is downloaded first and runs in the old WP version first. So if a polifill is missing from the old version but used in update-core.php, it throws a fatal error.

Seems this was the reason updates from WP 5.7 and earlier to 6.3-beta1 failed, see: https://wordpress.slack.com/archives/C02RQBWTW/p1687896726091119.

Setting this to the highest priority as it needs a patch asap.

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

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


11 months ago
#51

  • Keywords has-patch added; needs-patch removed

#52 @azaozz
11 months ago

  • Keywords needs-testing added

@SergeyBiryukov commented on PR #4728:


11 months ago
#53

Good catch, thanks!

I have added comments along the lines of r56002 so that the polyfills are not accidentally reintroduced later 🙂

#54 @ironprogrammer
11 months ago

Test Report

This validates the fix applied to update-core.php in https://github.com/WordPress/wordpress-develop/pull/4728.

Steps to Test Patch

  1. Install a fresh copy of WordPress 5.8 or earlier.
  2. Enable debug mode and logging to track errors (WP_DEBUG, WP_DEBUG_LOG). Enable SCRIPT_DEBUG to workaround known issue explained in Additional Information below.
  3. Follow these steps to build a custom WordPress installation package with the patch applied to it:
    # change directory to test site root, e.g.
    cd ~/Sites/wp-install-test/
    
    # PREPARE CUSTOM PACKAGE ZIP
    # download 6.3-beta1 (or update to a newer zip if needed)
    curl -O https://downloads.wordpress.org/release/wordpress-6.3-beta1.zip
    unzip wordpress-6.3-beta1.zip
    
    # download patch file for PR 4728 (for update-core.php)
    cd wordpress
    curl -O https://patch-diff.githubusercontent.com/raw/WordPress/wordpress-develop/pull/4728.diff
    
    # apply patch
    patch wp-admin/includes/update-core.php 4728.diff
    # should show: patching file wp-admin/includes/update-core.php
    
    # now zip the customized package
    cd ..
    zip -r wordpress-custom.zip wordpress
    # NOTE: Custom ZIP file MUST be accessible at:
    #   http://<your-test.site>/wordpress-custom.zip
    
    # PREPARE CUSTOM UPDATE PLUGIN
    # install custom wordpress update plugin
    curl https://gist.githubusercontent.com/ironprogrammer/1aaff6f60b263c1842e3584365729ffc/raw/9731d0c07b377684052c43c4df2b025032130310/fake-wordpress-update.php -o wp-content/plugins/fake-wordpress-update.php
    # enable the plugin
    wp plugin activate fake-wordpress-update
    
  4. In WP admin, navigate to Dashboard > Updates.
  5. Update to version "6.3.0-custom" (it should be the first available option) to install the patched package.

Expected Results

  • ✅ Site should upgrade to the custom package without errors.

Environment

  • Hardware: MacBook Pro Apple M1 Pro
  • OS: macOS 13.4.1
  • Browser: Safari 16.5.1
  • Server: nginx/1.25.0
  • PHP: 7.4.33
  • WordPress: Tested 5.0 through 5.9 majors -> 6.3-beta1 custom build
  • Active Plugins:

Actual Results

When testing the bugfix patch:

  • ✅ Site upgraded without issue.

Additional Information

Before the 6.3 Beta 1 build is officially released, there is a known issue with CSS in WP admin when SCRIPT_DEBUG is not enabled.

Last edited 11 months ago by ironprogrammer (previous) (diff)

#55 follow-up: @azaozz
11 months ago

In 56088:

Revert use of str_starts_with() and str_contains() in update-core.php.

Fixes updating WordPress from 5.7 and earlier versions. When updating this file runs first in the old version where the polifills may not be available.

Props: ironprogrammer, SergeyBiryukov, dd32, azaozz.
See: #58206.

#56 @azaozz
11 months ago

  • Keywords has-patch needs-testing removed

Leaving the ticket open after [56088] because of comments 44 and 49.

#58 @flixos90
11 months ago

  • Focuses performance removed

Per previous discussion above, removing the performance focus as this is not performance related.

This ticket was mentioned in Slack in #core by oglekler. View the logs.


10 months ago

#60 @spacedmonkey
10 months ago

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

No follow up action required. Closing for now.

#61 @milana_cap
10 months ago

  • Keywords add-to-field-guide added

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

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

#62 @SergeyBiryukov
10 months ago

In 56241:

Bootstrap/Load: Require wp-includes/compat.php in src/index.php.

This allows for using polyfill functions if src/index.php is the entry point (this file exists as a reminder to build the assets, and is different from the actual index.php file that gets built and boots WordPress).

Includes:

  • Moving the check for the required PHP and MySQL versions earlier.
  • Making the load order consistent between src/index.php, wp-load.php, and wp-settings.php.

Follow-up to [46183], [56006], [56007].

Props westonruter, Presskopp, joemcgill, SergeyBiryukov.
See #58206.

#63 @SergeyBiryukov
10 months ago

In 56245:

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], [55990], [56014], [56021], [56031], [56032], [56065], [56241].

See #58206.

#64 in reply to: ↑ 55 ; follow-up: @frankit
9 months ago

Replying to azaozz:

In 56088:

Revert use of str_starts_with() and str_contains() in update-core.php.

Fixes updating WordPress from 5.7 and earlier versions. When updating this file runs first in the old version where the polifills may not be available.

Props: ironprogrammer, SergeyBiryukov, dd32, azaozz.
See: #58206.

I might be wrong but I just commented on the github commit that it looks to me that str_starts_with() on line 1429 has been left over.
I'm getting Fatal error: Uncaught Error: Call to undefined function str_starts_with() [...] from that line while trying to update from a wp < 5.7 to latest 6.3

This ticket was mentioned in PR #5034 on WordPress/wordpress-develop by frankIT.


9 months ago
#65

  • Keywords has-patch added

Quoting: @azaozz

Fixes updating WordPress from 5.7 and earlier versions. When updating this file runs first in the old version where the polifills may not be available.

In commit 45363a5 the use of str_starts_with() has been reverted everywhere but from line 1429
That line raise a Fatal error: Uncaught Error: Call to undefined function str_starts_with() [...] when upgrading from < 5.7

https://core.trac.wordpress.org/ticket/58206

#66 in reply to: ↑ 64 @azaozz
9 months ago

Replying to frankit:

Thanks for the PR. It seems better if this is in a new ticket for 6.3.1. Made #59145 for it.

#67 follow-up: @poena
4 months ago

Do I understand correctly that the polyfill is only added to WordPress 5.9 and later?

The use of str_contains() in Twenty Twenty causes fatal errors because the theme supports WordPress version 4.7 and PHP 5.2.4.

#68 in reply to: ↑ 67 @SergeyBiryukov
4 months ago

Replying to poena:

Do I understand correctly that the polyfill is only added to WordPress 5.9 and later?

The use of str_contains() in Twenty Twenty causes fatal errors because the theme supports WordPress version 4.7 and PHP 5.2.4.

Good catch, thanks! Yes, the polyfill is only available in WordPress 5.9 or later. Created a follow-up ticket: #60241.

Note: See TracTickets for help on using tickets.