Make WordPress Core

Opened 11 years ago

Closed 10 years ago

Last modified 10 years ago

#27882 closed defect (bug) (fixed)

Address some issues found running Scrutinizer

Reported by: wonderboymusic's profile wonderboymusic Owned by:
Milestone: 4.0 Priority: normal
Severity: normal Version:
Component: General Keywords: needs-patch
Focuses: Cc:

Description

Scrutinizer is great: https://scrutinizer-ci.com/tour/measure-and-improve-code-quality

It found over 3000 problems with our code, we could perhaps heed its advice on one or two.

Attachments (3)

dead-code-ajax-actions.diff (3.0 KB) - added by wonderboymusic 11 years ago.
dead-code-list-tables.diff (3.6 KB) - added by wonderboymusic 11 years ago.
dead-code-admin-inc-file.diff (1.0 KB) - added by wonderboymusic 11 years ago.

Download all attachments as: .zip

Change History (94)

#1 @markoheijnen
11 years ago

Shouldn't we create tickets based on the advice instead of creating an unclear focussed ticket?

#2 @wonderboymusic
11 years ago

This is a placeholder ticket, Marko

#3 @markoheijnen
11 years ago

I get that. Doesn't mean this ticket is needed.

#4 @wonderboymusic
11 years ago

Thanks for the help, Marko

#5 @bpetty
11 years ago

  • Keywords close added

Marko is right, I've also been tracking Scrutinizer recently too, but until we have specific fixes in mind (and ideally patches to go with them), it's pointless to open a general "placeholder" ticket that can never be discussed (because it's not about specific issues), resolved, and closed. There will always be new Scrutinizer issues, and very likely several that will never be fixed for numerous reasons mostly related to back-compat.

FWIW, here's my public Scrutinizer report since none was even linked yet:
https://scrutinizer-ci.com/g/tierra/wordpress/issues/master

Please note that Scrutinizer has *not* been configured for known WP-specific rulesets, so much of these reports are going to be incorrect or not applicable, especially for 3rd party libs.

#6 @wonderboymusic
11 years ago

  • Milestone Future Release deleted
  • Resolution set to invalid
  • Status changed from new to closed

Only closing because I have no interest in this minutiae.

#7 follow-up: @Denis-de-Bernardy
11 years ago

Some of it seems pretty valid, though, e.g.:

https://scrutinizer-ci.com/g/tierra/wordpress/issues/master/files/src/wp-admin/customize.php?selectedLabels%5B0%5D=9&orderField=path&order=asc

"The variable $url seems only to be defined at a later point. Did you maybe move this code here without moving the variable definition?"

#8 in reply to: ↑ 7 @SergeyBiryukov
11 years ago

Replying to Denis-de-Bernardy:

"The variable $url seems only to be defined at a later point. Did you maybe move this code here without moving the variable definition?"

It's defined by wp_reset_vars(): tags/3.9/src/wp-admin/includes/misc.php#L263.

#9 @Denis-de-Bernardy
11 years ago

Ah, ya. So much for static analysis, I suppose… :-)

#10 @nacin
11 years ago

wonderboymusic was well within his rights and responsibilities (as a core committer, no less) to open up a tracking ticket for investigation. If you have nothing valuable to say, don't say it.

#11 @wonderboymusic
11 years ago

  • Keywords needs-patch added; close removed
  • Milestone set to 4.0
  • Resolution invalid deleted
  • Status changed from closed to reopened

#12 @wonderboymusic
11 years ago

In 28263:

Remove a dead variable, $start, from wp_dashboard_recent_comments().

See #27882.

#13 @wonderboymusic
11 years ago

In 28264:

Remove unused variable setting for $revisions in wp_prepare_revisions_for_js().

See #27882.

#14 @wonderboymusic
11 years ago

In 28265:

Remove duplicate variable setting for $link in wp_list_authors().

See #27882.

#15 @wonderboymusic
11 years ago

In 28268:

Dead code in wp-admin/includes/file.php:

  • In wp_handle_upload() and wp_handle_sideload(), $ext gets conditionally reset... and then is never used.
  • In request_filesystem_credentials(), $password is initialized as an empty string. The variable is never used.

See #27882.

#16 @wonderboymusic
11 years ago

In 28271:

In edit_form_image_editor(), $filename and $title are set and then unused. They shall be removed.

See #27882.

