Refactor: `data_wp_context` function does not follow WP standards.

[cbravobernal]

As @swisspidy commented in this discussion: ​https://github.com/WordPress/gutenberg/pull/58943#discussion_r1494618835

`data_wp_context() is the first unprefixed WordPress function in Core.

​https://github.com/WordPress/wordpress-develop/blob/8ee7651d0e953f0bdc5705bb5359f4067fefa89b/src/wp-includes/interactivity-api/interactivity-api.php#L142-L166

Also, the function name does not indicate fully what it does. It could be renamed to:

• wp_interactivity_get_context_attribute
• wp_interactivity_context_attr
• wp_interactivity_create_context_directive_attr

Please feel free to discuss, I think we are on time to change it before 6.5

[swissspidy]

data_wp_context() is the first unprefixed WordPress function in Core.

That's not what I said and also not true. I said it is the only function in src/wp-includes/interactivity-api/interactivity-api.php that doesn't have a wp_interactivity_ prefix.

[youknowriad]

The name does seem not specific enough. It’s not clear that it relates to interactivity or HTML generation

[ankitmaru]

Absolutely agree with @swissspidy's assessment. It's crucial to maintain consistent naming conventions in WordPress Core, especially considering the vast ecosystem built around it.

Renaming data_wp_context() to something more descriptive like wp_interactivity_get_context_attribute or wp_interactivity_context_attr would not only enhance clarity but also contribute to better code readability and maintainability.

[gziolo]

The name looked fine when surrounded by isolated Interactivity API code. I agree it should get updated as it is too generic.

Aside, I'm still unsure I like this emerging pattern of helper methods that return a string formatted as an HTML partial of an attribute name and its value. In this case, it's helpful, but we should seek better ways to construct HTML as explored in #60229.

[cbravobernal]

Replying to swissspidy:

data_wp_context() is the first unprefixed WordPress function in Core.

That's not what I said and also not true. I said it is the only function in src/wp-includes/interactivity-api/interactivity-api.php that doesn't have a wp_interactivity_ prefix.

Sorry if misunderstood. Anyway I agree with what you mentioned.

[westonruter]

If we wanted to follow the existing pattern of WordPress ​template tags (which we probably shouldn't!), like the_title_attribute(), then it could be the_data_wp_context_attribute(). Consider also body_class() versus get_body_class().

Since most template tags have a the_ version that prints and a get_ version that... gets, perhaps there should be two:

• wp_interactivity_get_context_attr()
• wp_interactivity_the_context_attr() or better just wp_interactivity_context_attr() without the_

Having a function that prints would avoid PHPCS complaints about printing unescaped output, although that would also be solved by adding it to ​EscapingFunctionsTrait::$autoEscapedFunctions.

Compare:

<div <?php echo wp_interactivity_get_context_attr( $context, $namespace ); ?>>

<div <?php wp_interactivity_context_attr( $context, $namespace ); ?>>

The second is briefer.

Nevertheless, perhaps prior art here is get_block_wrapper_attributes() which notably does not have a prefix and it doesn't have a the_block_wrapper_attributes() variant. I guess it depends on how often the function would be needing to return a value versus print the value.

[cbravobernal]

I guess it depends on how often the function would be needing to return a value versus print the value.

As the function is used to include the HTML attribute in a tag, I can bet on almost always needing to print the value.

[swissspidy]

Actually, in the places the function is currently used in core (render_block_core_search and WP_Navigation_Block_Renderer, it's not used in printing, just returning.

I don't really see value in data_wp_context returning the whole data-wp-context="..." attribute string. This could just as well return only the encoded attribute value.

Then we could name the function wp_interactivity_encode_context( array $context, string $store_namespace ); and users would do data-wp-context=" . wp_interactivity_encode_context(...) . " (or with sprintf or the HTML API or whatever)

[gziolo]

I don't really see value in data_wp_context returning the whole data-wp-context="..." attribute string. This could just as well return only the encoded attribute value.

That sounds like a good compromise to return the value.

[swissspidy]

Alright, so maybe instead of wp_interactivity_encode_context() what if we call it wp_interactivity_get_context_directive()? Any other suggestions?

When we make this change, we'll also need to update the two core blocks in the Gutenberg repo to use the new function. Ideally so that it doesn't break when using the plugin with an older beta. Who can help with that?

We should aim to fix this for beta 3.

[cbravobernal]

Replying to swissspidy:

Alright, so maybe instead of wp_interactivity_encode_context() what if we call it wp_interactivity_get_context_directive()? Any other suggestions?

When we make this change, we'll also need to update the two core blocks in the Gutenberg repo to use the new function. Ideally so that it doesn't break when using the plugin with an older beta. Who can help with that?

We should aim to fix this for beta 3.

I can make a compat fallback for Gutenberg.

For me it is not yet clear what to do:

• Only rename the function to: wp_interactivity_get_context_directive
• Edit the function to return the attribute content instead of the entire directive.

[swissspidy]

For me it is not yet clear what to do:

We don't have consensus on that yet, but I propose doing both.

[luisherranz]

It's not that easy.

First, it's not easy to spot, but Pascal's code is wrong. It also seems like nobody else here noticed.

users would do data-wp-context=" . wp_interactivity_encode_context(...) . "

The correct code is data-wp-context=' . wp_interactivity_encode_context(...) . '.

It's not Pascal's fault. The problem is not obvious by looking at the code. It's also not easy to notice and fix once it happens. So if seasoned developers like us make this mistake, imagine other less experienced developers.

Second, writing data-wp-context manually is not safe.

Here, $some_value could contain '> <script>attack()</script>:

<div data-wp-context='{ "someValue": "<?php echo $some_value; ?>" }'>

Even though this one looks better on the surface, it is also subject to the same attacks:

<div data-wp-context='<?php echo wp_json_encode( array( 'someValue' => $some_value ) ); ?>'>

Developers should not have to learn these quirks. We must provide them with simple functions that are easy to remember, easy to use, and always safe.

That's the approach the HTML API is following, and that's what we should follow for the Interactivity API as well.

It's our responsibility to maximize the chances that developers will use this helper. For that purpose, we must:

• Educate developers to always use the helper function and never write it manually.
• Provide them with the best possible developer experience to do so.

Educating alone is not enough. If the developer experience is not great, developers will tend to forget/avoid the cumbersome/boilerplate, and just write data-wp-context manually, leading to errors and attacks. For that reason, I think the developer experience should be the top priority here.

The best developer experience possible is always the simplest one. If this is in the intent of the developer:

<?php
$context = array(
  'someValue' => $some_value,
);
?>
<div 
  <?php echo get_block_wrapper_attributes(); ?>
  data-wp-interactive="myPlugin"
  data-wp-context='{ ... }'
>

The simplest thing we can ask them is to replace their data-wp-context string with a data_wp_context() call.

<?php
$context = array(
  'someValue' => $some_value,
);
?>
<div 
  <?php echo get_block_wrapper_attributes(); ?>
  data-wp-interactive="myPlugin"
  <?php echo data_wp_context( $context ); ?>
>

There's nothing extra for them to remember, but to use the data_wp_context PHP function instead of the data-wp-context string. That's it. No new names, no extra syntax, no single/double quotes problems…

Echoing the result is fine, as that's a pattern they are already using, and it prevents them from having to remember two different function names, one for render.php and another for render_callback.

All other options suggested here are valid, but they are suboptimal in my opinion.

By the way, I'm also looking forward to HTML templating (#60229), but we should not wait for that. We should provide the best possible solution today. For injecting directives using the HTML Tag Processor, we should also provide a helper, but that's a separate use case and it should be a separate conversation.

[swissspidy]

I'm sure you understand that there can be typos in some quick example code. That doesn't really make a case against it.

It's our responsibility to maximize the chances that developers will use this helper.

So why only a helper for data-wp-context and not for others?

Consistency is also part of developer experience. That includes consistent names and behaviors.

Just because data_wp_context reads the same as data-wp-context doesn't make it a good function name. So a function with a prefix that's consistent with other wp_interactivity_api functions makes more sense.

And functions that return a complete attribute="value" pair are not common either. So is something like data-wp-context=' . wp_interactivity_encode_context(...) . ' really that inconvenient?

If someone uses the wrong quotes and their code breaks, they'll realize quickly, no?

Someone can do data-wp-context='{ "someValue": "<?php echo $some_value; ?>" }' even if there is a nice data_wp_context filter. People will always find a way.

Echoing by default doesn't really make sense to me as a) you can easily echo a return value but not the other way around, and b) the return use case is more common, as existing examples show.

[luisherranz]

I'm sure you understand that there can be typos in some quick example code

Maybe it was a typo to you, but that's exactly what I would expect someone to write. And it has already happened on several occasions to people experimenting with the Interactivity API.

It's just not obvious that the result of wp_interactivity_encode_context( array( 'someValue' => $some_value ) ) will contain double quotes. Nothing indicates that. It's also not obvious that when an attribute value contains double quotes, it needs to be surrounded by single quotes. People are not usually exposed to that because it's a very rare case. We cannot assume that because we know those things, everybody will know. And we can't expect that everybody will carefully read the documentation.

If someone uses the wrong quotes and their code breaks, they'll realize quickly, no?

I don't think so. It's not that straightforward to discover. And again, we've already seen that with people experimenting with the Interactivity API who didn't manage to figure out the problem until they showed us the code.

Why only a helper for data-wp-context and not for others?

Each directive that contains a JSON string in its value will need one of these. Initially, we were going to add data_wp_interactive as well, but as there is only one available key ("namespace"), we added the ability to use strings instead of JSON strings.

These helpers don't need to be exclusive to directives using JSON strings. Other future directives that might need some sort of special server validation could have helpers as well.

Echoing by default doesn't really make sense to me as a) you can easily echo a return value but not the other way around, and b) the return use case is more common, as existing examples show

I agree!

Someone can do data-wp-context='{ "someValue": "<?php echo $some_value; ?>" }' even if there is a nice data_wp_context filter. People will always find a way.

Absolutely. That's the reason I think this should be as easy and straightforward as possible, to maximize the chances of people using it.

[swissspidy]

So if we forget about the function's behavior for a moment, what's your take on simply renaming the function for more consistency? We initially suggested names such as wp_interactivity_get_context_attribute for more clarity and consistency.

[luisherranz]

what's your take on simply renaming the function for more consistency?

I know that you mean consistency with other WordPress functions, but to me, it's more important to keep consistency with what the function is replacing: data-wp-context. So data_wp_context is the most consistent name in my opinion. It's also the only one where people don't need to remember one extra concept because it's a direct mental link data-wp-context <-> data_wp_context.

I know that's not the answer that you're looking for, so just to understand the issue better, may I know what type of problems you envision if we follow that path and use data_wp_directivename for these helpers? What's at stake here?

[swissspidy]

The current function is not great for developer experience. Again:

• The name is not consistent with other functions related to the Interactivity API nor with any other functions in core.
• Lack of precedence for similar helper functions in core that output such a attr=value pair, thus lack of consistency with rest of core.
• And mainly, it's simply not clear what the function belongs to nor what it does. Heck, it could even be mistaken as a data provider for unit tests.

That's why this whole ticket was created in the first place.

If we keep data_wp_context() as-is, we're stuck with it forever. So better to fix that now before publishing documentation and trying to explain it to everyone.

[luisherranz]

We have different visions about what great developer experience means, but it's fine 🙂

Let's go with wp_interactivity_data_wp_context then, so at least it's as easy to remember as possible, and the name implies that the data-wp-context part is included.

[darerodz]

My two cents.

I would choose wp_interactivity_data_wp_context, as Luis just proposed. I think it's clear that the function belongs to the Interactivity API and is related to the data-wp-context directive, without adding other words to the name (e.g., directive, attr).

In my opinion, other terms mentioned in the discussion wouldn't be necessary, as their meaning would be implicit, like encode (because devs are not going to decode the context), or get (similar case with set).

[cbravobernal]

I agree that adding a get could also lead to think about needing a set. And in this case, the context is set previously by defining a PHP variable, while the function is just transforming data.

Trac ticket: https://core.trac.wordpress.org/ticket/60575

cc @luisherranz

It works for me. @c4rl0sbr4v0 and @darerodz, can we proceed with it? It will require changes in WordPress packages for core blocks and adjustment to the dev note.

Gutenberg version:
​https://github.com/WordPress/gutenberg/pull/59465

Gutenberg version:
​https://github.com/WordPress/gutenberg/pull/59465

It works for me. @c4rl0sbr4v0 and @DAreRodz, can we proceed with it? It will require changes in WordPress packages for core blocks and adjustment to the dev note.

Sure @gziolo .
Dev note is updated. Handbook and Gutenberg updates are available here:

​https://github.com/WordPress/gutenberg/pull/59465

[swissspidy]

In 57742:

Interactivity API: Rename data_wp_context() to wp_interactivity_data_wp_context().

Increases clarity about where the function belongs to, bringing it in line with other related functions.

Props swissspidy, gziolo, cbravobernal, youknowriad, ankitmaru, westonruter, luisherranz, darerodz.
Fixes #60575.

Thanks y'all! Committed in https://core.trac.wordpress.org/changeset/57742

[swissspidy]

In 57743:

Interactivity API: Revert [57742] pending a Gutenberg package update.

This function can only be renamed after updating Gutenberg npm packages, as some of the core blocks already use this function.

See #60575.

[swissspidy]

Marking as commit again because we can simply re-add this change together with the package update.

[swissspidy]

In 57762:

Interactivity API: Rename data_wp_context() to wp_interactivity_data_wp_context().

Increases clarity about where the function belongs to, bringing it in line with other related functions.

After initially merging this change in [57742] and reverting it in [57743], this reintroduces it now that the Gutenberg packages have been updated accordingly in [57760].

Props swissspidy, gziolo, cbravobernal, youknowriad, ankitmaru, westonruter, luisherranz, darerodz.
Fixes #60575.