[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  |     Focuses:
--------------------------------------------+---------------------
Changes (by jrf):

 * keywords:  has-patch has-unit-tests => has-patch has-unit-tests php81


Comment:

 At the request of @costdev, I've had a look at both
 [https://github.com/WordPress/wordpress-develop/pull/3132 GH #3132] as
 well as [https://github.com/WordPress/wordpress-develop/pull/3133 GH
 #3133].

 First things I noticed:
 * Both are missing a test for `null`.
 * [https://github.com/WordPress/wordpress-develop/pull/3133 GH #3133] has
 tests with floats, [https://github.com/WordPress/wordpress-
 develop/pull/3132 GH #3132] does not.
 * Neither has a test with a negative numeric value.
 * Also missing: a resource, but to be honest, I'm fine with not testing
 that.

 Realistically, this is not something which should be fixed in WP, but it
 should be fixed at the point of origin (where this function is called)
 instead.

 All the same, I understand the desire to prevent the PHP 8.1 "passing null
 to non-nullable" deprecation notice and the PHP 8.0 "passing array to
 string" warning.

 If you want my opinion, I would combine the two approaches:

 Basically use the patch from [https://github.com/WordPress/wordpress-
 develop/pull/3133 GH #3133], but add the `_doing_it_wrong()` from
 [https://github.com/WordPress/wordpress-develop/pull/3132 GH #3132] within
 the `if ( ! is_string( $string ) ) {` condition before returning.
 I would not special case `null`, but treat it the same as other non-scalar
 values.

 Leaving the `_doing_it_wrong()` out, as @peterwilsoncc
 [https://github.com/WordPress/wordpress-
 develop/pull/3132?#discussion_r954384359 suggests], would be hiding an
 error which should be fixed instead, which is bad practice and not helping
 devs.

 > Given the ease with which user submitted data can be turned in to an
 array by the user and the frequency with which is normalized to a non
 string by developers `$thing = ! empty( $_POST['t'] ) ? wp_unslash(
 $_POST['t'] ) : false;`

 To me, that's all the more reason for a `_doing_it_wrong(`) as we need to
 "teach" devs better input sanitation practices, not just to prevent these
 kind of PHP errors, but also to keep the users safe from hacks.

 > Off course the logical thing to do would be to fix the template code,
 but this is one of those badly designed templates with the code hard to
 read, ofuscated, splited, mixed, brain-melting, so I ended up having to
 add 1 line to WordPress file:

 @chocofc1 I understand and hear your frustration, but the fact that the
 template is bad is a reason to improve the template, not to try and fix a
 non-bug in WP.

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