Make WordPress Core

Opened 3 years ago

Last modified 3 years ago

#55662 new enhancement

Eliminating magic numbers

Reported by: cybr's profile Cybr Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: General Keywords:
Focuses: coding-standards Cc:

Description

Magic numbers in code are unique values with unexplainable meaning. These numbers can be confusing to the reader. To learn more, see: https://en.wikipedia.org/wiki/Magic_number_(programming)

Using this example:
$max_filesize = 5 * 1024 * 1024;

A common way to eliminate those numbers is by using either comments or constants. Example:
$max_filesize = 5 * 1024 * 1024; // 5 MB
$max_filesize = 5 * MB_IN_BYTES;

WordPress counts at least 20 instances of using 1024 in PHP, either meaning filesize, screen, or image dimensions. Using constant definitions would instantly differentiate their meaning when browsing the code.

Another example is fileupload_maxk, which defaults to 1500. If we define that size using a constant, we could easily update all related instances without having to inspect those manually: simply by updating the constant value. This cohesiveness aids in reducing regressions.

Change History (2)

#1 follow-up: @dd32
3 years ago

All of the occurrences of 1024 are within external libraries, or simply don't refer to Kilobytes.

Using the KB_IN_BYTES constant for those would be incorrect. The majority of the non-KB numbers happen to be the image constants, which while duplicative, centralising it within a constant wouldn't necessarily be beneficial or provide cleaner code, as the sizes should never be used for anything other than a function default (Generally, WordPress refers to things by the human-readable slug, such as 'large').

Here's a list of the not-in-external-code uses I quickly pulled:

$ grep 1024 wp-admin wp-includes -ri | grep -vE '(ID3|tinymce|plupload|sodium_compat|pclzip|css|.js|.crt)'
wp-admin/includes/schema.php:		'large_size_w'                    => 1024,
wp-admin/includes/schema.php:		'large_size_h'                    => 1024,
wp-admin/includes/update.php:			'width'     => 1024,
wp-admin/includes/media.php:		$large_size_h = 1024;
wp-admin/includes/media.php:		$large_size_w = 1024;
wp-admin/includes/media.php:	printf( __( 'Scale images to match the large size selected in %1$simage options%2$s (%3$d × %4$d).' ), $a, $end, (int) get_option( 'large_size_w', '1024' ), (int) get_option( 'large_size_h', '1024' ) );
wp-admin/includes/theme.php:				'width'     => 1024,
wp-includes/default-constants.php:	define( 'KB_IN_BYTES', 1024 );
wp-includes/default-constants.php:	define( 'MB_IN_BYTES', 1024 * KB_IN_BYTES );
wp-includes/default-constants.php:	define( 'GB_IN_BYTES', 1024 * MB_IN_BYTES );
wp-includes/default-constants.php:	define( 'TB_IN_BYTES', 1024 * GB_IN_BYTES );
wp-includes/default-constants.php:	define( 'PB_IN_BYTES', 1024 * TB_IN_BYTES );
wp-includes/default-constants.php:	define( 'EB_IN_BYTES', 1024 * PB_IN_BYTES );
wp-includes/default-constants.php:	define( 'ZB_IN_BYTES', 1024 * EB_IN_BYTES );
wp-includes/default-constants.php:	define( 'YB_IN_BYTES', 1024 * ZB_IN_BYTES );
wp-includes/rewrite.php:define( 'EP_TAGS', 1024 );
wp-includes/functions.php: * It is easier to read 1 KB than 1024 bytes and 1 MB than 1048576 bytes. Converts
wp-includes/functions.php: * Technically the correct unit names for powers of 1024 are KiB, MiB etc.

Regarding fileupload_maxk & 1500 it might make sense to swap that to a constant, it would be helpful to define a default for network-wide via code, however, there's not exactly many cases of it:

$ grep -E '(1500|fileupload_maxk)' wp-admin wp-includes -ri | grep -vE '(ID3|sodium_compat|.js|SimplePie|.css|custom-image)'
wp-admin/network/settings.php:		'fileupload_maxk',
wp-admin/network/settings.php:				<th scope="row"><label for="fileupload_maxk"><?php _e( 'Max upload file size' ); ?></label></th>
wp-admin/network/settings.php:							'<input name="fileupload_maxk" type="number" min="0" style="width: 100px" id="fileupload_maxk" aria-describedby="fileupload-maxk-desc" value="' . esc_attr( get_site_option( 'fileupload_maxk', 300 ) ) . '" />'
wp-admin/includes/ms.php:	if ( $file_size > ( KB_IN_BYTES * get_site_option( 'fileupload_maxk', 1500 ) ) ) {
wp-admin/includes/ms.php:		$file['error'] = sprintf( __( 'This file is too big. Files must be less than %s KB in size.' ), get_site_option( 'fileupload_maxk', 1500 ) );
wp-admin/includes/schema.php:		'fileupload_maxk'             => 1500,
wp-includes/rest-api/endpoints/class-wp-rest-attachments-controller.php:		if ( $file_size > ( KB_IN_BYTES * get_site_option( 'fileupload_maxk', 1500 ) ) ) {
wp-includes/rest-api/endpoints/class-wp-rest-attachments-controller.php:				sprintf( __( 'This file is too big. Files must be less than %s KB in size.' ), get_site_option( 'fileupload_maxk', 1500 ) ),
wp-includes/ms-functions.php:	if ( strlen( $upload['bits'] ) > ( KB_IN_BYTES * get_site_option( 'fileupload_maxk', 1500 ) ) ) {
wp-includes/ms-functions.php:		return sprintf( __( 'This file is too big. Files must be less than %s KB in size.' ) . '<br />', get_site_option( 'fileupload_maxk', 1500 ) );
wp-includes/ms-functions.php:	$fileupload_maxk = KB_IN_BYTES * get_site_option( 'fileupload_maxk', 1500 );
wp-includes/ms-functions.php:		return min( $size, $fileupload_maxk );
wp-includes/ms-functions.php:	return min( $size, $fileupload_maxk, get_upload_space_available() );

I'm more concerned about the one lonely default of 300 :)

#2 in reply to: ↑ 1 @SergeyBiryukov
3 years ago

Replying to dd32:

Regarding fileupload_maxk & 1500 it might make sense to swap that to a constant, it would be helpful to define a default for network-wide via code, however, there's not exactly many cases of it:
...
I'm more concerned about the one lonely default of 300 :)

Related: #27224

Note: See TracTickets for help on using tickets.