Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 7 years ago

#35926 closed defect (bug) (fixed)

Allow controls to be registered without any associated settings

Reported by: westonruter's profile westonruter Owned by: westonruter's profile 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)

35926.0.diff (3.6 KB) - added by westonruter 9 years ago.
WIP
35926.1.diff (7.5 KB) - added by westonruter 9 years ago.
35926.2.diff (12.6 KB) - added by westonruter 9 years ago.

Download all attachments as: .zip

Change History (18)

@westonruter
9 years ago

WIP

#1 @westonruter
9 years ago

  • Keywords needs-unit-tests added

#2 @celloexpressions
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 @westonruter
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 @jorbin
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.

@westonruter
9 years ago

#6 follow-up: @celloexpressions
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.

@westonruter
9 years ago

#7 in reply to: ↑ 6 @westonruter
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 @westonruter
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 and new_menu_name.
  • Introduces WP_Customize_Control::$capability and WP_Customize_Partial::$capability, and if set checks them in the respective check_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.

Version 0, edited 9 years ago by westonruter (next)

#9 @westonruter
9 years ago

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

In 36689:

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 and new_menu_name.
  • Introduces WP_Customize_Control::$capability and WP_Customize_Partial::$capability, and if set checks them in the respective check_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.

See #27355.
Fixes #35926.

#10 @westonruter
9 years ago

In 36776:

Customize: Fix PHP notice when calling WP_Customize_Control::json() inside content_template() method.

A temp control is instantiated when WP_Customize_Manager:: render_control_templates() is called. This control needs to explicitly specify an empty settings array to avoid trying to use a temp setting which won't exist.

See #35926.
See #29572.

#11 @dlh
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 @westonruter
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 @westonruter
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.

#14 @dlh
9 years ago

@westonruter No concerns. Thanks very much for investigating and responding.

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


7 years ago

Note: See TracTickets for help on using tickets.