Make WordPress Core

Opened 16 months ago

Closed 8 months ago

Last modified 8 months ago

#59652 closed task (blessed) (fixed)

Replace trigger_error() with wp_trigger_error() for 6.6

Reported by: hellofromtonya's profile hellofromTonya Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.6 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch commit
Focuses: Cc:

Description

This ticket tracks an initiative to replace each instance of trigger_error() with wp_trigger_error(). The initiative seeks to avoid generating E_USER family errors unless WP_DEBUG is on. In doing so, users should not see these messages in normal production.

Previous releases:

References:

Change History (14)

#1 @swissspidy
12 months ago

  • Type changed from enhancement to task (blessed)

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


12 months ago

#3 @swissspidy
12 months ago

  • Milestone changed from 6.5 to 6.6
  • Summary changed from Replace trigger_error() with wp_trigger_error() for 6.5 to Replace trigger_error() with wp_trigger_error() for 6.6

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


9 months ago
#4

  • Keywords has-patch added

In this PR I have replaced trigger_error with wp_trigger_error and for the function name first parameter in wp_trigger_error I have used METHOD and FUNCTION based on either this is in function or class

Trac ticket: https://core.trac.wordpress.org/ticket/59652

#5 @prasadkarmalkar
9 months ago

Hi @swissspidy , I have Raised PR for this issue. In this PR I have replaced trigger_error with wp_trigger_error and for the function name first parameter in wp_trigger_error.

Last edited 9 months ago by prasadkarmalkar (previous) (diff)

#6 @rajinsharwar
9 months ago

  • Keywords changes-requested added

Hi @prasadkarmalkar, thanks for the PR. As you might be aware, wp_trigger_error can also return HTML tags, so we need to strip the tags before using it. That's the reason the unit tests are failing.

EG:

wp_strip_all_tags( wp_trigger_error( __FUNCTION__, $error_message ) );

@thelovekesh commented on PR #6621:


9 months ago
#7

Hi @Rajinsharwar!

Is there a specific reason to strip HTML tags from triggered error messages? I believe the HTML tags are expected, allowing the front end to render the error messages makeup(e.g. links) appropriately for the user.

Regarding the failed tests, wp_trigger_error() includes the method or function name in the error message, which is causing the tests to fail.

https://github.com/WordPress/wordpress-develop/blob/f1c0e5856e71f1431e1cd1ef9d6775bb8495b532/src/wp-includes/functions.php#L6065

_Please note, how function/method name is being appended in the front of error message._

I believe the test cases need to be updated to account for the __FUNCTION__ name prefix if it is being passed to wp_trigger_error().

#8 @prasadkarmalkar
9 months ago

I have Updated test cases for wp_opcache_invalidate_directory and clean_dirsize_cache

@hellofromTonya commented on PR #6621:


8 months ago
#9

Hello and thanks for working on this effort 👋 Some of the changes are in external packages / libraries that are maintained outside of Core, e.g. Requests, PHPMailer, SimplePie, etc.

For this first round of replacements (especially this late in the beta cycle), I'm thinking to only replace within Core's maintained codebase. For these external libraries, discussion can happen whether in their repo (such as with Requests) or within 6.7+ trac ticket.

I'll push a change to this PR shortly that reverts the changes to external libraries. And then review the remaining changes for 6.6 commit consideration.

#10 @hellofromTonya
8 months ago

  • Owner set to hellofromTonya
  • Status changed from new to reviewing

Patch: https://github.com/WordPress/wordpress-develop/pull/6621

Self-assigning ownership for review and 6.6 commit consideration.

@hellofromTonya commented on PR #6621:


8 months ago
#11

I did a force push because it easiest and most clean way for me to restore the changes in external libraries from the original first commit https://github.com/WordPress/wordpress-develop/pull/6621/commits/c99f02736e745cddc5e23cea572a2d8a542d5d2b. Also rebased it on top of the current WP trunk just in case.

#12 @hellofromTonya
8 months ago

  • Keywords commit added; changes-requested removed

Patch: https://github.com/WordPress/wordpress-develop/pull/6621

Ready for commit. Preparing it now.

#13 @hellofromTonya
8 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 58409:

Code Modernization: Use wp_trigger_error() instead of trigger_error().

Replaces trigger_error() with wp_trigger_error().

The usage of wp_trigger_error() avoids generating E_USER family errors unless WP_DEBUG is on. In doing so, users should not see these messages in normal production.

Notes:

  • Removes E_USER_NOTICE when passed as an argumnent, as it's the default error level.
  • An empty string is passed for the function name when its name is already in the error message or does not add value to the error message.
  • Externally maintained libraries are not included.

Follow-up to [55204], [25956], [29630], [38883], [52062], [52049], [54272], [38883], [55245], [51599], [14452], [38883], [24976].

Props prasadkarmalkar, rajinsharwar, thelovekesh, hellofromTonya, swissspidy.
Fixes #59652.

@hellofromTonya commented on PR #6621:


8 months ago
#14

Thank you everyone for your contributions 🌟

Committed via https://core.trac.wordpress.org/changeset/58409 to ship in 6.6.

Note: See TracTickets for help on using tickets.