#17 @wonderboymusic
11 years ago

In 28274:

In general-template.php - remove dead code:

  • In get_calendar(), $cache is set twice. The first is useless.
  • In wp_default_editor(), $user is (accidentally) assigned in a condition. Since it is never used, this is unnecessary.
  • In language_attributes(), $output is set twice before it is ever used. The first is unnecessary.
  • In paginate_links(), $n is set twice before it is ever used. The first is unnecessary.

See #27882.

#18 @wonderboymusic
11 years ago

In 28279:

In WP_List_Table::display_rows_or_placeholder(), the call to get_column_info() in unnecessary and unused. The call to get_column_count() immediately after makes the same call internally.

See #27882.

#19 @wonderboymusic
11 years ago

In 28289:

Remove unreachable break statements in wp-admin/comments.php. break is unnecessary after die, exit, and wp_die.

See #27882.

#20 @wonderboymusic
11 years ago

In 28290:

In wp-admin/custom-header.php, $default_color is set twice before it is used. The first is unnecessary.

See #27882.

#21 @wonderboymusic
11 years ago

In 28291:

Remove unreachable break statements in wp-admin/edit-tags.php. break is unnecessary after die, exit, and wp_die.

See #27882.

#22 @wonderboymusic
11 years ago

In 28292:

In ajax-actions.php, remove dead code:

  • In wp_ajax_add_tag(), $post_type is set and never used.
  • In wp_ajax_hidden_columns(), $hidden is set twice, but only checks for the existence of $_POST['hidden'] the first time. The two lines can be combined and work together.
  • In wp_ajax_inline_save(), $mode is set and never used.
  • In wp_ajax_find_posts(), $searchand = $search = ''; is leftover cruft, neither variable is used. $wpdb does not need to be imported, it is never used.
  • In wp_ajax_wp_fullscreen_save_post(), $post_type is set and never used.
  • In wp_ajax_save_attachment_order(), $post is set and never used.
  • In wp_ajax_send_attachment_to_editor(), $title is set and never used.

See #27882.

#23 @wonderboymusic
11 years ago

In 28293:

In WP_Media_List_Table, remove dead code:

  • In ->prepare_items(), importing $wpdb is unnecessary.
  • In ->get_columns(), $taxonomies is set twice before it is used.
  • In ->display_rows(), $taxonomy_object is set and never used.

See #27882.

#24 @wonderboymusic
11 years ago

In 28296:

In WP_Users_List_Table::get_views(), $current_role is set and never used.

See #27882.

#25 @wonderboymusic
11 years ago

In 28297:

In get_media_item(), $tags and $_tags are set and never used.

See #27882.

#26 @wonderboymusic
11 years ago

In 28298:

In attachment_submit_meta_box(), remove dead code:

  • $action doesn't need to be imported.
  • $can_publish is set and never used, which means it doesn't need...
  • $post_type_object or $post_type, which only served $can_publish.

See #27882.

#27 @wonderboymusic
11 years ago

In 28299:

In update_option_new_admin_email(), $email is set and never used.

See #27882.

#28 @wonderboymusic
11 years ago

In 28300:

In wp-admin/includes/nav-menu.php, remove dead code:

  • In _wp_ajax_menu_quick_search(), in an else block, $post_obj was set and not used. Another block sets it again and does use it.
  • In wp_nav_menu_item_post_type_meta_box(), $post_type_object was set and, sadly, never used. $error was set and not used.

See #27882.

#29 @wonderboymusic
11 years ago

In 28301:

In install_plugin_information(), $title is set in a foreach loop and never used. This appears to be due to copy-pasting a previous foreach loop and not discarding these bits.

See #27882.

#30 @wonderboymusic
11 years ago

In 28302:

In get_sample_permalink_html(), $view_link is set and never used.

See #27882.

#31 @wonderboymusic
11 years ago

In 28303:

In WP_Screen::render_screen_options(), remove dead code:

  • $wp_list_table does not need to be imported.
  • $post is set and never used.

See #27882.

#32 @wonderboymusic
11 years ago

In 28304:

In wp-admin/includes/update.php, remove dead code:

  • In get_core_checksums(), $return is set and never used.
  • In core_update_footer(), break is unnecessary and unreachable after return.
  • In get_theme_updates(), $themes is set and never used.
  • In wp_theme_update_row(), $theme_name is the only user of $themes_allowedtags... neither are used.

