Adding escaping function in wp_interactivity_data_wp_context PHPDoc

[mosne]

in the /wp-includes/interactivity-api/interactivity-api.php the following example is given in the PHPDoc:

<div <?php echo wp_interactivity_data_wp_context( array( 'isOpen' => true, 'count' => 0 ) ); ?>>

We should always follow best practices, so I propose adding proper escaping:

<div <?php echo wwp_kses_data( wp_interactivity_data_wp_context( array( 'isOpen' => true, 'count' => 0  ) ) ); ?>>

[mosne]

P.S. In case the wp_interactivity_data_wp_context function is truly safe, we could add it to the coding standard exceptions.

[audrasjb]

As this is a Docs related enhancement, let's try milestoning it to 6.6.

…ata_wp_context

Trac ticket: #61430

[Rahe]

Seems good to me :)

[jonsurrell]

I think this function should be added as an exception. It returns an HTML attribute name and value escaped and ready to print. It doesn't seem like there's anything for kses to do here.

​From wp_kses_data (emphasis mine):

$data string required
Content to filter, expected to not be escaped.

---

I did notice that the description is wrong ​here for wp_interactivity_data_wp_context:

This helper function simplifies the creation of data-wp-context directives by providing a way to pass an array of data, which encodes into a JSON string safe for direct use as a HTML attribute value.

Although the return description does hint that it returns the full attribute "a complete … directive":

Return
string A complete data-wp-context directive with a JSON encoded value representing the context array and the store namespace if specified.

It seems like that could be improved to clarify that it returns and HTML attribute suitable to be output to the page.

[audrasjb]

Replying to jonsurrell:

I think this function should be added as an exception. It returns an HTML attribute name and value escaped and ready to print. It doesn't seem like there's anything for kses to do here.

​From wp_kses_data (emphasis mine):

$data string required
Content to filter, expected to not be escaped.

By the way, it's worth noting that the contributor who filled this ticket opened it after the plugin review team asked him to escape the result of this function.

That's why it seems like it made sense to enforce escaping in the example provided.

I did notice that the description is wrong ​here for wp_interactivity_data_wp_context:

This helper function simplifies the creation of data-wp-context directives by providing a way to pass an array of data, which encodes into a JSON string safe for direct use as a HTML attribute value.

Although the return description does hint that it returns the full attribute "a complete … directive":

Return
string A complete data-wp-context directive with a JSON encoded value representing the context array and the store namespace if specified.

It seems like that could be improved to clarify that it returns and HTML attribute suitable to be output to the page.

Great point!

https://core.trac.wordpress.org/ticket/61430

Instead of adding escape function we should update docblock check comment here

[audrasjb]

Moving to 6.6.1.

[hellofromTonya]

Can the patch be ready and committed quickly for a 6.6.1 RC (which is earmarked for later in the day tomorrow, July 18th - no time is set yet)? If no, can move it to 6.6.2.

[hellofromTonya]

With 6.6.1 RC happening in a couple of hours, moving this ticket to 6.6.2.

[dmsnell]

Both open PRs are attempting to escape the content, which will likely introduce corruption into the interactivity data.

@hellofromTonya please let's reject both patches as proposed.

On the other hand, a documentation update to this function would be helpful, and should be easy to approve. There is no outstanding defect if I'm interpreting this correct, so there are no code changes to resolve this ticket, and nothing to test.

The only thing that's necessary is ensuring that our coding standards aren't accidentally encouraging people to introduce breakage by applying rules where they aren't appropriate, such as in this case where that seems to have occurred (again). Hopefully someone on this ticket can rearrange their patch to reflect this.

Maybe someone with more experience in the WPCS domain could help by proposing a rule change to exempt this function?

[hellofromTonya]

Discussed in today's 6.6.2 bug scrub.

This ticket doesn't yet have consensus. Marking it as 2nd-opinion to denote it needs discussion of whether the change should happen or not.

Given it does not appear to be a change from 6.6 cycle, moving it to 6.7.

[desrosj]

This one hasn't seen any activity during the 6.7 cycle. With the 6.7 beta 1 deadline in less than 24 hours, I'm going to punt this to future release.