WordPress.org

Make WordPress Core

Opened 3 weeks ago

Closed 3 weeks ago

Last modified 12 hours ago

#42365 closed defect (bug) (fixed)

Error while theme update from customizer

Reported by: Mahesh901122 Owned by: obenland
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.9
Component: Customize Keywords: has-screenshots has-patch
Focuses: ui Cc:

Description

I have tested on https://wordpress.org/wordpress-4.9-beta4.zip

I'm updating theme from customizer. Theme updated successfully. But, Could not show the spinner & trigger the JS error: Uncaught TypeError: Cannot read property 'counts' of undefined in file updates.js?ver=4.9-beta4:345


How to reproduce?

  • I have updated "Twenty Sixteen" theme version with 1.2.
  • Goto Customizer -> Themes -> Installed Themes and update the "Twenty Sixteen".

Check this video - http://bsf.io/zu64m


I'm tried to apply the patch but not found the root of this error.

After some digging I found that the #update-theme not found while theme update.

// Update the theme details UI.
$notice = $( '#update-theme' ).closest( '.notice' ).removeClass( 'notice-large' );

The #update-theme is added though JS localize though function get_theme_update_available(). Maybe the localize variables are not found here.

Screenshot - http://bsf.io/x10cu

Attachments (8)

js-error-theme-update-customize.png (37.9 KB) - added by rinkuyadav999 3 weeks ago.
42365-v1.patch (505 bytes) - added by rinkuyadav999 3 weeks ago.
Works with direct Selector
42365-v2.patch (606 bytes) - added by rinkuyadav999 3 weeks ago.
Fix for update button over theme icon - grid
42365.diff (1.3 KB) - added by celloexpressions 3 weeks ago.
Don't try to update counts that don't exist.
42365.2.diff (3.0 KB) - added by obenland 3 weeks ago.
42365.3.diff (3.6 KB) - added by obenland 3 weeks ago.
42365.4.diff (3.6 KB) - added by obenland 3 weeks ago.
Screen Shot 2017-11-19 at 5.31.22 PM.png (901.7 KB) - added by johnjamesjacoby 12 hours ago.

Download all attachments as: .zip

Change History (30)

#1 @rinkuyadav999
3 weeks ago

  • Keywords needs-patch has-screenshots dev-feedback added

Yes, i try to update theme. Update link appear till theme update instead spinner. And js error:

#2 @rinkuyadav999
3 weeks ago

@westonruter

/wp-admin/updates.js L953 $notice

colsole.log( $notice );

return > Object { selector: "", length: 0, prevObject: Object, context: HTMLDocument → customize.php }

I think it is not pointing to correct selector. that's why it does not add class 'updating-message' and text 'wp.updates.l10n.updating' inside p.

#3 @westonruter
3 weeks ago

  • Milestone changed from Awaiting Review to 4.9
  • Owner set to celloexpressions
  • Status changed from new to assigned

@celloexpressions: Could you review and work out a patch today?

@rinkuyadav999
3 weeks ago

Works with direct Selector

#4 @westonruter
3 weeks ago

  • Keywords has-patch added; needs-patch removed
  • Status changed from assigned to reviewing

@rinkuyadav999
3 weeks ago

Fix for update button over theme icon - grid

#5 @rinkuyadav999
3 weeks ago

Still Two issue:

  1. JS error after update - TypeError: settings.totals is undefined
  2. 'Update Theme' message still appear in Model / when click on 'Theme Details'

@celloexpressions
3 weeks ago

Don't try to update counts that don't exist.

#6 @celloexpressions
3 weeks ago

  • Keywords dev-feedback removed
  • Owner changed from celloexpressions to westonruter

We need to prevent updates.js from trying to change global update counts in the customizer. See 42365.diff. This also fixes some issues that had been caused by the JS error, and includes the related selector fix from 42365-v2.patch.

#7 @westonruter
3 weeks ago

@rinkuyadav999 Would you please give 42365.diff a test?

#8 @rinkuyadav999
3 weeks ago

