Make WordPress Core

Opened 8 months ago

Closed 10 days ago

Last modified 10 days 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
4 months ago

  • Type changed from enhancement to task (blessed)

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


4 months ago

#3 @swissspidy
4 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.


4 weeks 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
4 weeks 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 4 weeks ago by prasadkarmalkar (previous) (diff)

#6 @rajinsharwar
4 weeks 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:


4 weeks 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
4 weeks ago

I have Updated test cases for wp_opcache_invalidate_directory and clean_dirsize_cache

@hellofromTonya commented on PR #6621:


10 days 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
10 days 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:


10 days 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
10 days ago

  • Keywords commit added; changes-requested removed

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

Ready for commit. Preparing it now.

#13 @hellofromTonya
10 days 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:


10 days 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.