Make WordPress Core

Opened 6 months ago

Closed 6 months ago

Last modified 6 months 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: 6.7
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
6 months ago

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

#3 @sabernhardt
6 months 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
6 months 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
6 months 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
6 months 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
6 months 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
6 months 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
6 months 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
6 months 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
6 months ago

Awesome, appreciate quick resolution! Thanks to everyone involved.

Note: See TracTickets for help on using tickets.