When i apply this patch, it display error SyntaxError: missing ; before statement[Learn More] updates.js:358:4 due to removal of if ( 'plugin' === type ) { L347 . see screenshot. But that may be issue with my TortoiseSVN.

https://preview.ibb.co/kMReY6/tsvn.png

I added that L347 manually in patch.

When i click on update button, it start updating theme with updating icon and message. Just after, when i open model, it still display Update available message and then updated message two time and then it hide on model close and then open. Same icon and message does not display over theme image and model like /wp-admin/themes.php

When we update theme on /wp-admin/themes.php, it reflect icon and message over theme image and model (both side).

#9 @westonruter
3 weeks ago

@rinkuyadav999 Did you reset your working directory before trying to apply the patch? There is no removal of anything in 42365.diff.

#10 @rinkuyadav999
3 weeks ago

@westonruter Yes 42365.diff fix JS error and update theme when we click on update button over theme screenshot BUT same reaction does not happens inside model.

When we click on update button, it start updating theme, just then if you will open that theme model, it will show update available message. it changes but after theme updated. and display updated message two times. and then when we close and open same theme model again, update section disappear.

Issue: Same reaction does not happens inside theme model.

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


3 weeks ago

#12 @westonruter
3 weeks ago

  • Owner changed from westonruter to obenland

@obenland
3 weeks ago

@obenland
3 weeks ago

@obenland
3 weeks ago

#13 follow-up: @westonruter
3 weeks ago

@rinkuyadav999 @celloexpressions Would you test the latest patch?

#14 @obenland
3 weeks ago

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

In 42046:

Customize: Spiff up theme updates

Fixes UI bugs around theme updates in the Customizer. Theme versions now get updated and users are no longer left alone after a successful update.

Props rinkuyadav999, celloexpressions for initial patch.
Fixes #42365.

#15 in reply to: ↑ 13 @rinkuyadav999
3 weeks ago

Replying to westonruter:

@rinkuyadav999 @celloexpressions Would you test the latest patch?

Issue: When we click on update button over theme screenshot, theme model still displays update available and link to update theme till theme get updated.

#16 @westonruter
3 weeks ago

Per @celloexpressions in Slack:

It sounds like the issue noted above is that you may get unexpected behavior if you open the details modal while a theme is updating. That would be expected currently, as we don't specifically address that situation. I'd probably consider that a borderline enhancement, and not critical. If you update from either the grid or the modal, everything works, and it's all updated the next time you switch between those views.

Otherwise, the improvements that @obenland made look good - I don't think anything else should be needed for 4.9. It's mostly worked in my testing previously, but I've always felt like updates are a convenience more than anything else within the customizer context (probably because I personally tend to run all updates from the unified update screen). And I'm not familiar with all of the interaction objective of updates.js in other contexts, so it can be hard to tell what's missing.

#17 @rinkuyadav999
3 weeks ago

@westonruter @celloexpressions

Yes, this is not actually an error because theme updates perfectly, it is about expecting behavior of 'Update Now' button within customize.

When we update theme on /wp-admin/themes.php, then theme 'Updating.....' message displays at both side (within grid and model).

When we update theme using customize, then message inside grid show 'Updating.....' BUT model does not show 'Updating.....'. i think model should also display 'Updating.....' message instead 'Update Available'.

#18 @rinkuyadav999
3 weeks ago

According to code, Model should display 'Updating....' message. but class notice-large does not remove, h3 does not remove, wp.updates.l10n.updating does not display inside p.

.
.
.
.....
} else if ( 'customize' === pagenow ) {

        // Update the theme details UI.
        $notice = $( '[data-slug="' + args.slug + '"].notice' ).removeClass( 'notice-large' );

        $notice.find( 'h3' ).remove();

        // Add the top-level UI, and update both.
        $notice = $notice.add( $( '#customize-control-installed_theme_' + args.slug ).find( '.update-message' ) );
        $notice = $notice.addClass( 'updating-message' ).find( 'p' );

} else {
....
.
.
.
.
$notice.text( wp.updates.l10n.updating );

Code are mostly same for /wp-admin/themes.php and works.

#19 @obenland
3 weeks ago

Yeah, it seems to work differently in the Customizer than on the themes screen. In the Customizer the modal doesn't seem to be manipulable until its shown, causing that difference in state.

#20 follow-up: @westonruter
3 weeks ago

@obenland The showDetails method writes the theme details to an overlay with a template whenever it is shown: https://github.com/WordPress/wordpress-develop/blob/a39d599a/src/wp-admin/js/customize-controls.js#L2542-L2564

I suppose section.overlay.html( section.template( theme ) ) here could be amended so that the theme includes any updating state?

#21 in reply to: ↑ 20 @obenland
3 weeks ago

Replying to westonruter:

I suppose section.overlay.html( section.template( theme ) ) here could be amended so that the theme includes any updating state?

If we tracked that state, yes. Updates.js doesn't do that though because there are no models in the vast majority of contexts it's used in

#22 @johnjamesjacoby
12 hours ago

This is still broken for me, at least on my local dev environment.

  • 4.9 stable
  • Multisite enabled
  • Logged in as super admin
  • Same user flow as originally reported
  • Clicking "Update Now" from the Customizer theme-view spits out that JS error

There's an argument to be made that updating a theme from within this view shouldn't even be possible.

Users are in there to pick a new theme, not to do software updates. I kinda thought it was a bug that the option was even available here, and only clicked it to see if it would work, or if the customizer JS would block it. At first I thought it _was_ blocked, but console proved otherwise.

Last edited 12 hours ago by johnjamesjacoby (previous) (diff)
Note: See TracTickets for help on using tickets.