Make WordPress Core

Opened 15 years ago

Closed 8 years ago

#13071 closed defect (bug) (fixed)

Update bubble appears only at the second load

Reported by: ocean90's profile ocean90 Owned by: swissspidy's profile swissspidy
Milestone: 4.7 Priority: normal
Severity: normal Version: 3.0
Component: Upgrade/Install Keywords: shiny-updates has-patch commit
Focuses: administration Cc:

Description

Change the version of a plugin and go directly to wp-admin/update-core.php page.
You will see the plugin update in the list but the bubbles on the menu appears only after a reload.

Attachments (7)

update.diff (886 bytes) - added by bswatson 10 years ago.
13071.png (414.1 KB) - added by ocean90 9 years ago.
13071.patch (772 bytes) - added by ocean90 9 years ago.
13071.2.patch (1.7 KB) - added by ocean90 9 years ago.
13071.diff (9.1 KB) - added by swissspidy 8 years ago.
13071.2.diff (10.9 KB) - added by swissspidy 8 years ago.
13071.3.diff (13.4 KB) - added by ocean90 8 years ago.

Download all attachments as: .zip

Change History (50)

#1 @ocean90
15 years ago

IRC log:

[18 Apr 10 18:25] * nacin * yeah, they're a page load behind.
[18 Apr 10 18:36] * nacin * ocean90: If we switch two giant code blocks in plugins.php, that one pageload delay goes away. I don't want to break anything else though at this point in the cycle
[18 Apr 10 18:37] * nacin * more or less, moving the admin-header.php call (and related code = 70 lines) down below the block that collects all the plugin information (= 100 lines)
[18 Apr 10 18:39] * ocean90 * nacin: so should i create a ticket at first?
[18 Apr 10 18:39] * nacin * If you want. set for 3.1
[18 Apr 10 18:39] * ocean90 * ok
[18 Apr 10 18:40] * nacin * It's low priority, rather minor, but easy to fix. just need to review 200 lines of code to make sure nothing would break.

#2 @nacin
15 years ago

Related to this, if you run an upgrade, at the end of the upgrade, some JS should run that reduces the bubble count by 1 (or more if bulk), or clear the bubble all together if that's the case.

Both the Dashboard > Updates bubble and the Plugins bubble.

#3 @nacin
14 years ago

  • Milestone changed from Awaiting Triage to Future Release

#4 @Viper007Bond
14 years ago

Same thing happens if you directly visit /wp-admin/plugins.php?plugin_status=upgrade when there's an update available. The yellow bar won't be there and you'll need to refresh before you can update.

#5 @ocean90
14 years ago

  • Keywords needs-patch added
  • Priority changed from low to normal

#6 @dd32
14 years ago

Similar (with a patch): #10884

In this case, any update checks hooked to a page/run on a page, need to be hooked before admin menu generation (which the load-$pagenow hook doesnt).

#7 @chriscct7
10 years ago

  • Keywords reporter-feedback added

Can you still reproduce this?

#8 @ocean90
10 years ago

  • Keywords reporter-feedback removed

Yes.

#9 @chriscct7
10 years ago

  • Focuses administration added

Okay, will mark as admin focus. The IRC chat makes it seem its just a matter of moving the plugin update code down. Maybe this could be done in 4.1

#10 @chriscct7
10 years ago

  • Keywords good-first-bug added

@bswatson
10 years ago

#11 @bswatson
10 years ago

Patch added, but I wasn't able to shift around lines of code as it's changed significantly since the IRC chat occurred.

#12 @bswatson
10 years ago

  • Keywords has-patch added; needs-patch removed

#13 @johnbillion
10 years ago

  • Keywords needs-testing added
  • Milestone changed from Future Release to 4.1

#14 @dd32
10 years ago

  • Owner dd32 deleted
  • Status changed from new to assigned

#15 @omarreiss
10 years ago

Tested and it works!

#16 @Yahire Furniture
10 years ago

Yes this is working for me as well

#17 @ocean90
10 years ago

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

[30696] missed the ticket.

#18 @johnbillion
10 years ago

In 31390:

Revert [30696] pending further investigation.

See #31011, #13071

#19 @SergeyBiryukov
10 years ago

  • Keywords needs-patch added; good-first-bug has-patch needs-testing removed
  • Milestone changed from 4.1 to Future Release
  • Resolution fixed deleted
  • Status changed from closed to reopened

#20 @dd32
10 years ago

In 31394:

Revert [30696] pending further investigation.

Props johnbillion.
Merges [31390] to the 4.1 branch.
See #13071. Fixes #31011.

@ocean90
9 years ago

@ocean90
9 years ago

#21 @ocean90
9 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Future Release to 4.6

13071.patch fixes the missing update row. wp_plugin_update_rows() is hooked into admin_init and therefore runs before wp_update_plugins() is called, see https://core.trac.wordpress.org/browser/trunk/src/wp-includes/update.php?rev=37518&marks=689-691,696-698#L684.

This ticket was mentioned in Slack in #feature-shinyupdates by ocean90. View the logs.


9 years ago

@ocean90
9 years ago

#23 @ocean90
9 years ago

13071.2.patch uses the after_plugin_row action instead. wp_plugin_update_rows() and wp_theme_update_rows() unregister the callback when they are called for the first time.

