Skip to content

Admin Titles: avoid PHP warnings in PHP 8.1 #4308

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jeherve wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

Admin Titles: avoid PHP warnings in PHP 8.1 #4308

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Admin Titles: avoid PHP warnings in PHP 8.1 
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

- Related Core trac ticket: https://core.trac.wordpress.org/ticket/57579
- Related PR: #4084
  • Loading branch information
@jeherve
jeherve committed Apr 6, 2023
commit 075890a98df21e020c5dcdeb2557d2c0de9b4520
5 changes: 4 additions & 1 deletion src/wp-admin/admin-header.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,10 @@
}

get_admin_page_title();
$title = strip_tags( $title );

if ( ! is_null( $title ) ) {
$title = wp_strip_all_tags( $title ); // phpcs:ignore WordPress.WP.GlobalVariablesOverride.Prohibited
}
Comment on lines +36 to +39
Copy link
Author

@jeherve jeherve Apr 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since wp_strip_all_tags() now handles null (see related trac ticket and commit d46dc08), I suppose we could do without the extra check, and only replace strip_tags by wp_strip_all_tags:

Suggested change
if ( ! is_null( $title ) ) {
$title = wp_strip_all_tags( $title ); // phpcs:ignore WordPress.WP.GlobalVariablesOverride.Prohibited
}
$title = wp_strip_all_tags( $title ); // phpcs:ignore WordPress.WP.GlobalVariablesOverride.Prohibited

Copy link
Member

@jrfnl jrfnl Apr 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HIya @jeherve !

IMO, the pulled fix is not the right approach/fix for the following reason: $title may still be null afterwards.
This means, the problem is now moved to subsequent functions which may be handling $title and using PHP native functions to do so, so this is just setting WP up for the next bug report instead of fixing the actual issue that $title should always be set and set to a string.

As for the second proposal (comment above), which I do think is better, there is one thing I'd like to point out about that:

  • Yes, it will handle null silently, but for anything scalar but non-string, that will now yield a warning, which is a significantly higher error level than the original deprecation (though justifiable).

In both cases, the // phpcs:ignore ... comment needs to be removed as that WPCS rule does not apply to WP Core and we should not add unnecessary noise like that to the codebase.


if ( is_network_admin() ) {
/* translators: Network admin screen title. %s: Network title. */
Expand Down