Interactivity API: Add server derived state getter handling

[sirreal]

Implement derived state callback handling in server directive processing.

The implementation expects a Closure type. call_user_func was avoided because it would be difficult to differentiate the string paths in the directive from a callable function name.

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

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[github-actions[bot]]

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

• The Plugin and Theme Directories cannot be accessed within Playground.
• All changes will be lost when closing a tab with a Playground instance.
• All changes will be lost when refreshing the page.
• A fresh instance is created each time the link below is clicked.
• Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance, it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

[github-actions[bot]]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props jonsurrell, darerodz, gziolo, luisherranz, cbravobernal.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[luisherranz]

Thanks for working on this, Jon 🙂

I think function closures is the way to go!

We had them implemented exactly as you've done here before, but we removed them to have more time to evaluate them before its public release.

However, what about mimicking the client store and, instead of this:

wp_interactivity_state( 'myPlugin', array(
  'list' => array(1, 2, 3),
  'text' => __('this is'),
  'itemText' => function( $store ) {
    return $store['state']['text'] . ': ' . $store['context']['item'];
  }
));

Do this:

$state = wp_interactivity_state( 'myPlugin', array(
  'list' => array(1, 2, 3),
  'text' => __('this is'),
  'itemText' => function() use ($state) {
    $context = wp_interactivity_get_context( 'myPlugin' );
    return $state['text'] . ': ' . $context['item'];
  }
));

That way, it'd work with the "same API" in the client and in the server.

What do you think?

[DAreRodz]

That way, it'd work with the "same API" in the client and in the server.

What do you think?

I think that would be nice. 😄

Just a note: for both approaches, if you want to read from a getter, I guess you would have to execute it, right? And it's the same for both approaches, with the difference that, for the second case, you don't have to pass any arguments. Following the example above, let's imagine that state.text is also a getter:

Approach 1

wp_interactivity_state( 'myPlugin', array(
  'list' => array(1, 2, 3),
  'text' => function ( $store ) {
    return __( $store['context']['text'] );
  },
  'itemText' => function ( $store ) {
    return $store['state']['text']( $store ) . ': ' . $store['context']['item'];
  }
));

Approach 2

$state = wp_interactivity_state( 'myPlugin', array(
  'list' => array(1, 2, 3),
  'text' => function () {
    $context = wp_interactivity_get_context( 'myPlugin' );
    return __( $store['context']['text'] );
  },
  'itemText' => function () use ( $state ) {
    $context = wp_interactivity_get_context( 'myPlugin' );
    return $state['text']() . ': ' . $context['item'];
  }
));

I see a benefit with the second approach: you don't need a reference to the $store to read the value from a state getter. 🤔

[sirreal]

I don't think it's viable to use a closure over the $state variable. That's effectively a snapshot of the state, but I think we should have access to the full state when the callback runs. I believe this aligns with what we have on the client.

Consider the following:

$s1 = wp_interactivity_state( 'ns', array( 'x'=>'y' ) );
$s2 = wp_interactivity_state( 'ns', array( 'z'=>'2' ) );

var_dump($s1, $s2, $s1 === $s2);

This prints:

array(1) {
  ["x"]=>
  string(1) "y"
}
array(2) {
  ["x"]=>
  string(1) "y"
  ["z"]=>
  string(1) "2"
}
bool(false)

The state changes and $s1 is out of date. Our closure might see that (depending on subsequent state changes). When the callback is evaluated, it should access the up-to-date current state at that moment.

If we were to use closures, we'd probably have to resort to some ugly reference passing that I don't think is a good idea.

[luisherranz]

What about this?

wp_interactivity_state( 'myPlugin', array(
  'list' => array(1, 2, 3),
  'text' => __('this is'),
  'itemText' => function() {
    $state   = wp_interactivity_state( 'myPlugin' );
    $context = wp_interactivity_get_context( 'myPlugin' );
    return $state['text'] . ': ' . $context['item'];
  }
));

[sirreal]

That seems like it would work, but what's the advantage? Is there a drawback to passing in the state and context as parameters? We'd have to implement the context function which doesn't exist right now as far as I know. That function would also only make sense when called in one of these functions while processing directives, right?

[luisherranz]

Is there a drawback to passing in the state and context as parameters?

I see a few drawbacks/inconsistencies with the old syntax.

1. In the client, the context is not part of the store, so store['context'] in the server would be inconsistent.
2. In the server, stores don't exist per se, only the initial state of those client stores, so store itself doesn't make much sense (as opposed to accessing state directly).
3. In the client, getters don't get the store via arguments.
4. In the client you can access other namespaces passing the namespace to the functions (store( 'otherPlugin' ) or getContext( 'otherPlugin')). If we use arguments, it's not clear how to get the state or context from a different namespace.

I agree that using functions to get the state and context feels boilerplaty (especially with WordPress' long function names), but it would be the way to make it more consistent with the client experience.

By the way, as those server "getters" are synchronous, maybe we can populate the default namespace and omit it, like we do in the client:

wp_interactivity_state( 'myPlugin', array(
  'list' => array(1, 2, 3),
  'text' => __('this is'),
  'itemText' => function() {
    $state   = wp_interactivity_state();
    $context = wp_interactivity_get_context();
    return $state['text'] . ': ' . $context['item'];
  }
));

[DAreRodz]

A couple of notes after playing a bit with this PR, trying to follow @luisherranz's approach:

• The functions to access the state and the context inside getters (wp_interactivity_state and wp_interactivity_get_context) are global.
• They have access to the global WP_Interactivity_API instance.
• They depend on the current state, context and namespace stacks during a process_directives() call.
• The state is available from a WP_Interactivity_API instance, but the context and namespace aren't. Instead, they're local to the process_directives() method and passed down to other methods as arguments (see this).
• That's what we're trying to avoid in this case... 🤔

To solve this, I guess we need to move the context and namespace stacks to class properties. @sirreal, do you agree with this approach? Any concerns or different ideas?

---

PS: if we do this, I imagine that would require updating a bunch of PHPUnit tests... 😬

[sirreal]

Yes, I think that makes sense because those global functions need to access those things.

[DAreRodz]

@sirreal I think I've addressed all your suggestions. Could you take another quick look?

If everything is OK, I guess the PR is ready to be merged. cc @gziolo.

[gziolo]

@sirreal or @DAreRodz, can you rebase this PR so I could commit it?

[gziolo]

Committed with https://core.trac.wordpress.org/changeset/58327.