WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 3 weeks ago

Last modified 3 weeks ago

#40764 closed defect (bug) (fixed)

Multisite theme update not showing new version number

Reported by: afragen Owned by: swissspidy
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.6
Component: Upgrade/Install Keywords: has-patch commit
Focuses: multisite Cc:

Description

When updating plugins via the ajax updater the new version number displays correctly in the plugin row when the update is completed.

In multisite, when a theme is updated via the ajax updater the new version number is not displayed in the theme row.

Attachments (5)

40764.diff (766 bytes) - added by afragen 5 months ago.
40764-2.diff (759 bytes) - added by afragen 5 months ago.
40764.2.diff (752 bytes) - added by swissspidy 3 months ago.
40764.3.diff (975 bytes) - added by afragen 2 months ago.
40764.4.diff (983 bytes) - added by afragen 2 months ago.

Download all attachments as: .zip

Change History (30)

@afragen
5 months ago

#1 @afragen
5 months ago

  • Focuses multisite added
  • Keywords has-patch added

#2 @afragen
5 months ago

  • Keywords dev-feedback added

@afragen
5 months ago

#3 @afragen
5 months ago

Adjusted the conditional check. I think it makes more sense.

#4 @ocean90
5 months ago

  • Keywords dev-feedback removed
  • Milestone changed from Awaiting Review to Future Release
  • Owner set to swissspidy
  • Status changed from new to reviewing
  • Version changed from trunk to 4.6

This ticket was mentioned in Slack in #core-multisite by afragen. View the logs.


4 months ago

#6 @afragen
4 months ago

  • Keywords dev-feedback added

#7 @afragen
4 months ago

  • Keywords needs-testing added; dev-feedback removed

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


4 months ago

@swissspidy
3 months ago

#9 @swissspidy
3 months ago

  • Keywords needs-testing removed
  • Milestone changed from Future Release to 4.9

A few notes on the first patches:

  • The old version needs to be fetched before any update happens, otherwise it's just the new version
  • The upgrader object holds the theme info, not the upgrader skin ($upgrader->theme_info() method)

40764.2.diff addresses this.

#10 follow-up: @afragen
3 months ago

$upgrader->skin->theme_info already exists and it really does contain the current theme_info.

If you want to explicitly get the current theme, that works too and might be easier to understand in code. I was simply trying to work with the data that currently existed.

Thanks for the feedback. I appreciate it.

#11 @afragen
3 months ago

If we are going to explicitly obtain the current WP_Theme object, for consistency, shouldn’t we use the same conditional check as in https://core.trac.wordpress.org/attachment/ticket/40764/40764.diff

if ( $theme->get( ‘Version’ ) ) {

Sorry not really good at WikiFormat.

#12 in reply to: ↑ 10 @swissspidy
3 months ago

Replying to afragen:

$upgrader->skin->theme_info already exists and it really does contain the current theme_info.

If you want to explicitly get the current theme, that works too and might be easier to understand in code. I was simply trying to work with the data that currently existed.

Thanks for the feedback. I appreciate it.

You're right. I see now that there's a $this->skin->theme_info = $this->theme_info($theme); line in Theme_Upgrader::bulk_upgrade(). I got suspicious because theme_info isn't an explicit property of the skin. Plus, that property is set within a loop and could theoretically get overridden by accident in a future change. Thus, it's not really straightforward. It would be safer to use Theme_Upgrader::theme_info() directly within the Ajax callback. However, this is a) just a wrapper for wp_get_theme() and b) it was called too late anyway and never returned the old version.

Regarding the conditional check in 40764.diff: that doesn't work reliably. $upgrader->skin->theme_info can be false. Not checking for that can lead to warnings.

! empty( $upgrader->skin->theme_info ) is better, but still not an explicit check for false or WP_Theme. Also, wp_get_theme() recommends checking the exists() method, which is what I did in 40764.2.diff

#13 @afragen
3 months ago

Sounds great thanks for the explanation. I also didn't know if the exists() method. I learned something, thanks.

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


2 months ago

This ticket was mentioned in Slack in #core-multisite by afragen. View the logs.


2 months ago

This ticket was mentioned in Slack in #core-multisite by pmbaldha. View the logs.


2 months ago

#17 @afragen
2 months ago

Based on core-multisite Slack chat https://wordpress.slack.com/archives/C03BVB47S/p1503336138000172 a patch refresh is attached.

I also changed the conditional for newVersion to match so both are similar.

$theme = wp_get_theme( $stylesheet );
    if ( $theme->exists() ) {
...

@afragen
2 months ago

This ticket was mentioned in Slack in #core-multisite by afragen. View the logs.


2 months ago

#19 follow-up: @pmbaldha
2 months ago

  • Keywords needs-testing added

I have tested 40764.2.diff and it is working fine. I have tested without patch, Network theme update is showing version number. After apply the 40764.2.diff patch, it works fine. There was some coding strandard related issue in 40764.2.diff, I have given it to @afragen.

However, I am not able to apply patch 40764.3.diff in my new checkout.

@afragen
2 months ago

#20 in reply to: ↑ 19 @afragen
2 months ago

Replying to pmbaldha:

I have tested 40764.2.diff and it is working fine. I have tested without patch, Network theme update is showing version number. After apply the 40764.2.diff patch, it works fine. There was some coding strandard related issue in 40764.2.diff, I have given it to @afragen.

However, I am not able to apply patch 40764.3.diff in my new checkout.

I think the issue was my local setup. See if the latest patch doesn't install cleanly.

This ticket was mentioned in Slack in #core-multisite by afragen. View the logs.


6 weeks ago

#22 @desrosj
5 weeks ago

  • Keywords commit added; needs-testing removed

Tested this on a subdomain and subdirectory install. Both fixed the issue for me. I think this is good to go.

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


4 weeks ago

#24 @swissspidy
3 weeks ago

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

In 41611:

Upgrade/Install: Show new version number when updating a theme on Multisite.

Props afragen.
Fixes #40764.

#25 @swissspidy
3 weeks ago

In 41619:

Upgrade/Install: Update unit tests after [41611].

See #40764.

Note: See TracTickets for help on using tickets.