Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interactivity API: Add server derived state getter handling #6394

Closed

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Apr 15, 2024

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.

Copy link

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.

@sirreal sirreal marked this pull request as ready for review April 18, 2024 11:17
Copy link

github-actions bot commented Apr 18, 2024

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
Copy link
Member

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
Copy link

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
Copy link
Member Author

sirreal commented Apr 23, 2024

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
Copy link
Member

luisherranz commented Apr 25, 2024

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
Copy link
Member Author

sirreal commented Apr 25, 2024

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
Copy link
Member

luisherranz commented May 2, 2024

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
Copy link

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
Copy link
Member Author

sirreal commented May 21, 2024

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

* @return string|null The processed HTML content. It returns null when the HTML contains unbalanced tags.
*/
private function process_directives_args( string $html, array &$context_stack, array &$namespace_stack ) {
private function process_directives_args( string $html ) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to keep this method separated as an internal function that can be called recursively (e.g., inside data_wp_each_processor). However, I guess the name process_directives_args is no longer appropriate.

Do you have new name ideas? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the name wrong now?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that it's wrong now, haha. But maybe it is? 😄 I just thought that the _args part in the name referred to the context and namespace stacks being passed as args, so it was no longer relevant.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see. So theres process_directives (public function) which calls process_directives_args, and that suffix doesn't make sense.

Yes, I think it could be updated, it's private anyways. Maybe it could just be _process_directives?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 2215c9a.

PS: I tried to come up with a more descriptive name (with no success). 😅

Copy link
Member Author

@sirreal sirreal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @DAreRodz, I left some suggestions and questions, but I don't see any major issues right now.

Comment on lines 119 to 145
public function state( string $store_namespace = '', array $state = array() ): array {
if ( ! $store_namespace ) {
$store_namespace = end( $this->namespace_stack );
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a function with a required argument and an optional argument. Now, I think it's more complicated. It's a function with one behavior with 0-1 arguments, and with 2 arguments it's different. For example, state( '', [ 'foo' => 'bar' ] ) is wrong, right? Setting state requires an explicit namespace:

  • state() => lookup
  • state( 'ns' ) => lookup
  • state( 'ns', ['x'=>'y'] ) => set + lookup
    • state( '', ['x'=>'y'] ) => ⛔️ wrong!

We should never be calling this like state( '', [ 'foo' => 'bar' ] ), right?

Suggested change
public function state( string $store_namespace = '', array $state = array() ): array {
if ( ! $store_namespace ) {
$store_namespace = end( $this->namespace_stack );
}
public function state( ?string $store_namespace = null, ?array $state = null ): array {
if ( ! $store_namespace ) {
if ( $state ) {
// doing it wrong? shouldn't have falsy ns + state setting
}
if ( null !== $store_namespace ) {
// doing it wrong? Empty string is not allowed.
}
$store_namespace = end( $this->namespace_stack );
}

It also seems like null is a better default value, we don't need to create the array and it should work well with the is_array check later. It seems like the array_replace_recursive call with empty array wouldn't do anything, right?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that's better behavior. I'll update the implementation.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 93acfc0.

*
* @param string $store_namespace Optional. The unique store namespace identifier.
*/
public function get_context( string $store_namespace = '' ): array {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems preferable to me to use null to indicate "doesn't exist" instead of empty string. We could also add "doing it wrong" messages here if empty string is passed.

Suggested change
public function get_context( string $store_namespace = '' ): array {
public function get_context( ?string $store_namespace = null ): array {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 42e3e62.

* @return string|null The processed HTML content. It returns null when the HTML contains unbalanced tags.
*/
private function process_directives_args( string $html, array &$context_stack, array &$namespace_stack ) {
private function process_directives_args( string $html ) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the name wrong now?

@@ -396,6 +459,10 @@ private function evaluate( $directive_value, string $default_namespace, $context
}
}

if ( $current instanceof Closure ) {
$current = $current();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like an easy place to break things by passing a closure that throws. Should we wrap this in try/catch with doing it wrong?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 3f571c0.

Comment on lines 183 to 186
* @param string $store_namespace Optional. The unique store namespace identifier.
* @return array The context for the specified store namespace.
*/
function wp_interactivity_get_context( string $store_namespace = '' ): array {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommended this inside the class.

The wp_interactivity_state function above also needs to be updated to match the class method.

Suggested change
* @param string $store_namespace Optional. The unique store namespace identifier.
* @return array The context for the specified store namespace.
*/
function wp_interactivity_get_context( string $store_namespace = '' ): array {
* @param string|null $store_namespace Optional. The unique store namespace identifier.
* @return array The context for the specified store namespace.
*/
function wp_interactivity_get_context( ?string $store_namespace ): array {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks better. Thanks. 👍

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 48ebafd.

Comment on lines 916 to 917
$state = $this->interactivity->state();
$context = $this->interactivity->get_context();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any tests using the global functions?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't. I guess I could use them here, but I'd have to modify the global WP_Interactivity_API instance, which is the one used by the global functions.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in a1fb279.

@DAreRodz
Copy link

@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.

* @covers ::state
*/
public function test_state_without_namespace() {
$interactivity = new ReflectionClass( $this->interactivity );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to rewrite the tests to avoid using reflections and exposing all the internals? It's very difficult to say how it all works just by looking at the test. One of the benefits of having code coverage is also documenting how the API can be used.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this is difficult to follow and seems to rely a lot on internals.

This sounds like a big refactor (maybe it could be pulled out to its own PR landed before this) but could we do something where we reset the global Interactivity API between tests in setup or teardown methods? Maybe that could use Reflection if absolutely necessary, but at least it should be isolate to one place.

I'd really like to see tests setting declaring their own state. I think the tests would become easier to read for the most part and it would make it easier to understand when there are multiple namespaces in play.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been thinking about approaching this refactor and don't see an easy fix. A thing that comes up would be to inject the namespace and context stacks somehow, maybe during the WP_Interactivity_API instantiation. Or maybe pass them as arguments to the evaluate method, even though they are private properties.

Another thing would be if it makes sense to test private methods in unit tests. Maybe we need to rethink the way tests are written. 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK leaving this for now.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd really like to see tests setting declaring their own state. I think the tests would become easier to read for the most part and it would make it easier to understand when there are multiple namespaces in play.

@gziolo, @sirreal, I've created two helper methods to define the internal namespace and context stacks and moved the relevant state to each test case. At least the test code is easier to understand and maintain this way.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems much better, thanks.

Copy link
Member Author

@sirreal sirreal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work on this @DAreRodz! A lot of this looks very nice, I've left a few comments and questions and I'd like to know your thoughts.

*/
public function test_evaluate_derived_state() {
$result = $this->evaluate( 'state.derived' );
$this->assertEquals( "Derived state: myPlugin-state\nDerived context: myPlugin-context", $result );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a preference to use assertSame where possible (strict equals vs. loose equals):

Suggested change
$this->assertEquals( "Derived state: myPlugin-state\nDerived context: myPlugin-context", $result );
$this->assertSame( "Derived state: myPlugin-state\nDerived context: myPlugin-context", $result );

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 3cf4236.

*/
public function test_evaluate_derived_state_that_throws() {
$result = $this->evaluate( 'state.derivedThatThrows' );
$this->assertEquals( null, $result );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$this->assertEquals( null, $result );
$this->assertNull( $result );

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 3cf4236.

* @covers ::state
*/
public function test_state_without_namespace() {
$interactivity = new ReflectionClass( $this->interactivity );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this is difficult to follow and seems to rely a lot on internals.

This sounds like a big refactor (maybe it could be pulled out to its own PR landed before this) but could we do something where we reset the global Interactivity API between tests in setup or teardown methods? Maybe that could use Reflection if absolutely necessary, but at least it should be isolate to one place.

I'd really like to see tests setting declaring their own state. I think the tests would become easier to read for the most part and it would make it easier to understand when there are multiple namespaces in play.

Comment on lines -227 to -262
$context_stack = array();
$namespace_stack = array();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing that I wonder about, at this top level these values always started empty at this level. I wonder if the properties should also be cleaned here. Are they ever reset to an empty array? Could we get into a state where we expect these to be empty (to start processing) but a previous call left them "dirty" with data from the last process call?

I am having some second thoughts about moving these to a class property because they are really scoped to this function. One alternative might be to pass them by reference. I think the result would be similar but they'd be clearly scoped to this function instead of class properties that only make sense at a particular time in the class.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am having some second thoughts about moving these to a class property because they are really scoped to this function. One alternative might be to pass them by reference. I think the result would be similar but they'd be clearly scoped to this function instead of class properties that only make sense at a particular time in the class.

That's how it worked previously, right?

To be honest, I completely agree it would be a better approach. The main issue is how to make that temporary state available through a class method, i.e., wp_interactivity()->get_context(). That's why I decided to use class properties.

Would it make sense to preserve all the previous logic but store some internal references to the locally instantiated context & namespace stacks so we can use them inside get_context()? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main issue is how to make that temporary state available through a class method, i.e., wp_interactivity()->get_context()

Right 😅 I forgot about this essential bit. Let's keep the class properties.

What if these stacks were created when this function runs and nulled when the function returns so they're unusable outside of directive processing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DAreRodz What do you think about this part of the question?

One thing that I wonder about, at this top level these values always started empty at this level. I wonder if the properties should also be cleaned here. Are they ever reset to an empty array? Could we get into a state where we expect these to be empty (to start processing) but a previous call left them "dirty" with data from the last process call?

Should these properties should be nullable? They'd be set to arrays during directive processing and set back to null when processing is complete. That would give us a nice way to show _doing_it_wrong messages when using functions the wrong way or outside of processing, it would be a null check.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense, as this method is synchronously executed, and there is no opportunity to call it again in the middle.

Done in 670022b. I've also added a _doing_it_wrong message for state() and get_context() when they are called outside of a process_directives call.

Copy link
Member Author

@sirreal sirreal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @DAreRodz, thanks for working through all the details. I can't approve since it's "my" PR, but I think this is in a good place ✅

(I left two small suggestions to avoid "after" language in a comment when we really mean "before or after").

Copy link
Member

@luisherranz luisherranz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've only had the chance to take a quick look at it, but the code seems fine to me. I approve it on behalf of Jon 🙂

Copy link

@cbravobernal cbravobernal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR looks great and would be awesome to add it this release.

@gziolo
Copy link
Member

gziolo commented Jun 4, 2024

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

@sirreal sirreal force-pushed the add/interactivity-derived-state-getter branch from 02c9ff0 to 69aa352 Compare June 4, 2024 08:41
@DAreRodz DAreRodz force-pushed the add/interactivity-derived-state-getter branch from 69aa352 to a0f0e9e Compare June 4, 2024 09:25
Copy link

@DAreRodz DAreRodz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the rebase, @sirreal! 👏

@gziolo
Copy link
Member

gziolo commented Jun 4, 2024

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

@gziolo gziolo closed this Jun 4, 2024
@gziolo gziolo deleted the add/interactivity-derived-state-getter branch June 4, 2024 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants