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

Conversation

cbravobernal
Copy link
Contributor

@cbravobernal cbravobernal commented Apr 19, 2024

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.

Screenshot 2024-04-22 at 12 50 48


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.

Copy link

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.

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

github-actions bot commented Apr 22, 2024

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, gziolo.

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

Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

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

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

@@ -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' );
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

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

@cbravobernal
Copy link
Contributor Author

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

Done in 4b78690

@cbravobernal cbravobernal marked this pull request as draft May 22, 2024 09:42
@cbravobernal
Copy link
Contributor Author

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.

@cbravobernal cbravobernal marked this pull request as ready for review May 22, 2024 20:08
@cbravobernal cbravobernal changed the title Interactivity API: Add debug notice for SSR Interactivity API: Add debug notices for SSR May 22, 2024
*/
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 ) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Seems like a bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then I'll try to fix it! 🚀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using numbers only triggers the same error 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

*/
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 ) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep

Copy link

@DAreRodz DAreRodz left a comment

Choose a reason for hiding this comment

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

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

Comment on lines 275 to 276
* @since 6.6.0 The function now adds a warning when the HTML contains unbalanced
* tags or SVG/Math with directives.

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in a9f15d8

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. */

Choose a reason for hiding this comment

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

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. */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in a9f15d8

Comment on lines 701 to 713
<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' ) );
}
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.

@michalczaplinski
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link

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
Copy link
Contributor Author

cbravobernal commented May 31, 2024

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
Copy link
Contributor Author

Now we differentiate between SVG and MATH tags and their inner tags with slightly different messages.
All tests has been updated.
b15ad5f

}
if ( $this->get_tag() === $tag_name ) {
if ( $this->has_self_closing_flag() ) {
continue;
Copy link
Member

Choose a reason for hiding this comment

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

These whitespaces look wrong. It needs to be double checked before committing.

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. */
Copy link
Member

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.

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 );
Copy link
Member

Choose a reason for hiding this comment

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

This message isn't translated but contains a hint for translators.

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. */
Copy link
Member

Choose a reason for hiding this comment

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

There are two tokens, so the hints for translators need some improvements.

@@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @since 6.6.0 The function displays a warning message when the HTML contains unbalanced tags.
* @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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 3e64ebb

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. */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/* translators: %1s: Namespace processed , %2s: The tag that caused the error; could be any HTML tag. */
/* translators: %1s: Namespace processed, %2s: The tag that caused the error; could be any HTML tag. */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 3e64ebb

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 );
Copy link
Member

@westonruter westonruter Jun 3, 2024

Choose a reason for hiding this comment

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

Suggested change
$message = sprintf( __( 'Namespace or reference path cannot be empty. Directive value referenced: %s ' ), $directive_value );
$message = sprintf( __( 'Namespace or reference path cannot be empty. Directive value referenced: %s.' ), $directive_value );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 3e64ebb

@gziolo
Copy link
Member

gziolo commented Jun 4, 2024

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

@gziolo gziolo closed this Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants