Interactivity API: Debug - Warning about Server Directives Processing errors.

[cbravobernal]

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

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

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

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

Draft PR. Is still work in progress.

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

Done in ​https://github.com/WordPress/wordpress-develop/pull/6413/commits/4b78690b19728bab8cf9b69c6eca0178bb0c3987

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.

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

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

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?

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

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.

Now we differentiate between SVG and MATH tags and their inner tags with slightly different messages.
All tests has been updated.
​https://github.com/WordPress/wordpress-develop/pull/6413/commits/b15ad5f3cb9f6ac1f8249d50908d675cb9673204

[gziolo]

In 58321:

Interactivity API: Print debug warning when server directives processing encounters errors

Aims to improve the developer experience of the Interactivity API server directives processing.

Props cbravobernal, jonsurrell, westonruter, darerodz, czapla, gziolo.
Fixes #61044.

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