[wp-trac] [WordPress Trac] #56434: Check that the input is a string in wp_strip_all_tags()

[WordPress Trac]

#56434: Check that the input is a string in wp_strip_all_tags()
-------------------------------------------------+-------------------------
 Reporter:  chocofc1                             |       Owner:  (none)
     Type:  defect (bug)                         |      Status:  new
 Priority:  normal                               |   Milestone:  6.1
Component:  Formatting                           |     Version:  2.9
 Severity:  minor                                |  Resolution:
 Keywords:  has-patch has-unit-tests php81 2nd-  |     Focuses:
  opinion                                        |
-------------------------------------------------+-------------------------
Changes (by costdev):

 * keywords:  has-patch has-unit-tests php81 => has-patch has-unit-tests
     php81 2nd-opinion


Comment:

 Alright, so after adding back the `_doing_it_wrong()`, including for
 `null`, other tests began failing because of an unexpected
 `_doing_it_wrong()`.

 After some investigating, there's some fun about throwing
 `_doing_it_wrong()` when `$string` is `null`.

 -----

 1. Comments: `wp_handle_comment_submission()` can send a comment without
 an `author_url`, prompting the following path:
   - `wp_new_comment()`
   - `wp_allow_comment()`
   - `wp_filter_comment()`
   - `apply_filters( 'pre_comment_author_url' )`
   - `default-filters.php` adds `wp_strip_all_tags()` as a callback.
   - 💥 `_doing_it_wrong()` is thrown on `null`.
 2. Login: By default, the username is empty (read: `null`).
   - Visit the login page.
   - `wp_signon()`
   - `wp_authenticate()`
   - `sanitize_user()`
   - `wp_strip_all_tags()`
   - 💥 `_doing_it_wrong()` is thrown on `null`.

 These are just two of the paths where `null` can be passed by Core to
 `wp_strip_all_tags()`.

 -----

 So, at the moment, I see some possible options:
 1. We evaluate all 25 calls/callbacks of `wp_strip_all_tags()` in Core and
 lock down all potential edge cases.
   - This will take some time and a couple of contributors to do it
 correctly.
 2. We don't throw a `_doing_it_wrong()` on `null`, and simply return an
 empty string for the sake of BC.
   - This is bad for input validation. Essentially, Core hasn't validated
 the input to a function used for input validation, and Core itself has
 been passing the wrong type in certain circumstances.

 I'm curious to hear what others think about the two options above, as well
 as other options you might have in mind.

 -----

 Additional information:
 - There are [https://wpdirectory.net/search/01GBCGRZW8HBTR4NPCRYSHZQ29
 ~10.5k plugins] that call `wp_strip_all_tags()`.
 - There are [https://wpdirectory.net/search/01GBCGTGH0QR08EZK40WC11VM3
 ~2300 themes] that call `wp_strip_all_tags()`.
 - The type has been documented as `string` since the function was
 introduced in WordPress 2.9.

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