WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#38370 closed defect (bug) (fixed)

Customizer: Theme install and preview is still available on a single site in a MS setup and throws JS error when attempting to install

Reported by: iamfriendly Owned by: westonruter
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.7
Component: Customize Keywords: has-patch commit
Focuses: javascript, multisite Cc:

Description

Trunk: 4.7-alpha-38178-src

Multisite setup, subdirectories.
nginx version: nginx/1.10.1
php: PHP 7.0.10

When attempting to install and preview a theme on a single site on a multisite network, a JS error appears in the console

customize-controls.js?ver=4.7-alpha-38178-src:2132 Uncaught TypeError: Cannot read property 'maybeRequestFilesystemCredentials' of undefined

There are 2 issues I think. 1) the obvious JS error and 2) on a multisite a theme needs to be installed and activated for a site at a network level, not from on a site itself.

Perhaps in this scenario it's wise to disable this functionality from within multisite (or maybe have an option for network administrators to allow it, but I'd highly recommend defaulting this to 'off' if this route is chosen)

Attachments (2)

38370.diff (2.7 KB) - added by celloexpressions 5 years ago.
Don't add available theme sections on multisite, as installation is not allowed in the customizer here.
38370.1.diff (3.8 KB) - added by celloexpressions 5 years ago.
Fix conditional placement.

Download all attachments as: .zip

Change History (15)

#1 @westonruter
5 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.7
  • Owner set to celloexpressions
  • Status changed from new to assigned

@celloexpressions
5 years ago

Don't add available theme sections on multisite, as installation is not allowed in the customizer here.

#2 @celloexpressions
5 years ago

  • Keywords has-patch added; needs-patch removed

Please test 38370.diff on a multisite install and see if that fixes it. Theme installation is not available in the customizer on multisite or for users who can't install themes.

#3 @iamfriendly
5 years ago

I don't think I'm fully clear on what the expected behaviour is here.

38370.diff removes the search, filter, popular, and latest entires from the sidebar in the customizer on multisite. However, the 'Feature Filter' and 'Favourites' still appear. So if I filter any then I still see a list of themes from .org to 'Install and Preview' - and I _only_ have twentyfifteen install on this multisite. That doesn't seem right. (Should it be filtering from the themes that I have installed and active for the current site?)

Additionally, the 'Favourites' doesn't seem to work (I have favourited twentysixteen on .org and it doesn't appear when I type in my .org username). I receive a "No themes found. Try a different search." message.

Finally with this patch, when clicking the 'Change' button next to 'Active Theme' from the main customizer page, I now get customize-controls.js?ver=4.7-alpha-38178-src:2112 Uncaught TypeError: Cannot read property 'expand' of undefined(…) in the console and I'm unable to click the 'back' arrow next to the 'You are Browsing: Themes' control title.

#4 @iamfriendly
5 years ago

What I should have said in the above comment was that I'm happy to run with this and create a patch. However, right now, I'm genuinely unsure what is expected to happen on multisite. If it's simply "none of this stuff should appear in MS" then that's cool by me (it'd be a shame, but if that's the way it needs to be in this release, then we can iterate on the MS behaviour in 4.8 onwards), but if there is already underlying code which should enable users to filter by activated themes for the current site on a MS install then if you could point me in that direction I'd appreciate it.

#5 @iamfriendly
5 years ago

  • Keywords needs-patch added; has-patch removed

#6 @ocean90
5 years ago

#38455 was marked as a duplicate.

#7 @Offereins
5 years ago

Tested the patch. I don't quite get why the Feature Filter and Favorites sections are still included. As the install_themes capability argument shows, those too need to be hidden for multisite contexts.

Last edited 5 years ago by Offereins (previous) (diff)

@celloexpressions
5 years ago

Fix conditional placement.

#8 @celloexpressions
5 years ago

  • Keywords has-patch added; needs-patch removed

Oops, thanks for catching that. I put the closing brace in the wrong place, this patch was made very quickly. See 38370.1.diff.

Multisite should currently see a search filter for installed themes but there's room for improvement. I suppose it would be possible to replicate the feature filter functionality, at least, here, since installed themes have the tags data that we already filter for the installed search. That would probably be a 4.8 thing, though, unless anyone wants to tackle what that'd look like this week. It would be a matter of adapting the existing feature filter UI to reuse the installed themes section and filter based on the control.params.theme.tags instead of doing a .org query, similarly to how the installed themes filter/search works.

#9 @iamfriendly
5 years ago

OK yup, 38370.1.diff seems to do what you want :)

One thing I notice though (and this may be another ticket) but if I go Customize - > Active Theme -> Change -> Hover over a non-active theme -> Theme Details -> Live Preview (from that modal) then the active theme does not change.

I doubt this is introduced by this, though?

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


5 years ago

#11 @celloexpressions
5 years ago

  • Keywords commit added
  • Owner changed from celloexpressions to westonruter
  • Status changed from assigned to reviewing

Let's get this in before beta 1, it's a pretty quick fix.

#12 @westonruter
5 years ago

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

In 38887:

Customize: Disable theme installation in multisite.

Props celloexpressions, iamfriendly.
Fixes #38370.

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


5 years ago

Note: See TracTickets for help on using tickets.