WordPress.org

Make WordPress Core

Opened 9 months ago

Closed 9 months ago

Last modified 8 months 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 9 months ago.
42365-v1.patch (505 bytes) - added by rinkuyadav999 9 months ago.
Works with direct Selector
42365-v2.patch (606 bytes) - added by rinkuyadav999 9 months ago.
Fix for update button over theme icon - grid
42365.diff (1.3 KB) - added by celloexpressions 9 months ago.
Don't try to update counts that don't exist.
42365.2.diff (3.0 KB) - added by obenland 9 months ago.
42365.3.diff (3.6 KB) - added by obenland 9 months ago.
42365.4.diff (3.6 KB) - added by obenland 9 months ago.
Screen Shot 2017-11-19 at 5.31.22 PM.png (901.7 KB) - added by johnjamesjacoby 8 months ago.

Download all attachments as: .zip

Change History (31)

#1 @rinkuyadav999
9 months 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
9 months 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
9 months 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
9 months ago

Works with direct Selector

#4 @westonruter
9 months ago

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

@rinkuyadav999
9 months ago

Fix for update button over theme icon - grid

#5 @rinkuyadav999
9 months 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
9 months ago

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

#6 @celloexpressions
9 months 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
9 months ago

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

#8 @rinkuyadav999
9 months 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
9 months 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
9 months 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.


9 months ago

#12 @westonruter
9 months ago

  • Owner changed from westonruter to obenland

@obenland
9 months ago

@obenland
9 months ago

@obenland
9 months ago

#13 follow-up: @westonruter
9 months ago

@rinkuyadav999 @celloexpressions Would you test the latest patch?

#14 @obenland
9 months 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
9 months 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
9 months 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
9 months 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
9 months 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
9 months 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
9 months 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
9 months 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
8 months 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 8 months ago by johnjamesjacoby (previous) (diff)

This ticket was mentioned in Slack in #core-customize by melchoyce. View the logs.


8 months ago

Note: See TracTickets for help on using tickets.