WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 4 months ago

#30799 closed task (blessed) (fixed)

Scrutinizer in 4.2

Reported by: wonderboymusic Owned by: wonderboymusic
Milestone: 4.2 Priority: normal
Severity: normal Version: 4.2
Component: General Keywords:
Focuses: Cc:

Description

Continued efforts to improve static analysis of the codebase. Most commits will be minor changes that either remove dead code, improve comprehensibility of existing code, or clarify types specified by @param docs.

Attachments (2)

30799.diff (2.9 KB) - added by wonderboymusic 2 years ago.
30799.2.diff (53.4 KB) - added by wonderboymusic 2 years ago.

Download all attachments as: .zip

Change History (72)

#1 @wonderboymusic
2 years ago

In 30978:

Improve some wp-admin/includes/class-wp-filesystem-*.php docs for @param.

See #30799.

#2 @wonderboymusic
2 years ago

In 30979:

Improve some docs for @param. Remove an unneeded $wpdb global import.

See #30799.

#3 @wonderboymusic
2 years ago

In 30980:

In wp_import_handle_upload():

$file was essentially getting declared/overwritten 3 times. In lieu of this, return an array containing the error immediately instead of doing a short-circuit array key assignment on error. Rename the local variable to $upload and use its properties instead of creating 3 new local vars, one of which stomped the array.

See #30799.

#4 @wonderboymusic
2 years ago

In 30981:

In image_size_input_fields():

  • Declare $out as an empty array - this is not a strict PHP requirement, but is a good practice before loops
  • Most of this function was tabbed twice, instead of once

See #30799.

#5 @wonderboymusic
2 years ago

In 30982:

For clarity, initialize some arrays that previously were only assigned via short circuit in loops.

See #30799.

#6 @wonderboymusic
2 years ago

In 30983:

Correct the @param docs for arguments that are truthy/falsey.

See #30799.

#7 @wonderboymusic
2 years ago

In 30984:

Comment List Tables:

  • Declare $extra_items property
  • Since ->get_column_info() overrides its parent's method, it doesn't need to set _column_headers, which is an undeclared property that is only used for caching the lookup after the first run in the parent class, which isn't called in the child class.
  • $_args in WP_List_Table has to be protected for WP_Post_Comments_List_Table to legally access it. $_args is awkward as is and should probably be only available via a getter.

See #30799.

@wonderboymusic
2 years ago

#8 @wonderboymusic
2 years ago

In 31077:

Pinking Shears.

See #30799.

#9 @wonderboymusic
2 years ago

In 31078:

Access Modifiers:

  • In WP_Plugin_Install_List_Table, use public instead of var
  • In WP_User, ->data is accessed directly on an instance if the constructor receives it: make it public
  • In WP_Locale, every property is exported to a global and is already public via var, half of the properties are accessed directly already, make them all public
  • In WP_Rewrite, several properties are accessed publicly in functions via the $wp_rewrite global, make those props public.
  • In WP_Rewrite, the property ->comment_feed_structure was misspelled as ->comments_feed_structure

See #30799.

#10 @wonderboymusic
2 years ago

In 31079:

Perl-style comments should not be used

See #30799.

#11 @wonderboymusic
2 years ago

In 31081:

WP_Query->parse_tax_query() - for BC, this method is not marked as protected. See [28987]. It needs an access modifier, it shall have public. The comment remains.

See #30799.

#12 @wonderboymusic
2 years ago

In 31083:

In Customizer classes:

  • public final function methods should be final public function - confusing Hack and aligns with PSR2
  • Some methods were missing access modifiers

See #30799.

#13 @wonderboymusic
2 years ago

In 31085:

There is no need to use the final modifier inside a final class. Everything in it is final by default.

See #30799.

#14 @wonderboymusic
2 years ago

In 31086:

PHP keywords and constants "true", "false", "null" should be in lower case - there was one lingering capitalized false in _http_build_query().

See #30799.

#15 @wonderboymusic
2 years ago

In 31087:

In wp-includes/taxonomy.php:

The keyword elseif should be used instead of else if so that all control keywords look like single words.

See #30799.

#16 @wonderboymusic
2 years ago

In 31090:

The keyword elseif should be used instead of else if so that all control keywords look like single words.

This was a mess, is now standardized across the codebase, except for a few 3rd-party libs.

See #30799.

#17 @wonderboymusic
2 years ago

In 31091:

In wp-includes/plugin.php, collapse 3 else-newline-tab-if statements.

See #30799.

#18 @wonderboymusic
2 years ago

In 31092:

In wp_xmlrpc_server, remove dead code.

See #30799.

#19 @wonderboymusic
2 years ago

In 31099:

Use && instead of and in the 3 places where and was used.

See #30799.

#20 @wonderboymusic
2 years ago

In 31100:

Jump statements should not be followed by other statements (there were 5 lingering).

See #30799.

