Make WordPress Core

Opened 4 months ago

Closed 4 months ago

#53493 closed enhancement (fixed)

Errors in `ms-files.php` aren't logged

Reported by: iandunn Owned by: iandunn
Milestone: 5.9 Priority: low
Severity: normal Version: 3.0
Component: Upload Keywords: early has-patch needs-testing
Focuses: multisite Cc:


r12936 added error_reporting(0) to ms-files.php. This makes troubleshooting problems like #53492 much harder.

I haven't groked #11742 yet, but 11742.2.diff describes the reason as:

Sets error_reporting to 0, removing requirement for a new SHORTLOAD constant and error suppression operators.

My guess is that the goal is to prevent PHP notices/warnings from breaking the image output. That makes sense, but it could be accomplished with ini_set( 'display_errors', 'off' ), without hiding errors from logs.

Change History (5)

#1 @iandunn
4 months ago

ticket:11742#comment:8 adds more context. Before 11742.2.diff, 11742.diff had taken this approach:

In order to be completely backwards compatible, I needed to create a new SHORTLOAD constant that is identical to SHORTINIT. This prevents an existing blogs.php drop-in that defines SHORTINIT from generating an unsuppressed E_NOTICE for redefining a constant.

So when error_reporting(0) was added in 11742.2.diff, the goal was really just to hide those specific notices.

However, 12742.5.diff intended it to hide all notices, similar to what the REST API, admin-alax.php, and XML-RPC do today:

Return error_reporting(0) to ms-files.php, reverting [13038]. Remove deprecated warnings as it breaks media file handling, and no translation support exists, reverting part of [12977].

#2 @iandunn
4 months ago

  • Keywords early has-patch needs-testing added
  • Milestone changed from Awaiting Review to 5.9
  • Owner set to iandunn
  • Status changed from new to accepted

Added a PR in https://github.com/WordPress/wordpress-develop/pull/1437, but it doesn't look like prbot pulled it in.

FYI @desrosj, @noisysocks

#3 @iandunn
4 months ago

it doesn't look like prbot pulled it in

Oh, it looks like it saw two Trac links, and chose the first and ignored the second. That makes sense.

#4 @iandunn
4 months ago

#53492 has reproduction steps

#5 @iandunn
4 months ago

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

In 51358:

Multisite: Log error/warnings/notices from ms-files.php.

Previously errors were not displayed or logged, but the original intention was only to prevent them from being displayed. Hiding them from logs makes problems like #53492 much harder to debug.

This makes the handling of errors in ms-files consistent with the REST API, admin-ajax, and XML-RPC.

Props iandunn, johnjamesjacoby.
Fixes #53493.

Note: See TracTickets for help on using tickets.