Interactivity API: Add debug notices for SSR #6413

cbravobernal · 2024-04-19T15:17:43Z

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

This PR aims to improve the developer experience of the Interactivity API server directives processing.
Right now, the SSR won't work if the HTML is unbalanced (there is some closer tag missing).

I added also notices for cases where the directive is empty, or the namespace is an empty string, an empty object or null.

This PR adds a warning in those cases, if the developer is in debug mode.

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.

github-actions · 2024-04-19T15:35:51Z

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.

github-actions · 2024-04-22T15:19:35Z

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 cbravobernal, jonsurrell, westonruter, darerodz, czapla.

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

sirreal

There's an annotation that I believe is the preferred way to implement these tests:

/**
 * @expectedIncorrectUsage WP_Interactivity_API::process_directives_args
 */

sirreal · 2024-04-24T16:16:45Z

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

@@ -639,7 +660,8 @@ public function test_process_directives_changes_html_if_contains_svgs() {
 			</header>
 		';
 		$processed_html = $this->interactivity->process_directives( $html );
-		$p              = new WP_HTML_Tag_Processor( $processed_html );
+		$this->setExpectedIncorrectUsage( 'WP_Interactivity_API::process_directives_args' );


This is available as an annotation. Add this to the tests that are expected to fail and all the changes to implement this tracking can be removed:

* @expectedIncorrectUsage WP_Interactivity_API::process_directives_args

src/wp-includes/interactivity-api/class-wp-interactivity-api.php

sirreal

I left a few notes but generally I think this is in a good place. I think the messages could be improved to be a bit more brief. I also think it might be helpful to include the interactivity namespace so that it's easier to locate the problem if there are many different interactive regions, what do you think?

For SVG/MATH:

Interactivity directives were detected on an incompatible %s tag when processing {{ namespace }}. These directives will be ignored.

For unbalanced (this is always due to a start tag without an end tag, right?):

Interactivity directives failed to process in {{ namespace }} due to a missing %s end tag.

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

src/wp-includes/interactivity-api/class-wp-interactivity-api.php

sirreal

I think the messages could be a bit more brief (#6413 (review)) but this is working well generally. Thanks!

cbravobernal · 2024-05-08T15:56:10Z

I think the messages could be a bit more brief (#6413 (review)) but this is working well generally. Thanks!

Done in 4b78690

cbravobernal · 2024-05-22T09:43:07Z

I'm adding more checks to the SSR processing. Now it will also check that the namespace is not {}, "" "empty string", or null written as a string.
That way it won't cause weird behaviors in the runtime.

src/wp-includes/interactivity-api/class-wp-interactivity-api.php

westonruter · 2024-05-23T17:50:04Z

src/wp-includes/interactivity-api/class-wp-interactivity-api.php

@@ -295,6 +297,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-' ) ) {


I suppose it's possible for a directive to be used on a tag inside of an svg or math element tree, in which case the directives are ignored and there is no such notice displayed, correct?

I suppose the only way to deal with that would be to add this same condition inside of skip_to_tag_closer, for example:

diff --git a/wp-includes/interactivity-api/class-wp-interactivity-api-directives-processor.php b/wp-includes/interactivity-api/class-wp-interactivity-api-directives-processor.php index 3b2dcb12..ef04ac92 100644 --- a/wp-includes/interactivity-api/class-wp-interactivity-api-directives-processor.php +++ b/wp-includes/interactivity-api/class-wp-interactivity-api-directives-processor.php @@ -204,6 +204,11 @@ final class WP_Interactivity_API_Directives_Processor extends WP_HTML_Tag_Proces 'tag_closers' => 'visit', ) ) ) { + 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 on an incompatible %1$s tag when processing "%2$s". These directives will be ignored.' ), $tag_name, end( $namespace_stack ) ); + _doing_it_wrong( __METHOD__, $message, '6.6.0' ); + } if ( $this->has_self_closing_flag() ) { continue; }

I think I tried to check the content in 9a172ec but was getting the entire root block and I reverted in eff4f6c

As far as I remember, inner tags were not processed. But, as I reread, it seems that if there are no directives in the main SVG or MATH tags, but there are tags inside those elements, they won't be processed and there won't be any notice.

I'll add a test for that and update the code with your proposal. Thanks!!

It seems that is not so easy to determine the content inside an SVG tag. So checking if it has any directive would not be possible.

HTML API will support SVG in a future, so I removed this notice.

Discussed in the public Slack channel:

https://wordpress.slack.com/archives/C071CRKGKUP/p1716839075325949

I'm not sure I understand, as it is currently walking over all the content inside the SVG tag in the skip_to_tag_closer method. But if the intention is that SVG will be supported soon anyway, then I guess it doesn't make sense to add a warning.

I could not reach the inner content inside that function, is there something I may be missing?

ping @sirreal

It does look like you should be able to reach all the tags there and could show debug messages. The issue in the screenshot seems to be with the next_tag() query, which is searching for the next tag with the matching name (SVG). Something like this should visit all the tags and provide a chance to show the message there:

diff --git before/src/wp-includes/interactivity-api/class-wp-interactivity-api-directives-processor.php after/src/wp-includes/interactivity-api/class-wp-interactivity-api-directives-processor.php index b12dcb4b3b1..2e15f583136 100644 --- before/src/wp-includes/interactivity-api/class-wp-interactivity-api-directives-processor.php +++ after/src/wp-includes/interactivity-api/class-wp-interactivity-api-directives-processor.php @@ -199,15 +199,17 @@ final class WP_Interactivity_API_Directives_Processor extends WP_HTML_Tag_Proces $depth = 1; $tag_name = $this->get_tag(); while ( $depth > 0 && $this->next_tag( - array( - 'tag_name' => $tag_name, - 'tag_closers' => 'visit', - ) + array( 'tag_closers' => 'visit' ) ) ) { - if ( $this->has_self_closing_flag() ) { - continue; + if ( ! $this->is_tag_closer() && ! empty( $this->get_attribute_names_with_prefix( 'data-wp-' ) ) ) { + // show warning + } + 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;

src/wp-includes/interactivity-api/class-wp-interactivity-api.php

sirreal · 2024-05-23T18:04:10Z

src/wp-includes/interactivity-api/class-wp-interactivity-api.php

 	 */
 	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 ( 'null' === $default_namespace ) {


Is this supposed to be the value null or the string "null"? It's the string now which looks pretty strange.

It is strange, but is the string. Is read as a valid string namespace for SSR but then crashes in the runtime (need to 100% confirm this)

The JS runtime calls JSON.parse on directives… maybe it should only do that in directives that look like objects.

I assume many stringified JS could make that crash now.

I think any strings should be valid namespaces. null shouldn't be special.

Seems like a bug.

Then I'll try to fix it! 🚀

Using numbers only triggers the same error 😅

Fixed WordPress/gutenberg#61960

sirreal · 2024-05-24T17:33:35Z

src/wp-includes/interactivity-api/class-wp-interactivity-api.php

 	 */
 	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 ( 'null' === $default_namespace ) {


Can we remove this section expecting WordPress/gutenberg#61960 to land?

DAreRodz

I think this PR is ready. 👌 I just left some suggestions.

DAreRodz · 2024-05-29T09:48:50Z

src/wp-includes/interactivity-api/class-wp-interactivity-api.php

+	 * @since 6.6.0 The function now adds a warning when the HTML contains unbalanced
+	 *              tags or SVG/Math with directives.


As we finally removed the debug message about using directives inside <svg> or <math> tags, we would need to update this description.

Suggested change

* @since 6.6.0 The function now adds a warning when the HTML contains unbalanced

* tags or SVG/Math with directives.

* @since 6.6.0 The function displays a warning message when the HTML contains

* unbalanced tags.

Done in a9f15d8

DAreRodz · 2024-05-29T09:58:34Z

src/wp-includes/interactivity-api/class-wp-interactivity-api.php

-		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: Tag that caused the error, could by any HTML tag. */


Not sure if there's a typo here. 👇

Suggested change

/* translators: %s: Tag that caused the error, could by any HTML tag. */

/* translators: %s: The tag that caused the error; could be any HTML tag. */

Done in a9f15d8

michalczaplinski · 2024-05-29T11:23:44Z

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

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


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.

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

michalczaplinski · 2024-05-29T12:32:58Z

Overall, it looks good and is close to be ready to merge IMO. I left a comment and had the same comments as David above.

Also, it would be good to add a unit test that asserts that the warning message appears correctly in the right circumstances.

It could be something like:

	/**
	 * Tests that the `process_directives` issues a warning when the HTML
	 * containing directives contains unbalanced tags.
	 *
	 * @ticket 61044
	 *
	 * @covers ::process_directives
	 *
	 * @expectedIncorrectUsage WP_Interactivity_API::process_directives_args
	 */
	public function test_process_directives_warns_on_unbalanced_tags() {
		$this->expectOutputString( 'Interactivity directives failed to process in "" due to a missing "P" end tag' );

		$this->interactivity->state( 'myPlugin', array( 'id' => '1231341324' ) );

		$this->interactivity->process_directives(
			'<div>
				<p data-wp-bind--id="myPlugin::state.id">
					Hello, world
			</div>
			</div>
		'
		);
	}

I have tried adding that test but I wasn't able to make it work for some reason:

The warning message seems to be hidden when using the @expectedIncorrectUsage WP_Interactivity_API::process_directives_args annotation.
When not using the @expectedIncorrectUsage WP_Interactivity_API::process_directives_args annotation, I used the $this->expectOutputString() or $this->expectNoticeMessage() but they don't seem to be triggered by the doing_it_wrong() call in the body of process_directives().

cbravobernal · 2024-05-29T14:09:04Z

Overall, it looks good and is close to be ready to merge IMO. I left a comment and had the same comments as David above.

Also, it would be good to add a unit test that asserts that the warning message appears correctly in the right circumstances.

It could be something like:
	/**
	 * Tests that the `process_directives` issues a warning when the HTML
	 * containing directives contains unbalanced tags.
	 *
	 * @ticket 61044
	 *
	 * @covers ::process_directives
	 *
	 * @expectedIncorrectUsage WP_Interactivity_API::process_directives_args
	 */
	public function test_process_directives_warns_on_unbalanced_tags() {
		$this->expectOutputString( 'Interactivity directives failed to process in "" due to a missing "P" end tag' );

		$this->interactivity->state( 'myPlugin', array( 'id' => '1231341324' ) );

		$this->interactivity->process_directives(
			'<div>
				<p data-wp-bind--id="myPlugin::state.id">
					Hello, world
			</div>
			</div>
		'
		);
	}
I have tried adding that test but I wasn't able to make it work for some reason:

The warning message seems to be hidden when using the @expectedIncorrectUsage WP_Interactivity_API::process_directives_args annotation.

When not using the @expectedIncorrectUsage WP_Interactivity_API::process_directives_args annotation, I used the $this->expectOutputString() or $this->expectNoticeMessage() but they don't seem to be triggered by the doing_it_wrong() call in the body of process_directives().

The only case that makes me think about it would be how would work in a future a test like this with two different warnings. 🤔

michalczaplinski · 2024-05-29T21:05:05Z

The only case that makes me think about it would be how would work in a future a test like this with two different warnings. 🤔

I'm sorry, I don't understand what you mean. Why would there be two different warnings?

DAreRodz · 2024-05-31T09:00:03Z

I'm sorry, I don't understand what you mean. Why would there be two different warnings?

I think he means that the only warning message the process_directives_args method outputs is the one for unbalanced tags, and thus, testing the warning message content is not required.

Although the message content changes depending on the tag that caused the error, right? @cbravobernal

cbravobernal · 2024-05-31T09:03:48Z

I think he means that the only warning message the process_directives_args method outputs is the one for unbalanced tags, and thus, testing the warning message content is not required.

I mean that. Maybe in a future, we warn for another cause (I don't know, a directive value processed in that function that requires any specific rule).
We only test that a warning is expected, not which one.

Although the message content changes depending on the tag that caused the error, right?

Yes, the message includes the tag processed when the unbalance variable sets to true.

cbravobernal mentioned this pull request Apr 19, 2024

Interactivity API: Debug - Warning about Server Directives Processing errors. WordPress/gutenberg#60035

Open

cbravobernal force-pushed the feature/add-ssr-notice branch from 225334a to 24199ab Compare April 22, 2024 14:57

cbravobernal marked this pull request as ready for review April 22, 2024 15:19

cbravobernal mentioned this pull request Apr 22, 2024

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

Open

sirreal suggested changes Apr 24, 2024

View reviewed changes

sirreal suggested changes Apr 25, 2024

View reviewed changes

src/wp-includes/interactivity-api/class-wp-interactivity-api.php Outdated Show resolved Hide resolved

src/wp-includes/interactivity-api/class-wp-interactivity-api.php Outdated Show resolved Hide resolved

cbravobernal force-pushed the feature/add-ssr-notice branch from 32fa2d8 to 5758d38 Compare April 25, 2024 15:24

sirreal reviewed Apr 26, 2024

View reviewed changes

sirreal approved these changes Apr 26, 2024

View reviewed changes

cbravobernal added 13 commits May 22, 2024 11:40

Add debug notice for SSR

5f1cc86

Add tagname to warning

ef7721d

Update tests checking the notice

12423e6

Fix whitespace and rephrase the notice message

82a1df1

Fix internal directives inside svg or mathml

9a172ec

Refactor tests

8abb040

Update test to include previous directives triggering the notice

129ee82

Do not check tags inside svgs or math for the notice

eff4f6c

Forgot translation functions

a447486

Move warning to the correct place, update tests

df899dd

Update tests

f624745

Rename tests to be able to run them individually

5562f12

Update error messages

77d14a8

cbravobernal marked this pull request as draft May 22, 2024 09:42

Add namespace check and warnings

42a0572

cbravobernal force-pushed the feature/add-ssr-notice branch from 4b78690 to 42a0572 Compare May 22, 2024 14:23

Remove added lines

1e7358f

cbravobernal added 2 commits May 22, 2024 16:38

Add since comments

3bf582d

Add expected incorrect usage

03654c4

cbravobernal marked this pull request as ready for review May 22, 2024 20:08

cbravobernal changed the title ~~Interactivity API: Add debug notice for SSR~~ Interactivity API: Add debug notices for SSR May 22, 2024

westonruter reviewed May 23, 2024

View reviewed changes

sirreal reviewed May 23, 2024

View reviewed changes

Address suggestions, still inner svg or math tags left

6278e20

sirreal reviewed May 24, 2024

View reviewed changes

cbravobernal added 2 commits May 27, 2024 14:18

Remove null string checking

82629d3

Revert SVG and MATH warning

64d0aea

DAreRodz approved these changes May 29, 2024

View reviewed changes

michalczaplinski reviewed May 29, 2024

View reviewed changes

Update descriptions

a9f15d8

cbravobernal mentioned this pull request May 29, 2024

Unbalanced tags cause SSR not to work Automattic/a4a-partner-directory-plugin#3

Merged

Directory

Interactivity API: Add debug notices for SSR #6413

Are you sure you want to change the base?

Interactivity API: Add debug notices for SSR #6413

Conversation

cbravobernal commented Apr 19, 2024 • edited

github-actions bot commented Apr 19, 2024

Test using WordPress Playground

Some things to be aware of

github-actions bot commented Apr 22, 2024 • edited

sirreal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirreal left a comment

Choose a reason for hiding this comment

sirreal left a comment

Choose a reason for hiding this comment

cbravobernal commented May 8, 2024

cbravobernal commented May 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbravobernal May 28, 2024 • edited

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

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

DAreRodz 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

michalczaplinski commented May 29, 2024

cbravobernal commented May 29, 2024

michalczaplinski commented May 29, 2024

DAreRodz commented May 31, 2024

cbravobernal commented May 31, 2024 • edited

cbravobernal commented Apr 19, 2024 •

edited

github-actions bot commented Apr 22, 2024 •

edited

cbravobernal May 28, 2024 •

edited

cbravobernal commented May 31, 2024 •

edited