WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 weeks ago

#13071 closed defect (bug) (fixed)

Update bubble appears only at the second load

Reported by: ocean90 Owned by: 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 2 years ago.
13071.png (414.1 KB) - added by ocean90 7 months ago.
13071.patch (772 bytes) - added by ocean90 7 months ago.
13071.2.patch (1.7 KB) - added by ocean90 7 months ago.
13071.diff (9.1 KB) - added by swissspidy 3 months ago.
13071.2.diff (10.9 KB) - added by swissspidy 2 months ago.
13071.3.diff (13.4 KB) - added by ocean90 8 weeks ago.

Download all attachments as: .zip

Change History (50)

#1 @ocean90
7 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
7 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
6 years ago

  • Milestone changed from Awaiting Triage to Future Release

#4 @Viper007Bond
6 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
6 years ago

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

#6 @dd32
6 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
2 years ago

  • Keywords reporter-feedback added

Can you still reproduce this?

#8 @ocean90
2 years ago

  • Keywords reporter-feedback removed

Yes.

#9 @chriscct7
2 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
2 years ago

  • Keywords good-first-bug added

@bswatson
2 years ago

#11 @bswatson
2 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
2 years ago

  • Keywords has-patch added; needs-patch removed

#13 @johnbillion
2 years ago

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

#14 @dd32
2 years ago

  • Owner dd32 deleted
  • Status changed from new to assigned

#15 @omarreiss
2 years ago

Tested and it works!

#16 @Yahire Furniture
2 years ago

Yes this is working for me as well

#17 @ocean90
2 years ago

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

[30696] missed the ticket.

#18 @johnbillion
22 months ago

In 31390:

Revert [30696] pending further investigation.

See #31011, #13071

#19 @SergeyBiryukov
22 months 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
22 months ago

In 31394:

Revert [30696] pending further investigation.

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

@ocean90
7 months ago

@ocean90
7 months ago

#21 @ocean90
7 months 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.


7 months ago

@ocean90
7 months ago

#23 @ocean90
7 months 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.


7 months ago

#25 @swissspidy
6 months ago

  • Keywords shiny-updates added

#26 @swissspidy
6 months 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
6 months 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
6 months 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
6 months 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.


6 months ago

#31 @ocean90
5 months 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
5 months 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
3 months 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
3 months ago

#34 @swissspidy
3 months 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
2 months 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
2 months 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
2 months ago

#37 @swissspidy
2 months 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.


2 months ago

#39 @ocean90
2 months 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.


2 months ago

@ocean90
8 weeks ago

#41 @ocean90
8 weeks 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 weeks ago

  • Keywords commit added; needs-testing removed

The latest patch works great.

#43 @swissspidy
7 weeks 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.