-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Changes from 22 commits
5f1cc86
ef7721d
12423e6
82a1df1
9a172ec
8abb040
129ee82
eff4f6c
a447486
df899dd
f624745
5562f12
77d14a8
42a0572
1e7358f
3bf582d
03654c4
6278e20
82629d3
64d0aea
a9f15d8
b15ad5f
01c69e0
3e64ebb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -198,16 +198,23 @@ 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; | ||
if ( ! $this->is_tag_closer() && $this->get_attribute_names_with_prefix( 'data-wp-' ) ) { | ||
/* translators: 1: SVG or MATH HTML tag, 2: Namespace of the interactive block. */ | ||
$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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These whitespaces look wrong. It needs to be double checked before committing. |
||
} | ||
$depth += $this->is_tag_closer() ? -1 : 1; | ||
} | ||
$depth += $this->is_tag_closer() ? -1 : 1; | ||
} | ||
|
||
return 0 === $depth; | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 3e64ebb |
||||||
* | ||||||
* @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. | ||||||
|
@@ -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; | ||||||
} | ||||||
|
@@ -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: %s: The tag that caused the error; could be any HTML tag. */ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are two tokens, so the hints for translators need some improvements. |
||||||
$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 ); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This message isn't translated but contains a hint for translators. |
||||||
_doing_it_wrong( __METHOD__, $message, '6.6.0' ); | ||||||
return null; | ||||||
} | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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' ) ); | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is just cleaning unnecessary repetitions on a test. Is a nitpick. |
||
|
||
/** | ||
|
@@ -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( | ||
|
@@ -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( | ||
|
@@ -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( | ||
|
@@ -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, | ||
|
@@ -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 ) ); | ||
} | ||
|
||
/** | ||
|
@@ -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. | ||
* | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see %2 in the translation. The hint for translators needs to be updated.