See #27882.

#33 @wonderboymusic
11 years ago

In 28305:

In wp-admin/includes/upgrade.php, remove dead code:

  • In maybe_create_table() and maybe_add_column(), the response of $wpdb->query() is set to a variable that is never used.
  • In make_db_current_silent(), which is a wrapper for dbDelta, a variable is set that is not used. This function appears to be a copy-paste of make_db_current()'s dbDelta() call but is meant to return nothing, and still does not.

See #27882.

#34 @wonderboymusic
11 years ago

In 28306:

In display_setup_form(), $admin_password is set and (fortunately) not used or displayed.

See #27882.

#35 @wonderboymusic
11 years ago

In 28307:

In wp-admin/link.php, break is unreachabled after exit.

See #27882.

#36 @wonderboymusic
11 years ago

In 28308:

In wp-admin/network/themes.php, break is unreachabled after exit.

See #27882.

#37 @wonderboymusic
11 years ago

In 28309:

In wp-admin/network/users.php, break is unreachabled after exit.

See #27882.

#38 @wonderboymusic
11 years ago

In 28310:

In wp-admin/plugin-editor.php, break is unreachabled after exit.

See #27882.

#39 @wonderboymusic
11 years ago

In 28311:

In wp-admin/plugins.php, break is unreachabled after exit.

See #27882.

#40 @wonderboymusic
11 years ago

In 28312:

In wp-admin/post.php, break is unreachabled after exit.

See #27882.

#41 @wonderboymusic
11 years ago

In 28313:

In wp-admin/theme-editor.php, break is unreachabled after exit.

See #27882.

#42 @wonderboymusic
11 years ago

In 28314:

In core_upgrade_preamble(), $alternate is set then never used.

See #27882.

#43 @wonderboymusic
11 years ago

In 28315:

In wp-admin/users.php, break is unreachabled after exit.

See #27882.

#44 @wonderboymusic
11 years ago

In 28316:

In wp-includes/bookmark.php, remove dead code:

  • In get_bookmarks(), $cache is set twice before it is used.
  • In sanitize_bookmark_field(), break is unreachable after return

See #27882.

#45 @wonderboymusic
11 years ago

In 28317:

In WP_Http_Curl::request(), $theResponse is unused. There are other curl_exec() calls that do not return as well.

See #27882.

#46 @wonderboymusic
11 years ago

In 28318:

In wp-includes/class-wp-admin-bar.php, break is unreachabled after return.

See #27882.

#47 @wonderboymusic
11 years ago

In 28319:

In wp-includes/class-wp-customize-setting.php, break is unreachable after return.

See #27882.

#48 @wonderboymusic
11 years ago

In 28320:

In WP_Image_Editor::get_output_format(), $file_mime and $file_ext are set twice before they are used.

See #27882.

#49 @wonderboymusic
11 years ago

In 28322:

In Walker::walk() and Walker::paged_walk(), $id_field is set and never used.

See #27882.

#50 @wonderboymusic
11 years ago

In 28323:

In wp-includes/comment-template.php, remove dead code:

  • In get_comment_reply_link(), $link is set twice before it is used.
  • In Walker_Comment::start_lvl(), case 'ul': is unreachable unless placed before default:.
  • In Walker_Comment::end_lvl(), case 'ul': is unreachable unless placed before default:.
  • In comment_form(), $id is conditionally set and never used.

See #27882.

#51 @wonderboymusic
11 years ago

In 28324:

In wp_set_comment_status(), the default case returns, so no default value for $status is needed.

See #27882.

#52 @wonderboymusic
11 years ago

In 28325:

In wp-includes/functions.php, remove dead code:

  • In current_time(), break is unreachable after return.
  • In add_query_arg(), $ret is set twice before being used.
  • In wp_mkdir_p(), $dir_perms is set twice before being used.

See #27882.

#53 @wonderboymusic
11 years ago

In 28326:

In paginate_links(), break is unreachable after return.

See #27882.

#54 @wonderboymusic
11 years ago

In 28327:

In edit_post_link(), $post_type_obj is unused.

See #27882.

#55 @wonderboymusic
11 years ago

In 28328:

In update_metadata_by_mid(), $original_value is unused.

See #27882.

#56 @wonderboymusic
11 years ago

In 28329:

In get_network_by_path(), $exact_domains is unused.

