Fix for deprecated rtrim passing null to parameter

[shanemac10]

The untrailingslashit function in formatting.php passes the $value param directly into the PHP rtrim function, which only takes Strings. Anything else trows a "Deprecated" warning (example below).

In my personal case, Yoast SEO was the culprit in the stacktrace, but this untrailingslashit should at least validate and/or cast the $value param to String with strval before calling rtrim.

<?php
function untrailingslashit( $value ) {
        return rtrim( strval( $value ), '/\\' );
}

Fixes these types of errors:
Deprecated: rtrim(): Passing null to parameter #1 ($string) of type string is deprecated in /wp-includes/formatting.php on line 2819

The untrailingslashit function in formatting.php passes the $value param directly into the PHP rtrim function, which only takes Strings. Anything else trows a "Deprecated" warning (example below).

In my personal case, Yoast SEO was the culprit in the stacktrace, but this untrailingslashit should at least validate and/or cast the $value param to String with strval before calling rtrim.

Incoming updated version:

function untrailingslashit( $value ) {
        return rtrim( strval( $value ), '/\\' ); // <-- Cast $value to String with strval()
}

Previous unvalidated version:

function untrailingslashit( $value ) {
        return rtrim( $value, '/\\' );
}

Trac ticket: 63379 Fix for deprecated rtrim passing null to parameter

[shanemac10]

​https://github.com/WordPress/wordpress-develop/pull/8764

[SirLouen]

Replying to shanemac10:

​https://github.com/WordPress/wordpress-develop/pull/8764

Hello @shanemac10
Double-check the patch, I think it has gone south with a wrong rebase or something.
Can you also povide a specific testing reproduction example?

[shanemac10]

@SirLouen Hey thanks for the quick response.

I just followed the links to submit a code change, and I have no idea why there is/was a bunch of other commits tacked on that were being flagged as having merge conflicts. I clicked a button to remove them and it just closed my PR, so I'm extra confused now (sorry). I went straight back to my forked branch from the trunk (shanemac10:rtrim-cast-value-to-string), and made the update and a new PR.

Here's the new PR...
​https://github.com/WordPress/wordpress-develop/pull/8765

This is a link to the code change itself. It's super simple...
​https://github.com/WordPress/wordpress-develop/compare/trunk...shanemac10:wordpress-develop:rtrim-cast-value-to-string

Can you also provide a specific testing reproduction example?

Can you elaborate? Like screenshots of the error on webpage?

[SirLouen]

Replying to shanemac10:

Can you also provide a specific testing reproduction example?

Can you elaborate? Like screenshots of the error on webpage?

No, like if I don't apply the patch what steps should I exactly take to see this error

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

[shanemac10]

@SirLouen

Okay, I made this real quick and tested it, and it causes a critical error before getting to the END if you run it. Just add to a functions.php file.

<?php
add_action('init', function() {
        $test_values = ['', ' test ', null, false, [] ];
        echo '<h2>START</h2>';
        foreach($test_values as $index => $test_value) {
                $test_output = untrailingslashit( $test_value );
                echo '<h2>'.$index.' = '.$test_output.'</h2>';
        }
        echo '<h2>END</h2>';
});

[sabernhardt]

Please report the bug to Yoast (if you have not already done that).

#54997 said that the function should expect a string and anything else is doing it wrong. You found the plugin's error in the stacktrace, and other developers will need to find their mistakes in that too.

[SirLouen]

Test Report

Description

🟠 This report can't fully validate that the indicated patch is the best option

Patch tested: ​https://github.com/WordPress/wordpress-develop/pull/8764

Environment

• WordPress: 6.9-alpha-60093-src
• PHP: 8.2.28
• Server: nginx/1.27.5
• Database: mysqli (Server: 8.4.5 / Client: mysqlnd 8.2.28)
• Browser: Chrome 136.0.0.0
• OS: Windows 10/11
• Theme: Twenty Twenty-One 2.5
• MU Plugins: None activated
• Plugins:
  • Test Reports 1.2.0

Reproduction Steps

1. For testing this, I've used the proposed script
2. The function is receiving a string, and the patch passes various types, only 2 of those strings.

Expected Results

• Results expected for non string values

Results after the patch

1. 🟠 There are no fatal errors, only Warnings for the array to string conversion
2. ✅ Full Unit-Test passing.

Additional Notes

