Make WordPress Core

Opened 14 years ago

Closed 13 years ago

#17694 closed defect (bug) (fixed)

DRY: code to display update counts is redundant and buggy

Reported by: mitchoyoshitaka's profile mitchoyoshitaka Owned by: nacin's profile 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 14 years ago.
17694.2.diff (13.1 KB) - added by mitchoyoshitaka 14 years ago.
Patch v2, using function_exists( 'get_core_updates' )
17694.3.diff (13.1 KB) - added by mitchoyoshitaka 14 years ago.
Patch v3, merge with [18157]

Download all attachments as: .zip

Change History (9)

#1 @duck_
14 years ago

Nice.

Would function_exists( 'get_core_updates' ) be more appropriate that 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.

And just spotted this, not introduced by the patch, but why "n Themes Updates"? Note the plural "Themes" unlike "n Plugin Updates".

Version 0, edited 14 years ago by duck_ (next)

@mitchoyoshitaka
14 years ago

Patch v2, using function_exists( 'get_core_updates' )

#2 @mitchoyoshitaka
14 years ago

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

@mitchoyoshitaka
14 years ago

Patch v3, merge with [18157]

#3 @nacin
13 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.

#4 @MattyRob
13 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 13 years ago by scribu (previous) (diff)

#5 @nacin
13 years ago

  • Milestone changed from Future Release to 3.3

#6 @ryan
13 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.