Interactivity API bug: Fix fatal error when trying to use a stdClass as state-context. #6672

cbravobernal · 2024-05-29T15:03:07Z

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

Updates #6581 with an unit test.

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.

…k well in server directives

github-actions · 2024-05-29T15:03:29Z

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.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @narendraie.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

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

Props narenin, cbravobernal, jonsurrell, gziolo, czapla.

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

github-actions · 2024-05-29T15:19:33Z

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

This looks good in general, thanks both!

sirreal · 2024-05-29T15:35:23Z

tests/phpunit/tests/interactivity-api/wpInteractivityAPI.php

@@ -785,6 +788,9 @@ public function test_evaluate_value() {

 		$result = $this->evaluate( 'otherPlugin::context.key' );
 		$this->assertEquals( 'otherPlugin-context', $result );
+
+		$result = $this->evaluate( 'state.obj.prop' );
+		$this->assertEquals( 'property', $result );


Suggested change

$this->assertEquals( 'property', $result );

$this->assertSame( 'property', $result );

sirreal · 2024-05-29T16:24:27Z

tests/phpunit/tests/interactivity-api/wpInteractivityAPI.php

+		$obj            = new stdClass();
+		$obj->prop      = 'property';
+		$generate_state = function ( $name, $obj = null ) {


It seems like this could be simplified and follow the same pattern as the rest of the state and statically set $obj->property = $name instead of taking an $obj parameter.

sirreal · 2024-05-29T16:24:57Z

tests/phpunit/tests/interactivity-api/wpInteractivityAPI.php

@@ -748,14 +748,17 @@ public function test_process_directives_does_not_change_inner_html_in_math() {
 	 * @param string $directive_value The directive attribute value to evaluate.
 	 * @return mixed The result of the evaluate method.
 	 */
-	private function evaluate( $directive_value ) {
-		$generate_state = function ( $name ) {
+	private function evaluate( $directive_value, ) {


Trailing comma may be a problem in some supported PHP versions.

Suggested change

private function evaluate( $directive_value, ) {

private function evaluate( $directive_value ) {

it is being indeed, thanks! I was getting crazy

michalczaplinski · 2024-05-30T11:33:23Z

It looks good to me, too! Nice work 👍

sirreal

Shall we test ArrayAccess too?

sirreal · 2024-05-30T11:57:05Z

tests/phpunit/tests/interactivity-api/wpInteractivityAPI.php

 			return array(
 				'key'    => $name,
 				'nested' => array( 'key' => $name . '-nested' ),
+				'obj'    => $obj,
 			);


We can also test array access:

Suggested change

);

'arrAccess' => new class() implements ArrayAccess {

public function offsetExists($offset): bool { return true; }

public function offsetGet($offset): mixed { return $offset; }

public function offsetSet($offset, $value): void {}

public function offsetUnset($offset): void {}

},

);

sirreal · 2024-05-30T12:00:26Z

tests/phpunit/tests/interactivity-api/wpInteractivityAPI.php

@@ -785,6 +789,9 @@ public function test_evaluate_value() {

 		$result = $this->evaluate( 'otherPlugin::context.key' );
 		$this->assertEquals( 'otherPlugin-context', $result );
+
+		$result = $this->evaluate( 'state.obj.prop' );
+		$this->assertSame( 'myPlugin-state', $result );


This is interesting, I don't know what result we'll get here. I assume it's a string 1. I'm not sure how the path is parsed and if it accounts for numeric access or coercion happens so that numeric string paths access arrays:

Suggested change

$this->assertSame( 'myPlugin-state', $result );

$this->assertSame( 'myPlugin-state', $result );

$result = $this->evaluate( 'state.arrAccess.1' );

$this->assertSame( '1', $result );

In this case, state.arrAccess.1, 1 is treated first a string. You can operate, in example

public function offsetGet( $offset ): mixed { return 2 * $offset; }

And will return 2 as a number.

What would be the best approach here?

I think it's fine as it. It's just interesting that "1" is used to access numeric arrays.

cbravobernal · 2024-05-31T11:24:30Z

tests/phpunit/tests/interactivity-api/wpInteractivityAPI.php

+				'arrAccess' => new class() implements ArrayAccess {
+					public function offsetExists( $offset ): bool {
+						return true; }
+					#[\ReturnTypeWillChange]


@sirreal that test implies adding a this line in order to be compatible with PHP 7.2

It seems that the mixed typing is only available in PHP 8 and above.

Should we keep this test then?

Can you omit the bool type hint as the return value?

I tested this for the class body and it works (without errors, deprecations, or warnings) on php 8.3 and 7.2, let's use this:

public function offsetExists( $offset ): bool { return true; } #[\ReturnTypeWillChange] public function offsetGet( $offset ) { return $offset; } public function offsetSet( $offset, $value ): void {} public function offsetUnset( $offset ): void {}

Can you omit the bool type hint as the return value?

That triggers a deprecation notice in recent versions (8.3).

I'd like to keep the test, if we support the behavior a test is a good thing 🙂

Can you omit the bool type hint as the return value?

Nope, it will trigger:

Fatal error: During inheritance of ArrayAccess: Uncaught Return type of ArrayAccess@anonymous::offsetExists($offset) should either be compatible with ArrayAccess::offsetExists(mixed $offset): bool, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice

This is what I meant based on the usage in other places:

gziolo · 2024-06-03T12:12:11Z

@cbravobernal, is this patch ready to land? #6672 (comment) – any actions to take here?

cbravobernal · 2024-06-03T15:19:22Z

@cbravobernal , is this patch ready to land? #6672 (comment) – any actions to take here?

I removed the bool type and added #[\ReturnTypeWillChange] at the beginning of the function like you mentioned to keep consistency with other parts of the code. Let's see the tests.

cbravobernal · 2024-06-03T18:47:38Z

All green so ready to land!

gziolo · 2024-06-04T06:57:04Z

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

narenin and others added 6 commits May 17, 2024 14:45

Fixed : Return value of isn't used

a205fce

Fixed - Interactivity API issue for Some property access does not wor…

08b0696

…k well in server directives

Removed unrelated changes

ab5bc6b

Implemented Suggestions

318fdcc

Add unit test

bb8ab0b

Remove not needed obj

916d741

cbravobernal mentioned this pull request May 29, 2024

Interactivity API: Iteration for WP 6.6 WordPress/gutenberg#60219

Closed

cbravobernal mentioned this pull request May 29, 2024

Fixed - Interactivity API : Some property access does not work well in server directives #6581

Closed

Additional check

e032c84

sirreal approved these changes May 29, 2024

View reviewed changes

cbravobernal added 2 commits May 29, 2024 19:21

Remove trailing comma

a1a3130

Address suggestion

5d6223f

sirreal reviewed May 30, 2024

View reviewed changes

cbravobernal added 3 commits May 31, 2024 12:09

Add array access test

659f124

removed not compatible mixed type in > 8

dc96757

Try return type will change

0b6d529

cbravobernal commented May 31, 2024

View reviewed changes

Implement returntypewillchange similar to other places

94ebb6e

Move function to the beginning of offset

e0855f6

gziolo closed this Jun 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Interactivity API bug: Fix fatal error when trying to use a stdClass as state-context. #6672

Interactivity API bug: Fix fatal error when trying to use a stdClass as state-context. #6672

cbravobernal commented May 29, 2024

github-actions bot commented May 29, 2024 •

edited

Loading

github-actions bot commented May 29, 2024

sirreal left a comment

sirreal May 29, 2024

sirreal May 29, 2024

sirreal May 29, 2024

cbravobernal May 29, 2024

michalczaplinski commented May 30, 2024

sirreal left a comment

sirreal May 30, 2024

sirreal May 30, 2024

cbravobernal May 31, 2024

sirreal May 31, 2024

cbravobernal May 31, 2024

gziolo May 31, 2024

sirreal May 31, 2024

sirreal May 31, 2024

cbravobernal Jun 3, 2024 •

edited

Loading

gziolo Jun 3, 2024

gziolo commented Jun 3, 2024

cbravobernal commented Jun 3, 2024

cbravobernal commented Jun 3, 2024

gziolo commented Jun 4, 2024

	$this->assertEquals( 'property', $result );
	$this->assertSame( 'property', $result );

	private function evaluate( $directive_value, ) {
	private function evaluate( $directive_value ) {

-			);
+				'arrAccess' => new class() implements ArrayAccess {
+					public function offsetExists($offset): bool { return true; }
+					public function offsetGet($offset): mixed { return $offset; }
+					public function offsetSet($offset, $value): void {}
+					public function offsetUnset($offset): void {}
+				},
+			);

Interactivity API bug: Fix fatal error when trying to use a stdClass as state-context. #6672

Interactivity API bug: Fix fatal error when trying to use a stdClass as state-context. #6672

Conversation

cbravobernal commented May 29, 2024

github-actions bot commented May 29, 2024 • edited Loading

Unlinked Accounts

github-actions bot commented May 29, 2024

Test using WordPress Playground

Some things to be aware of

sirreal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalczaplinski commented May 30, 2024

sirreal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbravobernal Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Jun 3, 2024

cbravobernal commented Jun 3, 2024

cbravobernal commented Jun 3, 2024

gziolo commented Jun 4, 2024

github-actions bot commented May 29, 2024 •

edited

Loading

cbravobernal Jun 3, 2024 •

edited

Loading