Make WordPress Core

Opened 9 months ago

Closed 8 months ago

Last modified 8 months ago

#59154 closed defect (bug) (fixed)

PHP Deprecated: ltrim():

Reported by: codersantosh's profile codersantosh Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.4 Priority: normal
Severity: normal Version: 6.3
Component: General Keywords: has-patch has-unit-tests has-testing-info commit
Focuses: php-compatibility Cc:

Description

PHP Deprecated: ltrim(): Passing null to parameter #1 ($string) of type string is deprecated in wp-includes/formatting.php on line 4477

To reproduce:

next_posts( $max_num_pages, false );

When $max_num_pages is 1,
PHP version: 8.1

Possible solution:

<?php
function next_posts( $max_page = 0, $display = true ) {
    $link = get_next_posts_page_link( $max_page );

    $output = '';
    if( $link ){
        $output = esc_url( $link );
    }
        
        if ( $display ) {
                echo $output;
        } else {
                return $output;
        }
}

Attachments (1)

59154.patch (637 bytes) - added by nihar007 9 months ago.
Patch added

Download all attachments as: .zip

Change History (14)

@nihar007
9 months ago

Patch added

#1 @nihar007
9 months ago

  • Keywords has-patch added

Hi @codersantosh,
You are right.
I think adding a check before filtering the link using the esc_url function, will be the best solution.

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


9 months ago
#2

I added a check before filtering the link using the esc_url function.

function next_posts( $max_page = 0, $display = true ) {

$link = get_next_posts_page_link( $max_page );

$output = ;
if ( $link ) {

$output = esc_url( $link );

}

if ( $display ) {

echo $output;

} else {

return $output;

}

}

Trac ticket: 59154

#3 @rajinsharwar
9 months ago

  • Milestone changed from Awaiting Review to 6.4
  • Owner set to rajinsharwar
  • Status changed from new to assigned

Looks good to me, let's put this for 6.4

@codersantosh commented on PR #5045:


9 months ago
#4

@NiharRan Ultra-rapid fix 😃

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


8 months ago

#6 @oglekler
8 months ago

  • Keywords needs-refresh added

This ticket was discussed during a bug scrub,

the patch need a rebase before it can be tested. Please keep in mind that we have 2.5 weeks until RC1.
So, we need to hurry up with it.

Add props @mukesh27

#7 @mukesh27
8 months ago

  • Keywords changes-requested added

@hellofromTonya commented on PR #5045:


8 months ago
#8

The change LGTM 👍 But it needs a test. I'll push a test to this PR shortly.

@hellofromTonya commented on PR #5045:


8 months ago
#9

Seems the service is hiccuping

Pulling wordpress-develop (nginx:alpine)...

received unexpected HTTP status: 503 Service Unavailable

@hellofromTonya commented on PR #5045:


8 months ago
#10

Running the new test locally on PHP 8.1 using the following command:

npm run test:php -- --filter Tests_Link_NextPosts

Before the fix, the test result:

1) Tests_Link_NextPosts::test_should_return_empty_string_when_no_next_posts_page_link
ltrim(): Passing null to parameter #1 ($string) of type string is deprecated

/var/www/src/wp-includes/formatting.php:4494
/var/www/src/wp-includes/link-template.php:2516
/var/www/tests/phpunit/tests/link/nextPost.php:37

After the fix, the test result:

PHPUnit 9.6.13 by Sebastian Bergmann and contributors.

Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

.                                                                   1 / 1 (100%)

Time: 00:00.328, Memory: 166.50 MB

One of the first things I consider is if the fix breaks BC (backward compatibility). In other words, is the result (except for the deprecation) the same? This is why I forced pushed the test to come before the fix, i.e. to test what the result is.

Before the fix, the result is:

  • an empty string is returned.
  • On PHP 8.1 and newer, a deprecation is triggered.

After the fix, an empty string is still returned.

This can also be seen in action here across different PHP versions https://3v4l.org/7cIvT. Notice, all versions return an empty string, even on PHP 8.1+.

This means: no BC break 🎉

#11 @hellofromTonya
8 months ago

  • Keywords has-unit-tests has-testing-info commit added; needs-refresh changes-requested removed
  • Owner changed from rajinsharwar to hellofromTonya
  • Status changed from assigned to reviewing

Hello @codersantosh,

Welcome to WordPress Core's Trac :) Thank you for reporting this issue and providing guidance on how to resolve.

Thank @nihar007 for creating the patch and everyone who reviewed and tested it. :)

Marking the patch for commit. Will prepare the commit shortly.

#12 @hellofromTonya
8 months ago

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

In 56740:

Code Modernization: Fix "passing null to non-nullable" deprecation from next_posts().

The esc_url() function expects to a string for $url parameter. There is no input validation within that function. The function contains a ltrim() which also expects a string. Passing null to this parameter results in Deprecated: ltrim(): Passing null to parameter #1 ($string) of type string is deprecated notice on PHP 8.1+.

Tracing the stack back, a null is being passed to it within next_posts() when get_next_posts_page_link() returns null (it can return a string or null).

On PHP 7.0 to PHP 8.x, an empty string is returned from esc_url() when null is passed to it. The change in this changeset avoids the deprecation notice by not invoking esc_url() when get_next_posts_page_link() returns null and instead sets the $output to an empty string, thus maintain the same behavior as before (minus the deprecation notice).

Adds a test to validate an empty string is returned and the absence of the deprecation (when running on PHP 8.1+).

Follow-up to [11383], [9632].

Props codersantosh, nihar007, hellofromTonya, mukesh27, oglekler, rajinsharwar.
Fixes #59154.

@hellofromTonya commented on PR #5045:


8 months ago
#13

Committed via https://core.trac.wordpress.org/changeset/56740. Thank you everyone for your contributions :)

Note: See TracTickets for help on using tickets.