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 debug notices for SSR #6413

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
5f1cc86
Add debug notice for SSR
cbravobernal Apr 19, 2024
ef7721d
Add tagname to warning
cbravobernal Apr 22, 2024
12423e6
Update tests checking the notice
cbravobernal Apr 22, 2024
82a1df1
Fix whitespace and rephrase the notice message
cbravobernal Apr 22, 2024
9a172ec
Fix internal directives inside svg or mathml
cbravobernal Apr 23, 2024
8abb040
Refactor tests
cbravobernal Apr 25, 2024
129ee82
Update test to include previous directives triggering the notice
cbravobernal Apr 25, 2024
eff4f6c
Do not check tags inside svgs or math for the notice
cbravobernal Apr 25, 2024
a447486
Forgot translation functions
cbravobernal Apr 25, 2024
df899dd
Move warning to the correct place, update tests
cbravobernal Apr 25, 2024
f624745
Update tests
cbravobernal Apr 25, 2024
5562f12
Rename tests to be able to run them individually
cbravobernal Apr 26, 2024
77d14a8
Update error messages
cbravobernal May 8, 2024
42a0572
Add namespace check and warnings
cbravobernal May 22, 2024
1e7358f
Remove added lines
cbravobernal May 22, 2024
3bf582d
Add since comments
cbravobernal May 22, 2024
03654c4
Add expected incorrect usage
cbravobernal May 22, 2024
6278e20
Address suggestions, still inner svg or math tags left
cbravobernal May 23, 2024
82629d3
Remove null string checking
cbravobernal May 27, 2024
64d0aea
Revert SVG and MATH warning
cbravobernal May 28, 2024
a9f15d8
Update descriptions
cbravobernal May 29, 2024
b15ad5f
Add diferent message for inner SVG or MATH tags
cbravobernal Jun 1, 2024
01c69e0
Address translate suggestions
cbravobernal Jun 3, 2024
3e64ebb
Address suggestions
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
Original file line number Diff line number Diff line change
Expand Up @@ -198,16 +198,19 @@ private function get_balanced_tag_bookmarks() {
public function skip_to_tag_closer(): bool {
$depth = 1;
$tag_name = $this->get_tag();
while ( $depth > 0 && $this->next_tag(
array(
'tag_name' => $tag_name,
'tag_closers' => 'visit',
)
) ) {
if ( $this->has_self_closing_flag() ) {
continue;

while ( $depth > 0 && $this->next_tag( array( 'tag_closers' => 'visit' ) ) ) {
if ( ! $this->is_tag_closer() && $this->get_attribute_names_with_prefix( 'data-wp-' ) ) {
/* translators: 1: SVG or MATH HTML tag. */
$message = sprintf( __( 'Interactivity directives were detected inside an incompatible %1$s tag. These directives will be ignored in the server side render.' ), $tag_name );
_doing_it_wrong( __METHOD__, $message, '6.6.0' );
}
if ( $this->get_tag() === $tag_name ) {
if ( $this->has_self_closing_flag() ) {
continue;
}
$depth += $this->is_tag_closer() ? -1 : 1;
}
$depth += $this->is_tag_closer() ? -1 : 1;
}

return 0 === $depth;
Expand Down
28 changes: 23 additions & 5 deletions src/wp-includes/interactivity-api/class-wp-interactivity-api.php
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,7 @@ public function process_directives( string $html ): string {
* it returns null if the HTML contains unbalanced tags.
*
* @since 6.5.0
* @since 6.6.0 The function displays a warning message when the HTML contains unbalanced tags or a directive appears in a MATH or SVG tag.
*
* @param string $html The HTML content to process.
* @param array $context_stack The reference to the array used to keep track of contexts during processing.
Expand All @@ -295,6 +296,11 @@ private function process_directives_args( string $html, array &$context_stack, a
* We still process the rest of the HTML.
*/
if ( 'SVG' === $tag_name || 'MATH' === $tag_name ) {
if ( $p->get_attribute_names_with_prefix( 'data-wp-' ) ) {
westonruter marked this conversation as resolved.
Show resolved Hide resolved
/* translators: 1: SVG or MATH HTML tag, 2: Namespace of the interactive block. */
$message = sprintf( __( 'Interactivity directives were detected on an incompatible %1$s tag when processing "%2$s". These directives will be ignored in the server side render.' ), $tag_name, end( $namespace_stack ) );
_doing_it_wrong( __METHOD__, $message, '6.6.0' );
}
$p->skip_to_tag_closer();
continue;
}
Expand Down Expand Up @@ -382,31 +388,43 @@ private function process_directives_args( string $html, array &$context_stack, a
}
}
}

/*
* It returns null if the HTML is unbalanced because unbalanced HTML is
* not safe to process. In that case, the Interactivity API runtime will
* update the HTML on the client side during the hydration.
* update the HTML on the client side during the hydration. It will also
* display a notice to the developer to inform them about the issue.
*/
return $unbalanced || 0 < count( $tag_stack ) ? null : $p->get_updated_html();
if ( $unbalanced || 0 < count( $tag_stack ) ) {
$tag_errored = 0 < count( $tag_stack ) ? end( $tag_stack )[0] : $tag_name;
/* translators: %1s: Namespace processed, %2s: The tag that caused the error; could be any HTML tag. */
$message = sprintf( __( 'Interactivity directives failed to process in "%1$s" due to a missing "%2$s" end tag.' ), end( $namespace_stack ), $tag_errored );
_doing_it_wrong( __METHOD__, $message, '6.6.0' );
return null;
}

return $p->get_updated_html();
}

/**
* Evaluates the reference path passed to a directive based on the current
* store namespace, state and context.
*
* @since 6.5.0
* @since 6.6.0 The function now adds a warning when the namespace is null, falsy, or the directive value is empty.
*
* @param string|true $directive_value The directive attribute value string or `true` when it's a boolean attribute.
* @param string $default_namespace The default namespace to use if none is explicitly defined in the directive
* value.
* @param array|false $context The current context for evaluating the directive or false if there is no
* context.
* @return mixed|null The result of the evaluation. Null if the reference path doesn't exist.
* @return mixed|null The result of the evaluation. Null if the reference path doesn't exist or the namespace is falsy.
*/
private function evaluate( $directive_value, string $default_namespace, $context = false ) {
list( $ns, $path ) = $this->extract_directive_value( $directive_value, $default_namespace );
if ( empty( $path ) ) {
if ( ! $ns || ! $path ) {
/* translators: %s: The directive value referenced. */
$message = sprintf( __( 'Namespace or reference path cannot be empty. Directive value referenced: %s' ), $directive_value );
_doing_it_wrong( __METHOD__, $message, '6.6.0' );
return null;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ public function test_wp_bind_doesnt_do_anything_on_non_existent_references() {
* @ticket 60356
*
* @covers ::process_directives
* @expectedIncorrectUsage WP_Interactivity_API::evaluate
*/
public function test_wp_bind_ignores_empty_value() {
$html = '<div data-wp-bind--id="">Text</div>';
Expand All @@ -167,6 +168,7 @@ public function test_wp_bind_ignores_empty_value() {
* @ticket 60356
*
* @covers ::process_directives
* @expectedIncorrectUsage WP_Interactivity_API::evaluate
*/
public function test_wp_bind_ignores_without_value() {
$html = '<div data-wp-bind--id>Text</div>';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,7 @@ public function test_wp_class_doesnt_change_class_attribute_with_empty_directive
* @ticket 60356
*
* @covers ::process_directives
* @expectedIncorrectUsage WP_Interactivity_API::evaluate
*/
public function test_wp_class_doesnt_change_class_attribute_with_empty_value() {
$html = '<div class="other-class" data-wp-class--some-class="">Text</div>';
Expand All @@ -251,6 +252,7 @@ public function test_wp_class_doesnt_change_class_attribute_with_empty_value() {
* @ticket 60356
*
* @covers ::process_directives
* @expectedIncorrectUsage WP_Interactivity_API::evaluate
*/
public function test_wp_class_doesnt_change_class_attribute_without_value() {
$html = '<div class="other-class" data-wp-class--some-class>Text</div>';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,7 @@ public function test_wp_context_works_with_multiple_directives() {
* @ticket 60356
*
* @covers ::process_directives
* @expectedIncorrectUsage WP_Interactivity_API::evaluate
*/
public function test_wp_context_directive_doesnt_work_without_any_namespace() {
$html = '
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -580,6 +580,8 @@ public function test_wp_each_nested_template_tags_using_previous_item_as_list()
* @ticket 60356
*
* @covers ::process_directives
*
* @expectedIncorrectUsage WP_Interactivity_API::process_directives_args
*/
public function test_wp_each_unbalanced_tags() {
$original = '' .
Expand All @@ -598,6 +600,8 @@ public function test_wp_each_unbalanced_tags() {
* @ticket 60356
*
* @covers ::process_directives
*
* @expectedIncorrectUsage WP_Interactivity_API::process_directives_args
*/
public function test_wp_each_unbalanced_tags_in_nested_template_tags() {
$this->interactivity->state( 'myPlugin', array( 'list2' => array( 3, 4 ) ) );
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,7 @@ public function test_wp_style_doesnt_change_style_attribute_with_empty_directive
* @ticket 60356
*
* @covers ::process_directives
* @expectedIncorrectUsage WP_Interactivity_API::evaluate
*/
public function test_wp_style_doesnt_change_style_attribute_with_empty_value() {
$html = '<div style="padding:10px" data-wp-style--color="">Text</div>';
Expand All @@ -379,6 +380,7 @@ public function test_wp_style_doesnt_change_style_attribute_with_empty_value() {
* @ticket 60356
*
* @covers ::process_directives
* @expectedIncorrectUsage WP_Interactivity_API::evaluate
*/
public function test_wp_style_doesnt_change_style_attribute_without_value() {
$html = '<div style="padding: 10px;" data-wp-style--color>Text</div>';
Expand Down
37 changes: 29 additions & 8 deletions tests/phpunit/tests/interactivity-api/wpInteractivityAPI.php
Original file line number Diff line number Diff line change
Expand Up @@ -649,7 +649,10 @@ public function test_process_directives_process_the_directives_in_the_correct_or
*
* @dataProvider data_html_with_unbalanced_tags
*
* @expectedIncorrectUsage WP_Interactivity_API::process_directives_args
*
* @param string $html HTML containing unbalanced tags and also a directive.
*
*/
public function test_process_directives_doesnt_change_html_if_contains_unbalanced_tags( $html ) {
$this->interactivity->state( 'myPlugin', array( 'id' => 'some-id' ) );
Expand Down Expand Up @@ -696,22 +699,17 @@ public function test_process_directives_changes_html_if_contains_svgs() {
);
$html = '
<header>
<svg height="100" data-wp-bind--width="myPlugin::state.width">
<svg height="100">
<title>Red Circle</title>
<circle cx="50" cy="50" r="40" stroke="black" stroke-width="3" fill="red" />
</svg>
<div data-wp-bind--id="myPlugin::state.id"></div>
<div data-wp-bind--id="myPlugin::state.width"></div>
</header>
';
$processed_html = $this->interactivity->process_directives( $html );
$p = new WP_HTML_Tag_Processor( $processed_html );
$p->next_tag( 'svg' );
$this->assertNull( $p->get_attribute( 'width' ) );
$p->next_tag( 'div' );
$this->assertEquals( 'some-id', $p->get_attribute( 'id' ) );
$p->next_tag( 'div' );
$this->assertEquals( '100', $p->get_attribute( 'id' ) );
}
Comment on lines 701 to 713
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be done in a separate PR? As far as I can tell, it doesn't have
anything to do with the changes in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is just cleaning unnecessary repetitions on a test. Is a nitpick.


/**
Expand All @@ -721,6 +719,7 @@ public function test_process_directives_changes_html_if_contains_svgs() {
* @ticket 60517
*
* @covers ::process_directives
* @expectedIncorrectUsage WP_Interactivity_API_Directives_Processor::skip_to_tag_closer
*/
public function test_process_directives_does_not_change_inner_html_in_svgs() {
$this->interactivity->state(
Expand Down Expand Up @@ -750,6 +749,7 @@ public function test_process_directives_does_not_change_inner_html_in_svgs() {
* @ticket 60517
*
* @covers ::process_directives
* @expectedIncorrectUsage WP_Interactivity_API::process_directives_args
*/
public function test_process_directives_change_html_if_contains_math() {
$this->interactivity->state(
Expand Down Expand Up @@ -784,6 +784,8 @@ public function test_process_directives_change_html_if_contains_math() {
* @ticket 60517
*
* @covers ::process_directives
* @expectedIncorrectUsage WP_Interactivity_API::process_directives_args
* @expectedIncorrectUsage WP_Interactivity_API_Directives_Processor::skip_to_tag_closer
*/
public function test_process_directives_does_not_change_inner_html_in_math() {
$this->interactivity->state(
Expand Down Expand Up @@ -814,7 +816,7 @@ 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 ) {
private function evaluate( $directive_value, $default_namespace = 'myPlugin' ) {
$generate_state = function ( $name ) {
return array(
'key' => $name,
Expand All @@ -829,7 +831,7 @@ private function evaluate( $directive_value ) {
);
$evaluate = new ReflectionMethod( $this->interactivity, 'evaluate' );
$evaluate->setAccessible( true );
return $evaluate->invokeArgs( $this->interactivity, array( $directive_value, 'myPlugin', $context ) );
return $evaluate->invokeArgs( $this->interactivity, array( $directive_value, $default_namespace, $context ) );
}

/**
Expand Down Expand Up @@ -923,6 +925,25 @@ public function test_evaluate_nested_value() {
$this->assertEquals( 'otherPlugin-context-nested', $result );
}

/**
* Tests the `evaluate` method for non valid namespace values.
*
* @ticket 61044
*
* @covers ::evaluate
* @expectedIncorrectUsage WP_Interactivity_API::evaluate
*/
public function test_evaluate_unvalid_namespaces() {
$result = $this->evaluate( 'path', 'null' );
$this->assertNull( $result );

$result = $this->evaluate( 'path', '' );
$this->assertNull( $result );

$result = $this->evaluate( 'path', '{}' );
$this->assertNull( $result );
}

/**
* Tests the `kebab_to_camel_case` method.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
*
* @group interactivity-api
*/
class Tests_WP_Interactivity_API_WP_Text extends WP_UnitTestCase {
class Tests_Interactivity_API_WpInteractivityAPIWPText extends WP_UnitTestCase {
/**
* Instance of WP_Interactivity_API.
*
Expand Down Expand Up @@ -117,6 +117,7 @@ public function test_wp_text_sets_inner_content_with_nested_tags() {
* @ticket 60356
*
* @covers ::process_directives
*
*/
public function test_wp_text_sets_inner_content_even_with_unbalanced_but_different_tags_inside_content() {
$html = '<div data-wp-text="myPlugin::state.text"><span>Text</div>';
Expand All @@ -131,6 +132,8 @@ public function test_wp_text_sets_inner_content_even_with_unbalanced_but_differe
* @ticket 60356
*
* @covers ::process_directives
*
* @expectedIncorrectUsage WP_Interactivity_API::process_directives_args
*/
public function test_wp_text_fails_with_unbalanced_and_same_tags_inside_content() {
$html = '<div data-wp-text="myPlugin::state.text">Text<div></div>';
Expand Down
Loading