Opened 8 years ago
Closed 8 years ago
#41488 closed defect (bug) (fixed)
Customize: uncaught JS error with namespaced nav menu fallbacks
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 4.9 | Priority: | normal |
Severity: | normal | Version: | 4.3 |
Component: | Customize | Keywords: | has-patch commit |
Focuses: | Cc: |
Description
A wp_nav_menu()
call with a namespaced fallback_cb
can generate a JavaScript error if the backslash creates invalid JSON. For example:
wp_nav_menu( [
'fallback_cb' => '\WordPress\my_callback,
'theme_location' => 'menu-1',
'menu_id' => 'primary-menu',
] );
throws an error at https://github.com/WordPress/WordPress/blob/82d0a53e77f0eb6a8de60b67a2b5266c4f042741/wp-includes/js/customize-preview-nav-menus.js#L113.
If the fallback happens to generate escape characters, like \birdpress\bird_callback
, then the error doesn't occur, but the value of navMenuArgs.fallback_cb
is mangled.
The attached patch takes a conservative approach by checking for a \
before determining a nav menu supports partial refresh.
I also tested what happens if the backslashes are padded to \\\\
in $exported_args
. It fixed the issue as far as I could tell, but the potential for side-effects seems greater.
Attachments (4)
Change History (15)
#2
@
8 years ago
Given this call:
wp_nav_menu( array( 'fallback_cb' => '\birdpress\bird_callback', 'theme_location' => 'top', 'menu_id' => 'top-menu', ) );
the value of partial.params.navMenuArgs.fallback_cb
I see at the line mentioned above is irdpressird_callback
.
But with this call:
wp_nav_menu( array( 'fallback_cb' => '\WordPress\my_callback', 'theme_location' => 'top', 'menu_id' => 'top-menu', ) );
the value of partial.params.navMenuArgs
is a string
, which throws the 'Missing navMenuArgs'
error.
#3
@
8 years ago
@dlh yeah, I can see that the JSON is getting corrupted. To illustrate that the problem resides in slashing, try adding this patch:
-
src/wp-includes/class-wp-customize-nav-menus.php
final class WP_Customize_Nav_Menus { 1271 1271 if ( isset( $args->customize_preview_nav_menus_args['can_partial_refresh'] ) && $args->customize_preview_nav_menus_args['can_partial_refresh'] ) { 1272 1272 $attributes = sprintf( ' data-customize-partial-id="%s"', esc_attr( 'nav_menu_instance[' . $args->customize_preview_nav_menus_args['args_hmac'] . ']' ) ); 1273 1273 $attributes .= ' data-customize-partial-type="nav_menu_instance"'; 1274 $attributes .= sprintf( ' data-customize-partial-placement-context="%s"', esc_attr( wp_json_encode( $args->customize_preview_nav_menus_args) ) );1274 $attributes .= sprintf( ' data-customize-partial-placement-context="%s"', wp_slash( esc_attr( wp_json_encode( $args->customize_preview_nav_menus_args ) ) ) ); 1275 1275 $nav_menu_content = preg_replace( '#^(<\w+)#', '$1 ' . $attributes, $nav_menu_content, 1 ); 1276 1276 } 1277 1277 return $nav_menu_content;
You'll see that by wrapping the escaped attribute in wp_slash
, it prevents the slashes from getting dropped. Somewhere somehow there is wp_unslash()
or stripslashes()
that is causing the JSON string "\\WordPress\\my_callback"
to become the invalid "\WordPress\my_callback"
.
This is the underlying issue that needs to be fixed.
#4
@
8 years ago
Thanks, I see what you mean now. It looks to me like the preg_replace()
might be the culprit. In 41488.2.diff, I tried to rearrange filter_wp_nav_menu()
so the preg_replace()
happens before injecting $customize_preview_nav_menus_args
.
#5
@
8 years ago
@dlh you've found something very interesting!
Given preg_replace( '#^(.)#', '$1' . json_encode( '\hello\world' ), ' ' )
I would expect it to output:
"\\hello\\world"
But instead it outputs:
"\hello\world"
Compare this with str_replace( ' ', ' ' . json_encode( '\hello\world' ), ' ' )
:
"\\hello\\world"
So yeah, it seems that not only does the pattern need escaping with PCRE but also the replacement, even when there are no capture groups!
By using preg_quote()
on the replacement value, it also fixes the issue: 41488.3.diff.
This seems like a better approach, yes? I wonder how many other instances of preg_replace()
have this problem in code I've written.
#6
@
8 years ago
I wasn't sure about preg_quote()
because it seems to affect a wider range of characters. For example, the data-customize-partial-id
attribute becomes data\-customize\-partial\-id\
(as of PHP 5.3), and :
in JSON becomes \:
.
#7
@
8 years ago
@dlh hummm, you're right:
> echo preg_replace( '/(attr)/', '$1 ' . preg_quote( 'data-customize-partial-id' ) , 'attr' ); < attr data\-customize\-partial\-id
OK, it seems the answer is right in the docs for preg_replace
: http://php.net/manual/en/function.preg-replace.php
To use backslash in replacement, it must be doubled.
I've replaced preg_quote()
with str_replace()
in 41488.4.diff.
#8
@
8 years ago
Tested 41488.4.diff and it worked as expected.
#9
@
8 years ago
- Keywords commit added
- Owner set to westonruter
- Status changed from new to accepted
This could be made part of 4.8.2.
#10
@
8 years ago
Added some test assertions to explicitly check for this issue: https://github.com/WordPress/wordpress-develop/pull/2
@dlh there should be no problem with accepting backslashes in callbacks. What you've come across may be incorrect use slashing or unslashing somewhere in the stack. How does
fallback_cb
get mangled?