#35926 closed defect (bug) (fixed)
Allow controls to be registered without any associated settings
Reported by: | westonruter | Owned by: | westonruter |
---|---|---|---|
Milestone: | 4.5 | Priority: | normal |
Severity: | normal | Version: | 3.4 |
Component: | Customize | Keywords: | has-patch has-unit-tests commit close |
Focuses: | Cc: |
Description
Sometimes controls need to be added which don't directly have associated settings. For example, there can be controls needed which manipulate some aspect of the Customizer interface or manage the behavior of other controls and their settings (e.g. a control that contains a button to create a new setting). For example, nav menus in the Customizer have two settings create_new_menu
and new_menu_name
both of which have custom setting types to basically make them no-op settings that don't get persisted into the DB. The settings are being used here as hacks to get the controls to register properly. I know this has been done in other places as well as the current “state of the art”, like http://wordpress.stackexchange.com/a/212432/8521
With the introduction of selective refresh (#27355), partials could technically be registered (though at the moment not via the constructor) that do not have any associated settings (and in their case, the isRelatedSetting()
method in JS can be used to dynamically changed settings with a partial). Controls should also be able to be registered without related settings.
Changes will be needed to the PHP WP_Customize_Control
and to the corresponding JS wp.customize.Control
to allow it to initialize properly with an empty settings array.
Note that the WP_Customize_Control
should then also have an intrinsic capability
property since it wouldn't inherently derive its capability from its settings (if it has none). (And the same should be true for WP_Customize_Partial
.)
Attachments (3)
Change History (18)
#2
@
9 years ago
I like this, although it's probably a bit late for 4.5. I think there are other places that core does it, but I may be thinking of outstanding patches. Also has potentially significant implications for themes and plugins so we should get the word out early if it happens (not that it breaks anything necessarily).
#3
@
9 years ago
@celloexpressions what implications? I can't think of any. Existing controls that use dummy settings will continue to work normally. Controls previously that lacked settings would error, so I don't think there would be any conflict with anything currently in existence.
This ticket was mentioned in Slack in #core by jorbin. View the logs.
9 years ago
#5
@
9 years ago
- Type changed from enhancement to defect (bug)
Chatted a bit with @westonruter and we figured out it is more of a bug that controls can't be registered without settings, so adjusting accordingly.
#6
follow-up:
↓ 7
@
9 years ago
This is definitely not a bug, it's a design decision. I think it's worth exploring changing the paradigm, but we should investigate why it was written this way originally before making a final decision. I definitely like the idea of it, for what it's worth. And it would be nice no to have the error, but on the other hand it could potentially result in improper usage/difficulty debugging missing a setting, which may have been the reason for it in the first place.
#7
in reply to:
↑ 6
@
9 years ago
- Owner set to westonruter
- Status changed from new to accepted
@celloexpressions if you register a control with empty/invalid settings at the moment, it throws a fatal error: “Call to a member function check_capabilities() on a non-object.” Anyway, it's not changing the paradigm, it's just ensuring that you can create a control without having to specify settings (expanding the paradigm). I expect this was not accounted for in the original architecture of the Customizer because controls were originally intended to always have related settings.
#8
@
9 years ago
- Keywords has-patch has-unit-tests commit added; needs-patch needs-unit-tests removed
Here are the changes in the patch which I plan to commit in the morning:
Customize: Allow controls to be registered without any associated settings.
- Improves parity between partials and controls. A partial or control can be settingless if instantiated with
settings
param as empty array (otherwise, if null, then the partial/control ID is used). - Eliminate need to create dummy settings that serve no purpose except to place a control in the UI.
- Removes dummy settings for
create_new_menu
andnew_menu_name
. - Introduces
WP_Customize_Control::$capability
andWP_Customize_Partial::$capability
, and if set checks them in the respectivecheck_capabilities()
methods. - Prevents PHP fatal error from happening when non-existing settings are provided to control: "Call to a member function
check_capabilities()
on a non-object". - Fixes issue where nav menu items and widgets were no longer working with selective refresh because cap check was failing.
Fixes #35926.
#11
@
9 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
After [36689], when creating a control in JS, the control's setting
property can't be assigned a setting instance directly in the options object.
That said, I'm not sure whether passing a setting
was "doing it wrong" to begin with. In my case, I was doing so only as a convenience in a unit test.
For example:
var setting = wp.customize.create( 'foo', 'foo', 'bar', { transport: 'refresh', previewer: wp.customize.previewer, }); var control = wp.customize.control.create( 'baz', 'baz', { setting: setting, });
console.log( control.setting )
shows the setting instance before [36689] and null
currently.
#12
@
9 years ago
- Keywords close added
@dlh interesting. The JS constructor is definitely confusing. I checked out the state of Core before [36689] and I could replicate what you did. I think however your code wasn't quite right to begin with, however. If you look at:
var control = wp.customize.control.create( 'baz', 'baz', { setting: setting, });
If you look at control.deferred.embedded.state()
you'll see that it is "pending". Why? Because the params.settings
were empty and so the deferred callback never fired, and so control.embed()
was never called. This means your control's ready()
callback was also never called, unless you were calling it manually. (Also, this embedded
deferred won't resolve since no section
param is provided.)
The proper way to instantiate the control in the way that core does it when the Customizer boots up is like this (to adapt your example above):
var control = wp.customize.control.create( 'baz', 'baz', { params: { settings: { 'default': setting.id }, } });
Take these two other examples for how controls are instantiated dynamically in Core:
https://core.trac.wordpress.org/browser/tags/4.4.2/src/wp-admin/js/customize-nav-menus.js#L2276
https://core.trac.wordpress.org/browser/tags/4.4.2/src/wp-admin/js/customize-widgets.js#L1991
It is not intuitive why there is an additional params
property here but that's just how these JS constructors take their arguments.
The reason why control.setting
made its way through before was because of this line in the Control
's initialize
method:
$.extend( control, options || {} );
But this is not the way the control expects the setting to be passed in, as it needs to be part of the params
object. With the most recent changes in [36689], the setting
will get set explicitly to null
if no settings
are provided.
So I think the resolution is to close this since the usage you were using, although intuitive, wasn't actually the way that Core constructs the controls.
#13
@
9 years ago
- Resolution set to fixed
- Status changed from reopened to closed
@dlh please re-open if you have a concern with my explanation.
WIP