Make WordPress Core

Opened 16 months ago

Last modified 4 months ago

#57579 new defect (bug)

Replace most strip_tags() with wp_strip_tags()

Reported by: ipajen's profile ipajen Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: General Keywords: php81 has-patch changes-requested needs-unit-tests
Focuses: php-compatibility Cc:

Description (last modified by hellofromTonya)

Originally reported as an issue with strip_tags() in wp-admin/admin-header.php, this ticket's scope as grown to initiative to replace strip_tags() with wp_strip_tags().

Proposal:

  • Replace most calls to strip_tags() with calls to wp_strip_all_tags()

@peterwilsoncc notes:

this is a slight compatibility change as it will strip the contents of <style> and <script> tags, but I don't think that's a bad thing

  • Add an early check in wp-admin/admin-header.php to set up the global variables.

@jrf notes:

I can see one more alternative option, which could (should) possible be applied in conjunction with the solution suggested by @peterwilsoncc: for wp-admin/admin-header.php to do an early check for the global variables it requires to be set and to set those to a default value (for example, an empty string for $title) if the variable is not available.

The original reported issue:

When using the plugin HD Quiz this warning is given in version 6.2-alpha-55159 with PHP 8.1.

Deprecated: strip_tags(): Passing null to parameter #1 ($string) of type string is deprecated in /public_html/blog/wp-admin/admin-header.php on line 36

Link to ticket for plugin:
https://wordpress.org/support/topic/strip_tags-php8-1-warning/#new-topic-0

"Core expects the $title global to always be set when wp-admin/admin-header.php is included." Is there something Core can do to improve the experience here?

Change History (16)

#1 @peterwilsoncc
16 months ago

  • Component changed from Plugins to General
  • Milestone changed from Awaiting Review to 6.2

This is due to a change to strip_tags in PHP which started throwing a warning when null is passed to the function, and a fatal error passed for other values. See this example page for how PHP has changed error handling for the function.

Folks are currently discussing how to handle this in the helper function wp_strip_all_tags() on the ticket #56434.

As WordPress calls strip_tags() directly in a number of cases, on this ticket let's discuss how to handle the deprecation warning elsewhere.

I see two options:

  1. replace most calls to strip_tags() with calls to wp_strip_all_tags() -- this is a slight compatibility change as it will strip the contents of <style> and <script> tags, but I don't think that's a bad thing
  2. add wp_strip_tags() as a wrapper for strip_tags() to maintain the current behavior

FWIW, I think option 1 is preferable as I think that's closer to the intended use of the function throughout WordPress.

Thanks for the ticket, I've moved it to the 6.2 milestone for visibility and consideration.

#2 follow-up: @jrf
16 months ago

Thanks @ipajen for reporting this!

I concur with @peterwilsoncc that option 1 is the preferred path forward, though also want to mention this sounds like this is primarily an issue with a plugin doing_it_wrong and that the plugin should be fixed (as well).

I can see one more alternative option, which could (should) possible be applied in conjunction with the solution suggested by @peterwilsoncc: for wp-admin/admin-header.php to do an early check for the global variables it requires to be set and to set those to a default value (for example, an empty string for `$title) if the variable is not available.

What do you think @peterwilsoncc ?

#3 in reply to: ↑ 2 @SergeyBiryukov
16 months ago

Replying to jrf:

I can see one more alternative option, which could (should) possible be applied in conjunction with the solution suggested by @peterwilsoncc: for wp-admin/admin-header.php to do an early check for the global variables it requires to be set and to set those to a default value (for example, an empty string for `$title) if the variable is not available.

Indeed, that sounds like a good idea too. We already do this for $current_screen:

// Catch plugins that include admin-header.php before admin.php completes.
if ( empty( $current_screen ) ) {
	set_current_screen();
}

Might be worth doing the same for other variables, though I think I would do this on a case-by-case basis for those that are known to cause notices if not set, like $title here.

Last edited 16 months ago by SergeyBiryukov (previous) (diff)

#4 @peterwilsoncc
16 months ago

@SergeyBiryukov @jrf Yeah, that makes sense too.

This ticket was mentioned in PR #4084 on WordPress/wordpress-develop by zamandevs.


15 months ago
#5

  • Keywords has-patch added

#6 @zamandevs
15 months ago

The above issue causing due to get_admin_page_title() producing a null.
For other strip_tags uses we can consider wp_strip_all_tags() as a replacement.

This ticket was mentioned in Slack in #core by costdev. View the logs.


15 months ago

#8 @costdev
15 months ago

  • Milestone changed from 6.2 to 6.3

This ticket was discussed in the bug scrub and it was agreed to move this ticket to 6.3.

This ticket was mentioned in PR #4308 on WordPress/wordpress-develop by @jeherve.


14 months ago
#9

This avoids the following warning on pages where a title is not defined:

PHP Deprecated:  strip_tags(): Passing null to parameter #1 ($string) of type string is deprecated in /wp-admin/admin-header.php on line 36

This should not normally happen, but some folks may pass null to add_submenu_page() when creating new pages, to hide that page from the admin menu. This may not be the best approach, but it is one that is documented in the codex and used in the wild: https://developer.wordpress.org/reference/functions/add_submenu_page/#comment-445

#10 @jeherve
14 months ago

I ran into the same issue on my end; I'm glad to see there is already a ticket and a proposed patch!

In the Pull Request linked above, I took a different approach from the original patch to fix that specific deprecation notice.

This ticket was mentioned in Slack in #core by oglekler. View the logs.


11 months ago

#12 @oglekler
11 months ago

  • Keywords changes-requested needs-unit-tests added
  • Milestone changed from 6.3 to Future Release

This ticket was discussed in the recent bug scrub and due to lack of activity it was decided to move it into future release.

The solution still needs to be worked out. It also needs a unit test that only runs on PHP 8.1+ to verify that no deprecation notice is fired.

It can be moved to the next available milestone when it will be ready or into the current 6.3 if it will be ready before RC1.

Props: @costdev @Clorith

This ticket was mentioned in Slack in #core-php by ipajen. View the logs.


10 months ago

#14 @hellofromTonya
10 months ago

  • Keywords php81 added; PHP81 removed

#15 @hellofromTonya
10 months ago

  • Description modified (diff)
  • Focuses php-compatibility added
  • Summary changed from Deprecated: strip_tags(): Passing null to parameter #1 ($string) of type string is deprecated in admin-header.php to Replace most strip_tags() with wp_strip_tags()

This ticket's scope has grown past the original reported issue. Thus, I've updated the Summary and Description to help contributors know what is being proposed and the work yet to be done.

I've also added the php-compatibility focus to include strip_tags() usage in Core in the PHP 8.1 compat exclusion list.

#16 @slimndap
4 months ago

This should not normally happen, but some folks may pass null to add_submenu_page() when creating new pages, to hide that page from the admin menu.

This also happens if you pass an empty string ('') as the first parameter ($parent_slug) to add_submenu_page().

Note: See TracTickets for help on using tickets.