Make WordPress Core

Opened 4 weeks ago

Closed 4 weeks ago

Last modified 4 weeks ago

#61873 closed defect (bug) (fixed)

Check if error_reporting function exists, otherwise breaks php8 installs when disabled in the ini config

Reported by: gansbrest's profile gansbrest Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.7 Priority: normal
Severity: normal Version: trunk
Component: Administration Keywords: has-patch
Focuses: administration Cc:

Description (last modified by sabernhardt)

Short summary of the problem: When error_reporting function is disabled on the hosting provider, it breaks admin area due to the fact that wp-admin/load-styles.php and wp-admin/load-scripts.php are triggering it without checking.

Some work has been done to resolve the problem on #52226

Unfortunately it doesn't address the issue completely as it only adds checks to the wp-load.php file.

Here is the great post that summarizes problem with error_reporting in WP for some additional context: https://phil.lavin.me.uk/2022/02/wordpress-changing-error_reporting-level/

Change History (11)

#2 @sabernhardt
4 weeks ago

  • Description modified (diff)
  • Keywords changes-requested removed

#3 @sabernhardt
4 weeks ago

Hi and welcome!

Could you remove the link to ticket 52226 from the PR summary, and edit the text surrounding it? The pull request automatically connected itself to the first Trac ticket mentioned there, and I think editing the summary would correct that.

#4 in reply to: ↑ description @SergeyBiryukov
4 weeks ago

Hi there, thanks for the ticket!

Replying to gansbrest:

Short summary of the problem: When error_reporting function is disabled on the hosting provider, it breaks admin area due to the fact that wp-admin/load-styles.php and wp-admin/load-scripts.php are triggering it without checking.

Some work has been done to resolve the problem on #52226

Unfortunately it doesn't address the issue completely as it only adds checks to the wp-load.php file.

As noted in the commit message for [50447]:

On systems with this function disabled, it's best to add a dummy function to the wp-config.php file, as there are multiple other calls in core or plugins.

However, as this call to the function is run prior to wp-config.php loading, it is now wrapped in a function_exists() check.

There are quite a few other error_reporting() calls in core, a quick search shows 21 matches in 11 files. However, defining an empty function in wp-config.php should resolve the issue without changing all those instances:

function error_reporting() {}

Afunction_exists() check was only added to wp-load.php is because it runs before wp-config.php is loaded.

#5 follow-up: @gansbrest
4 weeks ago

@SergeyBiryukov just wanted to clarify couple things.

I do use dummy function in wp-config.php, the problem still remains on admin pages (you can test it yourself by disabling error_reporting in the ini and navigating to /wp-admin pages).

Here is the example of direct call from wp-admin area:

GET /wp-admin/load-styles.php?c=1&dir=ltr&load%5Bchunk_0%5D=dashicons,admin-bar,common,forms,admin-menu,dashboard,list-tables,edit,revisions,media,themes,about,nav-menus,wp-pointer,widgets&load%5Bchunk_1%5D=,site-icon,l10n,buttons,wp-auth-check,wp-color-picker&ver=6.5.4

As you can see it calls /wp-admin/load-styles.php directly (similar to load-scripts.php calls).

If we look inside of the load-styles/scripts.php files (see my pull request above) we can notice that error_reporting is called before wp-config.php is initialized (hence the need for the PR).

It just throws fatal errors for each style or script.

I had to patch my WP installations with the additional function_exists checks I submitted in the PR here to get rid of fatal errors and restore functionality on PHP 8 setups ( with dummy function in wp-config.php of course ).

p.s. I did go through all of the remaining error_reporting calls in core and none of them seems to be breaking running websites with dummy error_reporting definition in the wp-config.php.

#6 @jrf
4 weeks ago

  • Focuses php-compatibility removed
  • Severity changed from major to normal

Just noting here for the record that adding error_reporting to the disabled functions is definitely not good practice and doesn't improve security at all.

#7 @martin.krcho
4 weeks ago

Adding the extra checks to load-styles.php and load-scripts.php seems sensible to me.

I completely agree with @jrf that disabling error_reporting function is not good practice. For me personally it's one of those weird edge cases WordPress "should" work around because it runs 43% of the web.

#8 @gansbrest
4 weeks ago

Agree with all of you that disabling error_reporting is definitely not the most common practice, but I've seen more than a few cases where it's in fact disabled.

In addition to that it's actually pretty convenient way to precisely control error_reporting via ini config in one place vs chasing error_reporting declarations across different modules.

Personally I don't see a harm if we add couple additional function_exists checks in the core, since we already did it for the wp-load.php, but open to other suggestions.


#9 in reply to: ↑ 5 @SergeyBiryukov
4 weeks ago

  • Milestone changed from Awaiting Review to 6.7
  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

Replying to gansbrest:

If we look inside of the load-styles/scripts.php files (see my pull request above) we can notice that error_reporting is called before wp-config.php is initialized (hence the need for the PR).

Ah yes, I missed that, thanks for clarifying! The PR makes sense to me.

#10 @SergeyBiryukov
4 weeks ago

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

In 58905:

Script Loader: Check if error_reporting() exists in load-(scripts|styles).php.

This avoids a fatal error on PHP 8 if error_reporting() is disabled in php.ini.

On systems with this function disabled, it's best to add a dummy function to the wp-config.php file, as there are multiple other calls in core or plugins.

However, as this call to the function is run prior to wp-config.php loading, it is now wrapped in a function_exists() check.

Follow-up to [50447].

Props gansbrest, sabernhardt, jrf, martin.krcho, SergeyBiryukov.
Fixes #61873.

#11 @gansbrest
4 weeks ago

Awesome, appreciate quick resolution! Thanks to everyone involved.

Note: See TracTickets for help on using tickets.