Skip to content

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

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.

Sign up for GitHub

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

Closed
cbravobernal wants to merge 14 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
a205fce
Fixed : Return value of isn't used
narenin May 17, 2024
08b0696
Fixed - Interactivity API issue for Some property access does not wor…
narenin May 20, 2024
ab5bc6b
Removed unrelated changes
narenin May 20, 2024
318fdcc
Implemented Suggestions
narendraie May 21, 2024
bb8ab0b
Add unit test
cbravobernal May 29, 2024
916d741
Remove not needed obj
cbravobernal May 29, 2024
e032c84
Additional check
cbravobernal May 29, 2024
a1a3130
Remove trailing comma
cbravobernal May 29, 2024
5d6223f
Address suggestion
cbravobernal May 29, 2024
659f124
Add array access test
cbravobernal May 31, 2024
dc96757
removed not compatible mixed type in > 8
cbravobernal May 31, 2024
0b6d529
Try return type will change
cbravobernal May 31, 2024
94ebb6e
Implement returntypewillchange similar to other places
cbravobernal Jun 3, 2024
e0855f6
Move function to the beginning of offset
cbravobernal Jun 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/wp-includes/interactivity-api/class-wp-interactivity-api.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,8 +389,10 @@ private function evaluate( $directive_value, string $default_namespace, $context
$path_segments = explode( '.', $path );
$current = $store;
foreach ( $path_segments as $path_segment ) {
if ( isset( $current[ $path_segment ] ) ) {
if ( ( is_array( $current ) || $current instanceof ArrayAccess ) && isset( $current[ $path_segment ] ) ) {
$current = $current[ $path_segment ];
} elseif ( is_object( $current ) && isset( $current->$path_segment ) ) {
$current = $current->$path_segment;
} else {
return null;
}
Expand Down
24 changes: 22 additions & 2 deletions tests/phpunit/tests/interactivity-api/wpInteractivityAPI.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -750,16 +750,30 @@ public function test_process_directives_does_not_change_inner_html_in_math() {
*/
private function evaluate( $directive_value ) {
$generate_state = function ( $name ) {
$obj = new stdClass();
$obj->prop = $name;
return array(
'key' => $name,
'nested' => array( 'key' => $name . '-nested' ),
'key' => $name,
'nested' => array( 'key' => $name . '-nested' ),
'obj' => $obj,
'arrAccess' => new class() implements ArrayAccess {
#[\ReturnTypeWillChange]
Copy link
Contributor Author

@cbravobernal cbravobernal May 31, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member

@gziolo gziolo May 31, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member

@sirreal sirreal May 31, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member

@sirreal sirreal May 31, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@cbravobernal cbravobernal Jun 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

@gziolo gziolo Jun 3, 2024

Choose a reason for hiding this comment

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

This is what I meant based on the usage in other places:
Screenshot 2024-06-03 at 14 09 44

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

public function offsetGet( $offset ): string {
return $offset; }
public function offsetSet( $offset, $value ): void {}
public function offsetUnset( $offset ): void {}
},
);
Copy link
Member

@sirreal sirreal May 30, 2024

Choose a reason for hiding this comment

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

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 {}
},
);

};
$this->interactivity->state( 'myPlugin', $generate_state( 'myPlugin-state' ) );
$this->interactivity->state( 'otherPlugin', $generate_state( 'otherPlugin-state' ) );
$context = array(
'myPlugin' => $generate_state( 'myPlugin-context' ),
'otherPlugin' => $generate_state( 'otherPlugin-context' ),
'obj' => new stdClass(),
);
$evaluate = new ReflectionMethod( $this->interactivity, 'evaluate' );
$evaluate->setAccessible( true );
Expand All @@ -785,6 +799,12 @@ 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 );
Copy link
Member

@sirreal sirreal May 30, 2024

Choose a reason for hiding this comment

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

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 );

Copy link
Contributor Author

@cbravobernal cbravobernal May 31, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Member

@sirreal sirreal May 31, 2024

Choose a reason for hiding this comment

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

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


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

/**
Expand Down