Make WordPress Core

Opened 18 months ago

Closed 14 months ago

Last modified 14 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 18 months ago.
Snippet
55926.diff (382 bytes) - added by SergeyBiryukov 18 months ago.

Download all attachments as: .zip

Change History (15)

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


18 months ago
#1

Fixed Fatal error for Network site,
Ref #55926

#2 @SergeyBiryukov
18 months ago

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

#3 @kebbet
18 months 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
18 months 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.

#5 @SergeyBiryukov
18 months 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 18 months ago by SergeyBiryukov (previous) (diff)

#7 @audrasjb
18 months ago

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

#8 @mjkhajeh
18 months 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.


15 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.


14 months ago

#11 @audrasjb
14 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
14 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.