[wp-trac] [WordPress Trac] #63379: Fix for deprecated rtrim passing null to parameter

[WordPress Trac]

#63379: Fix for deprecated rtrim passing null to parameter
-------------------------------------------------+-------------------------
 Reporter:  shanemac10                           |       Owner:  (none)
     Type:  defect (bug)                         |      Status:  new
 Priority:  normal                               |   Milestone:  Awaiting
                                                 |  Review
Component:  Formatting                           |     Version:  6.8
 Severity:  normal                               |  Resolution:
 Keywords:  has-patch has-testing-info dev-      |     Focuses:  php-
  feedback                                       |  compatibility
-------------------------------------------------+-------------------------
Changes (by SirLouen):

 * keywords:  close => has-patch has-testing-info dev-feedback


Comment:

 == 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
 [https://core.trac.wordpress.org/ticket/63379#comment:6 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)

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/63379#comment:8>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform