Opened 8 years ago
Last modified 4 years ago
#39487 reviewing defect (bug)
Default to 'transport'=>'postMessage' for a setting associated with a selective refresh partial
Reported by: | danielbachhuber | Owned by: | noisysocks |
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | 4.5 |
Component: | Customize | Keywords: | has-patch has-unit-tests |
Focuses: | Cc: |
Description
As a WordPress developer, it can be confusing when registering a selective refresh partial to a setting doesn't work because I've omitted 'transport'=>'postMessage'
for the setting.
When registering a selective refresh partial to a setting, I'd expect 'transport'=>'postMessage'
to be set dynamically for the setting.
Attachments (2)
Change History (8)
#1
@
8 years ago
- Keywords needs-patch added; dev-feedback removed
- Milestone changed from Awaiting Review to Future Release
#2
@
5 years ago
- Keywords has-patch needs-testing needs-unit-tests added; needs-patch removed
- Milestone changed from Future Release to 5.4
- Version set to 4.5
I've been tripped up by not using postMessage
with my selectively refreshed settings many times. It'd be great if core could provide a safety net here.
39487.diff attempts to implements the logic described in comment:1.
The logic is added to the \WP_Customize_Selective_Refresh
class in part because it's related to selective refresh and part to avoid adding more methods to \WP_Customize_Manager
if they don't strictly have to be. The added check_transport_of_settings_with_partials()
method is called via hooks to allow developers to disable it consistently in the admin and the preview.
As I was working on this patch, I learned that there's precedent for a theme to register a partial for a setting but intentionally leave the setting transport as refresh
unless some condition is met. Core itself uses this pattern with the header_image
setting, which is changed to postMessage
in \WP_Customize_Manager::register_controls()
depending the on theme support for custom-header
.
The patch would also change the transport for these settings, perhaps unexpectedly. Luckily, if the partial keeps $fallback_refresh
enabled, then a full refresh of the preview will continue to occur if the partial refresh fails. Accordingly, the patch doesn't modify settings associated with partials that disable $fallback_refresh
.
I'll work on unit tests, but I'd love for anyone to take the patch for a spin to try to get something into the next release!
#4
@
5 years ago
- Milestone changed from 5.4 to Future Release
Hi,
With 5.4 Beta 3 approaching and the Beta period reserved for bugs introduced during the cycle, this is being moved to Future Release. If any maintainer or committer feels this should be included or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.
This ticket was mentioned in Slack in #core-customize by dlh. View the logs.
4 years ago
#6
@
4 years ago
- Keywords needs-testing removed
- Owner set to noisysocks
- Status changed from new to reviewing
I tested 39487.2.diff locally and it works great! 👍
There are two lint errors in class-wp-customize-selective-refresh.php
. We need to use array()
instead of []
.
Not a blocking comment at all, but I'm a little worried that changing $setting->transport
in customize_preview_init
creates a weird situation where the WP_Customize_Setting
objects that a plugin or theme sets up in customize_register
isn't representative of what ends up being "executed".
For example, the var_dump()
here still outputs "refresh"
:
function foo_theme_customize_register( WP_Customize_Manager $wp_customize ) { $wp_customize->add_setting( 'mydate', array( 'type' => 'theme_mod', 'transport' => 'refresh', ) ); $wp_customize->add_control( 'mydate', array( 'type' => 'date', 'section' => 'colors', 'label' => __( 'Date' ), 'description' => __( 'This is a date control with a red border.' ), 'input_attrs' => array( 'placeholder' => __( 'mm/dd/yyyy' ), ), ) ); $wp_customize->selective_refresh->add_partial( 'mydate', array( 'selector' => '.my-date', 'container_inclusive' => false, 'render_callback' => function() { echo 'Date: ' . get_theme_mod( 'mydate' ); }, ) ); var_dump( $wp_customize->get_setting( 'mydate' )->transport ); } add_action( 'customize_register', 'foo_theme_customize_register' );
What do you think about overwriting $setting->transport
in WP_Customize_Selective_Refresh::add_partial()
instead? This way developers might immediately see that their setting is changed with normal debugging techniques.
This makes sense to me.
One possible implementation would be to add a
foreach
loop in\WP_Customize_Manager::prepare_controls()
which can loop over all of the registered partials and to get their associated settings, and if an setting hasrefresh
as its transport, then it could be overridden topostMessage
.