Make WordPress Core

Opened 8 years ago

Closed 4 years ago

#36263 closed defect (bug) (fixed)

When building a shortcode with `wp.shortcode.string()` the `attrs` property is finnicky.

Reported by: georgestephanis's profile georgestephanis Owned by: yale01's profile yale01
Milestone: 5.4 Priority: low
Severity: minor Version: 3.5
Component: Shortcodes Keywords: dev-feedback 2nd-opinion good-first-bug has-patch has-unit-tests
Focuses: javascript Cc:

Description

If you pass it in as:

wp.shortcode.string({
	tag   : 'short',
	type  : 'single',
	attrs : {
		named   : { foo : 'bar' },
		numeric : [ 'abc123' ]
	}
});

it works fine -- resulting in [short abc123 foo="bar"], but if you flip the order so that numeric comes before name, like so:

wp.shortcode.string({
	tag   : 'short',
	type  : 'single',
	attrs : {
		numeric : [ 'abc123' ],
		named   : { foo : 'bar' }
	}
});

the output is [short numeric="abc123" named="[object Object]"]

This is because we're doing an overly basic array comparison of the keys in which order matters --

https://core.trac.wordpress.org/browser/trunk/src/wp-includes/js/shortcode.js?rev=34933#L218

// Identify a correctly formatted `attrs` object.
} else if ( _.isEqual( _.keys( attrs ), [ 'named', 'numeric' ] ) ) {
	this.attrs = attrs;

I wonder if we could do something as simple as using _.difference instead --

} else if ( _.difference( _.keys( attrs ), [ 'named', 'numeric' ] ).length === 0 ) {
	this.attrs = _.defaults( attrs, { named : {}, numeric : [] } );

or something?

Open to suggestions.

Attachments (2)

36263.diff (628 bytes) - added by yale01 7 years ago.
improved array keys comparison
36263.2.diff (1.7 KB) - added by adamsilverstein 7 years ago.

Download all attachments as: .zip

Change History (17)

#1 @georgestephanis
8 years ago

  • Keywords good-first-bug added

#2 @zsusag
8 years ago

@georgestephanis

In theory that seems to work; however, I'm wondering how one would be able to use wp.shortcode.string(). I'm relatively new here to the forums and to WordPress core but if I am correct most users implement shortcode via PHP instead of JavaScript. Am I mistaken in this? I'm trying to replicate your bug.

#4 @georgestephanis
8 years ago

But perhaps more to your question -- when you're inserting media from the media modal in the editor screen into a post body, yes, it fires an ajax request to get the shortcode generated by php. Because legacy reasons. But if you're using Editor Views and editing them in JS, that all happens client-side, purely in JS.

#5 @zsusag
8 years ago

@georgestephanis Ahh, I see. Thank you for the examples and explanation.

With the greater insight that does seem to be a better and more "friendly" way of handling things. I haven't found time to formally set up a test for your code but it does look like that would be a more intuitive way of going about things.

#6 @obenland
8 years ago

  • Version changed from trunk to 3.5

#7 @mircoraffinetti
8 years ago

@georgestephanis: just a short feedback. We tested your suggested code and it works fine.

#8 @DrewAPicture
8 years ago

@georgestephanis Any follow up here?

@yale01
7 years ago

improved array keys comparison

#9 @yale01
7 years ago

patch added, from code provided @georgestephanis and tested againt the case he provided

Version 0, edited 7 years ago by yale01 (next)

#10 @yale01
7 years ago

  • Keywords has-patch added

#11 @adamsilverstein
7 years ago

  • Keywords has-unit-tests added

36263.2.diff adds a quint test verifying the fix

#12 @DrewAPicture
7 years ago

  • Owner set to yale01
  • Status changed from new to assigned

Assigning ownership to mark the good-first-bug as "claimed".

This ticket was mentioned in Slack in #core by sergey. View the logs.


4 years ago

#14 @SergeyBiryukov
4 years ago

  • Milestone changed from Awaiting Review to 5.4

#15 @SergeyBiryukov
4 years ago

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

In 47003:

Shortcodes: Make sure wp.shortcode.string() accepts the attrs array keys in any order.

Props yale01, georgestephanis, adamsilverstein, zsusag, mircoraffinetti, SergeyBiryukov.
Fixes #36263.

Note: See TracTickets for help on using tickets.