This ticket was mentioned in Slack in #feature-shinyupdates by ocean90. View the logs.


9 years ago

#25 @swissspidy
9 years ago

  • Keywords shiny-updates added

#26 @swissspidy
9 years ago

  • Keywords needs-testing added

13071.2.patch doesn't seem to work for me :-(

Changed the versions of a theme and a plugin and visited update-core.php. The updates are shown in the list tables, but not in the admin menu. After a reload, the bubble in the admin menu is shown.

#27 @ocean90
9 years ago

  • Keywords needs-testing removed

@swissspidy That's right, tt only fixes the tables. I don't think we can fix the admin menu in its current state.

#28 @swissspidy
9 years ago

Ah, gotcha. Just read #31011 again.

Could we at perhaps add some JavaScript that increases the counter in the admin menu when visiting update-core.php for the first time? Or is that too much of an edge case scenario?

#29 @ocean90
9 years ago

13071.2.patch seems to break custom updaters like https://github.com/restrictcontentpro/restrict-content-pro/blob/eb2d5e1f26cb17f54506c401835088a05b00dc91/RCP_Plugin_Updater.php#L60-L61.

Replying to swissspidy:

Could we at perhaps add some JavaScript that increases the counter in the admin menu when visiting update-core.php for the first time? Or is that too much of an edge case scenario?

A script in the footer should work since it's running after all the update checks. That's why it's visible in the toolbar. update-core.php wouldn't be the only place, we need it for themes.php and plugins.php too.

This ticket was mentioned in Slack in #feature-shinyupdates by obenland. View the logs.


9 years ago

#31 @ocean90
9 years ago

In 37978:

Upgrade/Install: Change priority for theme/update update rows.

wp_plugin_update_rows() and wp_theme_update_rows() are using the site transients update_plugins and update_themes which are set by wp_update_plugins() and wp_update_themes(). Both functions are hooked into load-plugins.php and load-themes.php. Therefore the update rows need to be registered after the transients were populated.

See #13071.

#32 @ocean90
9 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from 4.6 to Future Release

Fixing the admin menu is something for another release.

#33 @swissspidy
8 years ago

  • Milestone changed from Future Release to 4.7
  • Owner set to swissspidy
  • Status changed from reopened to assigned

Could we at perhaps add some JavaScript that increases the counter in the admin menu when visiting update-core.php for the first time?

Let's do this.

@swissspidy
8 years ago

#34 @swissspidy
8 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

13071.diff is a first pass / proof-of-concept. I took the opportunity to clean up some JS parts as well. The overall solution seems a bit hacky though.

#35 @swissspidy
8 years ago

@obenland I know you're busy with the plugin directory, but maybe you've got a few minutes to test the patch? You know the code quite well.

Basically it separates the update counter and actually updating the HTML across the site. It makes the code easier to understand. After that, wp_print_update_count_for_js() is where the magic happens. It updates the counter and forces a refresh. That way, the update bubble always shows the correct when visiting plugins.php, themes.php or update-core.php.

Note that I haven't added a docblock yet and perhaps the function name can be improved.

#36 @obenland
8 years ago

I think we should try to re-use _wpUpdatesItemCounts (available as settings.plugins and settings.themes), that would allow us to get rid of the footer script hook. It also feels like refreshCount() does a lot of the same things as deletePluginSuccess() for example, I think there might be potential to optimize that.

@swissspidy
8 years ago

#37 @swissspidy
8 years ago

13071.2.diff tries to reuse _wpUpdatesItemCounts for passing the update counts.

I am not sure about the similarities between refreshCount() and deletePluginSuccess(). The former refreshes the update counts everywhere on the screen, including the tabs on the plugin list table, while the latter only adjusts these tabs — and delegates updating the 'Update Available' tab to refreshCount(). Not much I'd change there right now.

If anything, I'd want to get rid of the duplicate code inside deletePluginSuccess(), but that's something for a new ticket.

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


8 years ago

#39 @ocean90
8 years ago

  • Owner changed from swissspidy to ocean90
  • Status changed from assigned to reviewing

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


8 years ago

@ocean90
8 years ago

#41 @ocean90
8 years ago

  • Owner changed from ocean90 to swissspidy

Thanks @swissspidy. Found a few issues with 13071.2.diff. I fixed them in 13071.3.diff:

  • The counter in .subsubsub didn't update for plugins because settings.totals.counts.themes was used. Changed to itemCount.
  • Yoda conditions should only be used for ==, !=, ===, and !==.
  • Added a missing semicolon and DocBlock.
  • _wpUpdatesItemCounts wasn't defined on wp-admin/themes.php.
  • _wpUpdatesItemCounts wasn't always defined on wp-admin/update-core.php. Bulk theme/plugin upgrades have their own output.
  • The update script is printed in the header since [27280]. I couldn't find a reason for that so I changed the group. postMessage is still working for me. Needs some testing.
  • Changing the group allows us to use wp_localize_script() in all places.

#42 @swissspidy
8 years ago

  • Keywords commit added; needs-testing removed

The latest patch works great.

#43 @swissspidy
8 years ago

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

In 38827:

Upgrade/Install: Refresh update counts after page load.

By enqueuing the updates script in the footer and passing the number of available updates to it after page load, the update bubbles will be more accurate.

Props ocean90, swissspidy.
Fixes #13071.

Note: See TracTickets for help on using tickets.