Make WordPress Core

Opened 2 years ago

Closed 22 months ago

Last modified 22 months ago

#55926 closed defect (bug) (fixed)

Fatal error after entered the empty value for 'Max upload file size' in wordpress network

Reported by: mjkhajeh's profile mjkhajeh Owned by: audrasjb's profile audrasjb
Milestone: 6.1 Priority: normal
Severity: critical Version:
Component: Networks and Sites Keywords: has-patch php8 has-unit-tests commit
Focuses: administration, multisite Cc:

Description

Hi, I entered the empty value for 'Max upload file size' in the wordpress network and WordPress is down.

<b>Fatal error</b>:  Uncaught TypeError: Unsupported operand types: string * int in /var/www/html/wordpress/wp-includes/ms-functions.php:2709
Stack trace:
#0 /var/www/html/wordpress/wp-includes/class-wp-hook.php(309): upload_size_limit_filter()
#1 /var/www/html/wordpress/wp-includes/plugin.php(191): WP_Hook-&gt;apply_filters()
#2 /var/www/html/wordpress/wp-includes/media.php(3751): apply_filters()
#3 /var/www/html/wordpress/wp-includes/block-editor.php(156): wp_max_upload_size()
#4 /var/www/html/wordpress/wp-includes/class-wp-theme-json-resolver.php(199): get_default_block_editor_settings()
#5 /var/www/html/wordpress/wp-includes/class-wp-theme-json-resolver.php(384): WP_Theme_JSON_Resolver::get_theme_data()
#6 /var/www/html/wordpress/wp-includes/script-loader.php(3076): WP_Theme_JSON_Resolver::get_merged_data()
#7 /var/www/html/wordpress/wp-includes/script-loader.php(3272): {closure}()
#8 /var/www/html/wordpress/wp-includes/class-wp-hook.php(307): {closure}()
#9 /var/www/html/wordpress/wp-includes/class-wp-hook.php(331): WP_Hook-&gt;apply_filters()
#10 /var/www/html/wordpress/wp-includes/plugin.php(476): WP_Hook-&gt;do_action()
#11 /var/www/html/wordpress/wp-settings.php(620): do_action()
#12 /var/www/html/wordpress/wp-config.php(103): require_once('...')
#13 /var/www/html/wordpress/wp-load.php(50): require_once('...')
#14 /var/www/html/wordpress/wp-admin/admin.php(34): require_once('...')
#15 /var/www/html/wordpress/wp-admin/network/admin.php(13): require_once('...')
#16 /var/www/html/wordpress/wp-admin/network/settings.php(11): require_once('...')
#17 {main}
  thrown in <b>/var/www/html/wordpress/wp-includes/ms-functions.php</b> on line <b>2709</b><br />

For updating options, I use default values, compare new values with default, and then update them. Maybe this method also works for you:

wp-admin/network/settings.php:111

<?php
$defaults = [
        'fileupload_maxk'       => 1500,
];

wp-admin/network/settings.php:123(After inserting $defaults. otherwise: 121)

<?php
if ( ! isset( $_POST[ $option_name ] ) && !isset( $defaults[$option_name] ) ) {
        continue;
} else if( isset( $defaults[$option_name] ) ) {
        $value = $defaults[$option_name];
}

Attachments (2)

wordpress-debug-snippet.png (339.2 KB) - added by mjkhajeh 2 years ago.
Snippet
55926.diff (382 bytes) - added by SergeyBiryukov 2 years ago.

Download all attachments as: .zip

Change History (15)

@mjkhajeh
2 years ago

Snippet

This ticket was mentioned in PR #2787 on WordPress/wordpress-develop by bhrugesh96.


2 years ago
#1

Fixed Fatal error for Network site,
Ref #55926

#2 @SergeyBiryukov
2 years ago

  • Keywords php8 added
  • Milestone changed from Awaiting Review to 6.1

#3 @kebbet
2 years ago

  • Keywords needs-refresh added

The linked PR seems to have unrelated changes in src/wp-content/themes/twentyseventeen/assets/css/blocks.css. So marking for needs refresh.

#4 @audrasjb
2 years ago

I refreshed the PR to remove unwanted changes, but I think we could also make the inline comment more generic, just in case we want to use it for other default values in the future.

@SergeyBiryukov
2 years ago

#5 @SergeyBiryukov
2 years ago

  • Keywords needs-unit-tests added

Hi there, thanks for the ticket and the PR!

I'm not sure adding a default value for fileupload_maxk on Network Settings screen is the right approach here:

  • It's not quite clear why of 20+ settings on that page, only one should have a default value.
  • The current PR, unless I'm missing something, would always save the 1500 value instead of the entered value.

I think a better solution would be to sanitize fileupload_maxk with absint() in sanitize_option(), like we already do for other numeric options, see 55926.diff. That would replace an empty value with zero, which seems to match the intention of entering an empty value in the first place.

Since upload_size_limit_filter() is the function that throws a fatal error here, a unit test for it would also be great.

Last edited 2 years ago by SergeyBiryukov (previous) (diff)

#7 @audrasjb
2 years ago

Indeed, sanitizing the value with absint() seems like the best option so far.

#8 @mjkhajeh
2 years ago

That's right. So it can be good if you check other options too. isn't it?

This ticket was mentioned in PR #3271 on WordPress/wordpress-develop by felipeelia.


22 months ago
#9

  • Keywords has-unit-tests added; needs-refresh needs-unit-tests removed

This PR uses the solution from https://core.trac.wordpress.org/attachment/ticket/55926/55926.diff and also casts the fileupload_maxk site option to an int in upload_size_limit_filter(). It also adds some unit tests for that function.

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

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


22 months ago

#11 @audrasjb
22 months ago

  • Keywords commit added
  • Owner set to audrasjb
  • Status changed from new to accepted

PR 3271 combines the best of both approaches :D
Let's go with this. I'll give it a quick test then commit.

#12 @audrasjb
22 months ago

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

In 54482:

Networks and Sites: Ensure fileupload_maxk is an int to avoid potential fatal errors.

This changeset fixes a potential fatal error, for example when "Max upload file size" setting is set to an empty value. It also adds unit tests for upload_size_limit_filter.

Props mjkhajeh, bhrugesh12, SergeyBiryukov, kebbet, audrasjb, felipeelia.
Fixes #55926.

Note: See TracTickets for help on using tickets.