[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                                        |
-------------------------------------------------+-------------------------

Comment (by costdev):

 > @peterwilsoncc  Calling sanitization functions is, unambiguously, the
 right thing to do so telling developers they are doing the wrong thing
 does not help.

 `_doing_it_wrong()` tells developers that they are doing a thing
 incorrectly, not that they are doing the wrong thing altogether. i.e. a
 function/method was called incorrectly, not "you tried to sanitize, bad
 dev".

 Letting developers know when they've made a mistake is definitely helpful,
 especially if they're trying to do a good thing, but a mistake makes it
 ineffective. It would remain ineffective if the function failed silently.

 > @dd32 I would note that wp_strip_all_tags() is not a sanitization
 function IMHO though..

 I'd say it's context-dependent - e.g. `_sanitize_text_fields()` and
 `sanitize_user()` use it. It's probably safe to say that it's at least a
 ''helper'' for sanitization if not used as a sanitization function itself.

 -----

 Some thoughts on the discussion:

 - Returning `false` for non-scalar makes sense. However, note that this
 will change the function's return type to `string|false`, and as
 truthy/falsy/empty() checks are inappropriate here (consider `<b>0</b>`),
 it would break BC.
 - Silently handling incorrect usage doesn't help developers track down why
 they're getting unexpected output, particularly when there's a long trace.

 IMO, the appropriate solution is the following:

 Scalar - Continue to cast to string.
 Non-scalar (except `null`) - Call `_doing_it_wrong()` and return `''`.
 `null` - see [https://core.trac.wordpress.org/ticket/56434#comment:7
 comment 7]. Need opinions.

 - Protects BC for scalar.
 - Protects sites from a fatal error for non-scalar in PHP 8+.
 - Informs developers of mistakes, without terminating execution in PHP 8+.
 - Only those who want to see debug messages will see them.

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