#21 @wonderboymusic
2 years ago

In 31101:

Overriding methods should do more than simply call the same method in the super class.

See #30799.

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


2 years ago

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


2 years ago

#25 @wonderboymusic
2 years ago

In 31116:

In Custom_Background and Custom_Header:

  • In ->init(), don't check current_user_can() since add_theme_page() will return false immediately if the cap check fails.
  • Bail if add_theme_page() returns false
  • wp_check_filetype_and_ext() doesn't need a 3rd param, it already defaults to null. Passing false would fail a strict check.

See #30799.

#26 follow-up: @wonderboymusic
2 years ago

In 31120:

Use PHP_SAPI constant instead of php_sapi_name() in iis7_supports_permalinks(), wp_fix_server_vars(), and wp_redirect().

See #30799.

#27 @wonderboymusic
2 years ago

In 31121:

Admin globals:

  • Declare $wp_importers as a global in admin.php
  • Declare $post_type, $post_type_object, and $post as globals where applicable

See #30799.

#28 @wonderboymusic
2 years ago

In 31122:

WP_Upgrader will take any "skin" that is passed to it, and set ->skin via composition. The default type of ->skin is WP_Upgrader_Skin, which doesn't have methods declared for ->bulk_header() and ->bulk_footer(). Add noop methods to WP_Upgrader_Skin.

See #30799.

#29 @wonderboymusic
2 years ago

In 31123:

In wp-admin/includes/revision.php, $post->modified is a coding error. It should be $post->post_modified.

See #30799.

#30 @wonderboymusic
2 years ago

In 31124:

Declare $wp_version, $required_php_version, and $required_mysql_version as globals in install and upgrade admin files.

See #30799.

#31 @wonderboymusic
2 years ago

In 31125:

Adding a @return annotation to constructors is generally not recommended as a constructor does not have a meaningful return value - anything that is returned is discarded.

See #30799.

#32 @wonderboymusic
2 years ago

In 31126:

Adding a @return annotation to constructors is generally not recommended as a constructor does not have a meaningful return value. Constructors do not have meaningful return values, anything that is returned from here is discarded.

See #30799.

#33 @wonderboymusic
2 years ago

In 31127:

Fix some @param docs that have chars too close them.
Add @property annotations to WP_User and WP_Post.
Remove erroneous @params from image editor class methods.
Officially add the property $_column_headers to WP_List_Table.

See #30799.

#34 @wonderboymusic
2 years ago

In 31128:

Declare $action as a global in wp-admin/comment.php.

See #30799.

#35 @wonderboymusic
2 years ago

In 31129:

Since get_theme_mod() returns false on failure, $background_image_thumb can be set to it and checked instead of calling get_background_image() 3 times in Custom_Background->admin_page().

See #30799.

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


2 years ago

#37 @wonderboymusic
2 years ago

In 31130:

get_header_image() can return false. In Custom_Image_Header->step_1(), check the value before setting the background-image portion of the style attribute. Setting the the URL to empty string will cause the current request to be set as the source of the background image.

See #30799.

#38 @wonderboymusic
2 years ago

In 31131:

In edit-form-advanced.php:

  • get_permalink( $post_ID ) can return false, set it to a variable and check it
  • Using the variable allows us to replace 11 separate calls to get_permalink( $post_ID ) in the file
  • These notices were triggered by the potential for false to be passed to esc_url()

See #30799.

#39 in reply to: ↑ 26 @SergeyBiryukov
2 years ago

Replying to wonderboymusic:

In 31120:

Use PHP_SAPI constant instead of php_sapi_name() in iis7_supports_permalinks(), wp_fix_server_vars(), and wp_redirect().

Just to clarify, what's wrong with php_sapi_name()?

#40 @wonderboymusic
2 years ago

In 31208:

WP_Filesystem_Base should declare $errors and $options as fields for use by subclasses.

See #30799.

#41 @wonderboymusic
2 years ago

In 31209:

In WP_Filesystem_Base subclasses that set $wp_base, remove this dead code.

Unused since [8009] - "Make WP_Filesystem work with new directory constants"

There is currently no declared field and no @property annotation.

See #30799.

#42 @wonderboymusic
2 years ago

In 31210:

Add 2 noop methods to WP_List_Table: ->column_default() and ->column_cb().

WP_List_Table is essentially an abstract class. Some of its methods throw die() warnings if they aren't overridden in a child class.

These noop methods wouldn't be abstract, because they are not required in subclasses. However, WP_List_Table can call these methods in its own method, ->single_row_columns(), whether a subclass defined them or not.

See #30799.

#43 @wonderboymusic
2 years ago

In 31211:

[31210] broke Supportflow on dotorg, which declares these methods as protected. Switch to protected for the noop methods. The subclasses can make them more visible using public.

See #30799.

#44 @wonderboymusic
2 years ago

In 31212:

@param cleanup:

  • get_metadata() will return literally anything, needs to be mixed
  • wp() and WP_Query::__construct() no longer just take a query string
  • Clarify a few others

See #30799.

#45 @wonderboymusic
2 years ago

In 31213:

By initializing this array before a loop, Scrutinizer reports 0 (zero) "Coding Style" errors.

There are plenty of other kinds of errors, but this label will be cleared out.

See #30799.

#46 @iandunn
2 years ago

re: r31211, I've updated SupportFlow to use public for column_default(), so you can switch back to public if you prefer that and aren't worried about other plugins.

#47 @wonderboymusic
2 years ago

In 31215:

Calling ->chown() and ->chgrp() in WP_Filesystem_FTPext produces no side-effects. Remove the calls. ->chgrp() is defined in the super class, and does nothing, can be removed from the child class.

See #30799.

#48 @wonderboymusic
2 years ago

In 31216:

Calling ->chown() and ->chgrp() in WP_Filesystem_ftpsockets produces no side-effects. Remove the calls. ->chgrp() is defined in the super class, and does nothing, can be removed from the child class.

See #30799.

#49 @wonderboymusic
2 years ago

In 31217:

Remove unused local vars in delete_plugins(), delete_theme(), WP_Date_Query->validate_date_values(), global_terms(), and WP_Text_Diff_Renderer_Table->_changed().

This will clear out the "Unused Code" label in the next Scrutinizer report.

See #30799.

#50 @wonderboymusic
2 years ago

In 31219:

Fix some erroneous @param annotations.

See #30799.

#51 @wonderboymusic
2 years ago

In 31220:

Fix some internal types that are passed to functions to avoid changing the acceptable types passed as arguments to those functions:

  • In WP_Importer->is_user_over_quota(), the default value for the first argument for upload_is_user_over_quota() is true. Don't bother passing 1.
  • When calling submit_button() with no $name, pass empty string instead of false.
  • The default value for the 2nd argument to get_edit_post_link() is 'display'. Because PHP is PHP, passing true is the same as passing 'display' or nothing. Don't bother passing true.
  • In WP_User_Meta_Session_Tokens::drop_sessions(), pass 0 instead of false to delete_metadata() as the value for $object_id, which expects an int.

See #30799.

#52 @wonderboymusic
2 years ago

In 31221:

wp_set_object_terms() takes $taxonomy as a string. Update @param.
See #30799.

#55 @wonderboymusic
2 years ago

In 31554:

Don't call the size function count() as part of a test condition in loops. Compute the size beforehand, and not on each iteration.

Scrutinizer added a Performance label: these are the only violations.

See #30799.

#56 @wonderboymusic
2 years ago

In 31555:

Cleanup the @property annotations for WP_User.

See #30799.

#57 @wonderboymusic
2 years ago

In 31672:

Use $this-> instead of parent:: when calling tablenav() in WP_Theme_Install_List_Table.

See #30799.

#58 @wonderboymusic
2 years ago

In 31678:

Avoid function calls on each iteration of a for loop.

See #30799.

#59 @wonderboymusic
2 years ago

In 31679:

Empty return statements are unnecessary at the end of functions.

See #30799.

#60 @wonderboymusic
2 years ago

In 31681:

There are a few functions that have the ability to return false instead of a string, so the return value should be checked before being passed to functions that expect string.

These are trivial, but they clear out some Scrutinizer issues.

See #30799.

#61 @wonderboymusic
2 years ago

In 31682:

Declare multisite as a field for WP_Object_Cache.

See #30799.

#62 @wonderboymusic
2 years ago

In 31683:

self should be used for accessing local static members.

See #30799.

#63 follow-up: @wonderboymusic
2 years ago

In 31684:

Remove the PHP4 constructor from WP_Widget.

See #30799.

#64 in reply to: ↑ 63 @nacin
2 years ago

Replying to wonderboymusic:

In 31684:

Remove the PHP4 constructor from WP_Widget.

See #30799.

This is guaranteed to break plugins.

#65 @wonderboymusic
2 years ago

will reverse

#66 @wonderboymusic
2 years ago

In 31685:

[31684] has BC issues, revert it.

Props nacin.
See #30799.

#67 @DrewAPicture
2 years ago

  • Owner set to wonderboymusic
  • Status changed from new to assigned
  • Type changed from defect (bug) to task (blessed)

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


2 years ago

#69 @DrewAPicture
2 years ago

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

Closing as fixed for 4.2 with @wonderboymusic's blessing. Good improvements all.

#70 @dd32
4 months ago

In 39687:

Upgrade: Fix the installation of TwentySeventeen upon upgrade from an early version.

This reverts part of [31124] which incorrectly caused $old_wp_version to equal the version of WordPress being upgraded to due to global variable access changes.

See #38551, #30799.
Fixes #39138 for trunk.

Note: See TracTickets for help on using tickets.