See #27882.

#57 @wonderboymusic
11 years ago

In 28330:

In Walker_Nav_Menu::start_el(), $class_names is set twice before it is used.

See #27882.

#58 @wonderboymusic
11 years ago

In 28331:

In wp_get_attachment_link(), $post_title is set and then not used.

See #27882.

#59 @wonderboymusic
11 years ago

In 28332:

In get_pages(), $cache does not need to be reset to an empty array. update_post_cache( $pages ) takes care of priming.

In set_post_thumbnail(), (accidental) assignment is unnecessary for $thumbnail_html as it is not used.

See #27882.

#60 @wonderboymusic
11 years ago

In 28333:

In WP_Query, remove duplicate variable setting:

  • In ->parse_search_order(), a value is always set for $search_orderby, no need for empty initialization
  • In ->get_posts(), $fields is always set, no need for empty initialization

See #27882.

#61 @wonderboymusic
11 years ago

In 28334:

In wp-includes/revision.php - remove dead code:

  • In wp_get_post_autosave(), break is unreachable after return
  • In _wp_put_post_revision(), $post_id is set then never used.

See #27882.

#62 @wonderboymusic
11 years ago

In 28335:

In wp_default_scripts(), $max_upload_size and its entangled children $max_up and $max_post create quite the ternary operator... that is never used.

See #27882.

#63 @wonderboymusic
11 years ago

In 28336:

In print_admin_styles(), $zip is never used after being set. $compress_css does not need to be imported.

See #27882.

#64 @wonderboymusic
11 years ago

In 28337:

In wp-includes/theme.php, break is unreachable after return.

See #27882.

#65 @wonderboymusic
11 years ago

In 28338:

In WP_Widget::update_callback(), $sidebars_widgets is unused.

See #27882.

#66 @wonderboymusic
11 years ago

In 28339:

In wpdb, remove dead code:

  • In ->tables(), break is unreachable after return
  • In ->query(), $return is always set, so doesn't need an initial value of 0
  • In ->delete(), $bits is unused

See #27882.

#67 @wonderboymusic
11 years ago

In 28340:

In wp-login.php, break is unreachable after exit

See #27882.

#68 @wonderboymusic
10 years ago

In 28539:

These functions import $wpdb but do not use it.

See #27882.

#69 @wonderboymusic
10 years ago

After all of that Hack/HHVM cleanup, Scrutinizer now finds 500 less issues with the code. ~2700 down from ~3300.

#70 @wonderboymusic
10 years ago

In 28544:

$theme_name is set but no longer used in wp_dashboard_right_now(). The theme name is displayed with the call to update_right_now_message().

See #27882.

#71 @wonderboymusic
10 years ago

In 28545:

In wp_dashboard_recent_posts(), $i is set and never used.

See #27882.

#72 @wonderboymusic
10 years ago

In 28547:

break is unreachable in WP_Theme::translate_header().

See #27882.

#73 @wonderboymusic
10 years ago

In 28548:

In get_attachment_icon_src(), $class is set in 2 conditions but never used.

See #27882.

#74 @wonderboymusic
10 years ago

In 28549:

$gallery_div is set twice in gallery_shortcode() before it is used.

See #27882.

#75 @wonderboymusic
10 years ago

In 28550:

$count is set in get_post_galleries() and never used, relic from PFUI RIP.

See #27882.

#76 @wonderboymusic
10 years ago

In 28551:

In is_user_option_local(), $user_id is set conditionally, but never used.

See #27882.

#77 @wonderboymusic
10 years ago

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

Gonna mark this as fixed after 64 healthy commits. Almost all of the remaining issues are related to:

  • Top-level code, global variables. There a lot of variables that get set in a file and are assumed by us to spill down to other places (think, the admin). We should get away from top-level code, but we have tons of it.
  • Parameters to functions/methods that are unused but will remain for BC.
  • Parameters to methods that only exist in subclasses to match parent methods' signatures.
  • All kinds of garbage in the files we include from external libraries.
  • Some actual issues we can address in other ways outside of this ticket.

#78 @kitchin
10 years ago

I have an outstanding patch in #26604 for dashboard.php, to remove 6 lines of confusing dead code. I *also* speculated about deprecating unused dashboard functions and options, but the patch itself removes code that does nothing at all. Ping me there if you want a fresh patch.

#79 @wonderboymusic
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