1. This function receives a string but it doesn't handle any other type, returning unexpected results
2. Ideally, it should do any of these 3 options

• A string static typing in the inputs of the function, hence returning a fatal error for anything not string. AFAIK, this option is very uncommon in the WP ecosystem
• Solution suggested in this patch, forcing string conversion for any incoming random type. Still, unexpected results could arise.
• Raising a controlled exception for anything not string (probably the best option here)

Judging past issues, it appears that all the confusion comes by the fact that this function is accepting any type (but can't handle it). The variety of errors for several types is big (for example, deprecation for null, warnings for arrays, etc...).

Personally, I would static type the function and return full errors on non strings, but since I know that static typing is not common in WP, I would prefer to deliver a controlled exception for incoming non strings.

@shanemac10 as @sabernhardt suggested, write Yoast with the code that is triggering this because they are basically sending something unsupported to this function (not a string), although this doesn't justify the fact that the result is not ideal, but it's not wrong either (and forcing string conversion could hide wrong passing wrong types to this function, generally I'm not very favorable to hiding expected errors)

Also review the GH PR again because there is no code. I've taken your proposed mod for the test

[ArtZ91]

I found that some bots triggering this E_DEPRECATED rtrim(): Passing null to parameter #1 ($string) of type string is deprecated just by GET request like https://mydomain//<SomeRandomString>.php (with two slashes after domain).

/wp-includes/formatting.php on line 2819

Upd. looks like it's related issue with All in One Wp Security plugin:

Array(
    [0] => Array(
        [function] => {closure}
    ),
    
    [1] => Array(
        [file] => /wp-includes/formatting.php
        [line] => 2819
        [function] => rtrim
    ),
    
    [2] => Array(
        [file] => /wp-content/plugins/all-in-one-wp-security-and-firewall/classes/wp-security-process-renamed-login-page.php
        [line] => 273
        [function] => untrailingslashit
    ),
    
    [3] => Array(
        [file] => /wp-content/plugins/all-in-one-wp-security-and-firewall/classes/wp-security-process-renamed-login-page.php
        [line] => 193
        [function] => is_renamed_login_page_requested
        [class] => AIOWPSecurity_Process_Renamed_Login_Page
        [type] => ::
    ),
    
    [4] => Array(
        [file] => /wp-content/plugins/all-in-one-wp-security-and-firewall/classes/wp-security-wp-loaded-tasks.php
        [line] => 21
        [function] => renamed_login_init_tasks
        [class] => AIOWPSecurity_Process_Renamed_Login_Page
        [type] => ::
    ),
    
    [5] => Array(
        [file] => /wp-content/plugins/all-in-one-wp-security-and-firewall/wp-security-core.php
        [line] => 493
        [function] => __construct
        [class] => AIOWPSecurity_WP_Loaded_Tasks
        [type] => ->
    ),
    
    [6] => Array(
        [file] => /wp-includes/class-wp-hook.php
        [line] => 324
        [function] => aiowps_wp_loaded_handler
        [class] => AIO_WP_Security
        [type] => ->
    ),
    
    [7] => Array(
        [file] => /wp-includes/class-wp-hook.php
        [line] => 348
        [function] => apply_filters
        [class] => WP_Hook
        [type] => ->
    ),
    
    [8] => Array(
        [file] => /wp-includes/plugin.php
        [line] => 517
        [function] => do_action
        [class] => WP_Hook
        [type] => ->
    ),
    
    [9] => Array(
        [file] => /wp-settings.php
        [line] => 749
        [function] => do_action
    ),
    
    [10] => Array(
        [file] => /wp-config.php
        [line] => 125
        [args] => Array(
            [0] => /wp-settings.php
        )
        [function] => require_once
    ),
    
    [11] => Array(
        [file] => /wp-load.php
        [line] => 50
        [args] => Array(
            [0] => /wp-config.php
        )
        [function] => require_once
    ),
    
    [12] => Array(
        [file] => /wp-blog-header.php
        [line] => 13
        [args] => Array(
            [0] => 
        )
    )
)

[SirLouen]

Generally this kind of plugins that are related to SEO lean towards manipulation of URL which could easily end in this error/deprecation notices.

As I commented in my test report, I truly believe that this should be handled with any of the 3 method proposed.

Personally I prefer static typing but I doubt this will be implemented this way because AFAIK WP rarely do this in any of the funtions.