WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#17694 closed defect (bug) (fixed)

DRY: code to display update counts is redundant and buggy

Reported by: mitchoyoshitaka Owned by: nacin
Milestone: 3.3 Priority: normal
Severity: normal Version: 3.2
Component: Administration Keywords: has-patch 3.3-early
Focuses: Cc:

Description

Approximately the exact same code exists in three places for collecting information on available updates and formatting a UI string for it: wp_admin_bar_updates_menu(), wp-admin/menu.php, and in wp-admin/network/menu.php.

Let's not repeat ourselves.

In addition, the hacky cut-and-paste nature of these three instances of code actually cause weird edge case bugs. This patch also fixes some other minor issues:

  • in wp-admin/menu.php, if is_multisite() and !is_super_admin() but !empty( $menu_permsplugins? ), the $plugin_update_count variable will be undefined.
  • in wp-admin/network/menu.php, if !current_user_can( 'update_themes' ) but current_user_can( 'update_core' ), the variable $theme_update_count referenced in the sum for $update_count will not be defined. A similar issue could occur with $plugin_update_count.
  • in wp-admin/network/menu.php, if current_user_can( 'update_themes' ), we print a useless <span class='update-plugins count-0'><span class='theme-count'>0</span></span>. We actually have CSS to then hide this stuff (#adminmenu li span.count-0, #sidemenu li a .count-0). It just shouldn't be printed.
  • in wp-admin/network/menu.php, if !current_user_can( 'update_core' ), the total update count of themes and plugins is not printed in the Updates label.
  • wp_admin_bar_updates_menu() would only add the ab-updates menu if !current_user_can('install_plugins') instead of checking 'update_plugins' and not checking 'update_themes' or 'update_core' at all.

Attachments (3)

17694.diff (13.1 KB) - added by mitchoyoshitaka 3 years ago.
17694.2.diff (13.1 KB) - added by mitchoyoshitaka 3 years ago.
Patch v2, using function_exists( 'get_core_updates' )
17694.3.diff (13.1 KB) - added by mitchoyoshitaka 3 years ago.
Patch v3, merge with [18157]

Download all attachments as: .zip

Change History (9)

mitchoyoshitaka3 years ago

comment:1 duck_3 years ago

Nice.

Would function_exists( 'get_core_updates' ) be more appropriate than is_admin() to make it more obvious what's being checked for? We're not not displaying the core update on the frontend because it's not the admin, but because it would fatal error.

Not introduced by the patch, but why "n Themes Updates"? Note the plural "Themes" unlike "n Plugin Updates".

Last edited 3 years ago by duck_ (previous) (diff)

mitchoyoshitaka3 years ago

Patch v2, using function_exists( 'get_core_updates' )

comment:2 mitchoyoshitaka3 years ago

Thanks duck_. Updated patch to use function_exists( 'get_core_updates' ). Created ticket #17701 for the string issue.

mitchoyoshitaka3 years ago

Patch v3, merge with [18157]

comment:3 nacin3 years ago

  • Keywords 3.3-early added
  • Milestone changed from Awaiting Review to Future Release
  • Owner changed from mitchoyoshitaka to nacin
  • Status changed from new to accepted

Mitcho, remind me about this one after we branch 3.2.

comment:4 MattyRob3 years ago

Please also see #18098, I opened that ticket as the update number in different areas are disparate. Closing #18098 as duplicate though and I think this ticket will pick that up.

Last edited 3 years ago by scribu (previous) (diff)

comment:5 nacin3 years ago

  • Milestone changed from Future Release to 3.3

comment:6 ryan3 years ago

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

In [18468]:

Consolidate update count code into wp_get_update_data(). Props mitchoyoshitaka. fixes #17694

Note: See TracTickets for help on using tickets.