There are a few new issues

#80 follow-up: @wonderboymusic
10 years ago

In 28631:

Adds a unit test to demonstrate that the order of case and default in a switch statement does not matter.

See #27882.

#81 @wonderboymusic
10 years ago

In 28636:

wp_xmlrpc_server::wp_getPage() should return new IXR_Error( instead of return(new IXR_Error(. One of the few places that is unparseable by static analysis.

See #27882.

#82 in reply to: ↑ 80 ; follow-up: @azaozz
10 years ago

Replying to wonderboymusic:

Adds a unit test to demonstrate that the order of case and default in a switch statement does not matter.

The order may not matter but makes it (much) harder to read/follow when default is not at the bottom. Same for the unreachable break; after return and die(). As far as I remember these were added for readability as switch() is one of the "harder to read" constructs, especially for inexperienced users. (Omitting a break merges the cases, some people miss or misunderstand that).

I don't particularly mind either way, but maybe we need to add some rules for switch() to the coding standards. Few years ago there were some.

Not sure we need a "demonstration test". Also it is not complete: it doesn't test the default case. A better demo would be something like:

function _switch_order_helper( $var ) { 
	switch ( $var ) { 
	default: 
		$return = 'default'; 
		break; 
	case 1: 
		$return = 'case 1'; 
		break; 
	} 

	return $return; 
}

function test_switch_order() { 
	$this->assertEquals( 'case 1', _switch_order_helper( 1 ) );
	$this->assertEquals( 'default', _switch_order_helper( 2 ) );
}
Last edited 10 years ago by azaozz (previous) (diff)

#83 follow-up: @wonderboymusic
10 years ago

The "demonstration tests" have been added to test assumptions in basic.php. We've never been burned by having too many unit tests, that's for sure.

#84 in reply to: ↑ 83 @azaozz
10 years ago

Replying to wonderboymusic:

Right, but for whom does it "demonstrate"? I doubt it the people that need this demonstration would actually look for it in the tests. Most likely they would go to php.net.

#85 in reply to: ↑ 82 @jdgrimes
10 years ago

Replying to azaozz:

The order may not matter but makes it (much) harder to read/follow when default is not at the bottom. Same for the unreachable break; after return and die(). As far as I remember these were added for readability as switch() is one of the "harder to read" constructs, especially for inexperienced users. (Omitting a break merges the cases, some people miss or misunderstand that).

I don't particularly mind either way, but maybe we need to add some rules for switch() to the coding standards. Few years ago there were some.

Maybe styling them something like this would improve readability?

switch ( $var ) {
	case 1:
		/* ... */
	break;

	case 2:
		/* ... */
	return;

	case 3:
		/* ... */
		// fallthru

	case 4:
		/* ... */
	exit;

	default:
		/* ... */
}

I think adding a comment whenever one case falls through into another is especially useful. But, you know, everybody has their own style.

#86 @wonderboymusic
10 years ago

The only 2 things I have an opinion on:

  1. can we standardize on default being at the bottom?
  2. Automation and static analysis are helpful ways for us to examine our whole code base. Every single tool complains about break after return|exit|die, whatever. We refactor switchs almost never. Is our process for code review so broken that a future case is guaranteed to carelessly fall through unless we proactively add extraneous breaks? We reopen tickets milliseconds after documentation changes have ambiguous grammatical errors. I may be outvoted on this one, but I think this is the least of our concerns - conversely, that same argument could have been made for me not to remove them in the first place. Sure, but I would rather us be able take advantage of more CI tools now and the future which is better for everyone.

#87 @kitchin
10 years ago

For readability, /* break; */ after return is as good as break; after return.

#88 @azaozz
10 years ago

The only 2 things I have an opinion on:

  • can we standardize on default being at the bottom?

Thinking we should. Much easier to read.

Sure, but I would rather us be able take advantage of more CI tools now and the future which is better for everyone.

Agreed. Not having unreachable break; is "the right way to go". We probably should require a comment when the break is missing on purpose though, as suggested by @jdgrimes above. // no break, // fall through, anything...

#89 @wonderboymusic
10 years ago

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

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.


10 years ago

#91 @michalzuber
10 years ago

In wp-admin/includes/class-wp-upgrader.php there are some undefined properties http://i.imgur.com/Lxipd0Y.png

Note: See TracTickets for help on using tickets.