Skip to content
This repository has been archived by the owner on Jul 28, 2023. It is now read-only.

SSR: Add wp-show attribute directive processor #141

Closed
wants to merge 38 commits into from
Closed

SSR: Add wp-show attribute directive processor #141

Show file tree
Hide file tree
Changes from 37 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
446ca57
SSR: Add wp-show attribute directive processor
ockham Mar 2, 2023
8a66bac
Simplify implementation of wp-show
ockham Mar 9, 2023
bc4adf3
Implement basic wrap_in_tag method()
ockham Mar 15, 2023
4e288c8
Format wp-show
ockham Mar 15, 2023
86d5d4a
Use wrap_in_tag to implement wp-show
ockham Mar 15, 2023
bb9b748
Add data- prefix
ockham Mar 20, 2023
929a6f2
Format
ockham Mar 20, 2023
fd977a1
Add PHPDoc
ockham Mar 20, 2023
5b63332
wrap_in_tag: Cover void tags
ockham Mar 20, 2023
6f281f9
wp-show: Add test to cover void tags
ockham Mar 20, 2023
d5ade51
Remove now-obsolete TODO comments
ockham Mar 20, 2023
f8e005c
Add test to demonstrate issue
ockham Mar 21, 2023
f4fa160
Return to original position after replacing
ockham Mar 21, 2023
cef691c
wrap_in_tag: Bail early if we're at a tag closer
ockham Mar 21, 2023
9eae803
Simply insert start and end tag in correct positions
ockham Mar 21, 2023
ba75900
Document that the pointer keeps pointing at the same tag
ockham Mar 21, 2023
7eb3777
Simplify unit test
ockham Mar 21, 2023
665b4cb
Move attribute to wrapping tag
ockham Mar 21, 2023
f1eee33
Fail if trying to wrap in void tag
ockham Mar 21, 2023
d2e68d3
Increase test coverage -- new test currently fails
ockham Apr 13, 2023
059fe61
Don't process directives in inner blocks
ockham Apr 13, 2023
87f0532
Add test case to cover issue
ockham Apr 13, 2023
042501d
Fix test
ockham Apr 13, 2023
e3d1743
More minimal
ockham Apr 13, 2023
229c304
Add another failing test case
ockham Apr 13, 2023
e864093
We might not want to limit to interactive blocks only
ockham Apr 17, 2023
81a9846
Revert "We might not want to limit to interactive blocks only"
ockham Apr 18, 2023
d9e5271
Revert "Don't process directives in inner blocks"
ockham Apr 18, 2023
e431beb
Fix test
ockham Apr 18, 2023
39ccb18
Rewrite wp-show processor
ockham Apr 25, 2023
40ee90d
Blunt but effective
ockham Apr 25, 2023
4d88309
Remove now-obsolete unit tests
ockham Apr 25, 2023
2a70577
PHPDoc
ockham Apr 25, 2023
9430be8
Flush before moving to next tag
ockham Apr 17, 2023
1dc96a2
Add unit test for nested directives
ockham May 4, 2023
f1a473a
Fix test string
ockham May 4, 2023
e892856
Update tests accordingly
ockham May 4, 2023
9c95d2d
Revert "Flush before moving to next tag"
ockham Jun 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
176 changes: 176 additions & 0 deletions phpunit/directives/attributes/wp-show.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,176 @@
<?php
/**
* data-wp-show tag directive test.
*/

require_once __DIR__ . '/../../../src/directives/attributes/wp-show.php';

require_once __DIR__ . '/../../../src/directives/class-wp-directive-context.php';
require_once __DIR__ . '/../../../src/directives/class-wp-directive-processor.php';

require_once __DIR__ . '/../../../src/directives/wp-html.php';

/**
* Tests for the wp-show tag directive.
*
* @group directives
* @covers process_wp_show
*/
class Tests_Directives_WpShow extends WP_UnitTestCase {
public function test_directive_leaves_content_unchanged_if_when_is_true() {
$markup = '<div data-wp-show="context.myblock.open">I should be shown!</div>';
$tags = new WP_Directive_Processor( $markup );
$tags->next_tag();

$context_before = new WP_Directive_Context( array( 'myblock' => array( 'open' => true ) ) );
$context = clone $context_before;
process_wp_show( $tags, $context );

$tags->next_tag( array( 'tag_closers' => 'visit' ) );
process_wp_show( $tags, $context );

$this->assertSame( $markup, $tags->get_updated_html() );
$this->assertSame( $context_before->get_context(), $context->get_context(), 'data-wp-show directive changed context' );
}

public function test_directive_wraps_content_in_template_if_when_is_false() {
$markup = '<div data-wp-show="context.myblock.open">I should not be shown!</div>';

$tags = new WP_Directive_Processor( $markup );
$tags->next_tag();

$context_before = new WP_Directive_Context( array( 'myblock' => array( 'open' => false ) ) );
$context = clone $context_before;
process_wp_show( $tags, $context );

$tags->next_tag( array( 'tag_closers' => 'visit' ) );
process_wp_show( $tags, $context );

$updated_markup = '<template data-wp-show="context.myblock.open"><div >I should not be shown!</div></template>';

$this->assertSame( $updated_markup, $tags->get_updated_html() );
$this->assertSame( $context_before->get_context(), $context->get_context(), 'data-wp-show directive changed context' );
}

public function test_directive_does_not_wrap_template_in_template() {
$markup = '<template data-wp-show="context.myblock.open">I should not be shown!</template>';

$tags = new WP_Directive_Processor( $markup );
$tags->next_tag();

$context_before = new WP_Directive_Context( array( 'myblock' => array( 'open' => false ) ) );
$context = clone $context_before;
process_wp_show( $tags, $context );

$tags->next_tag( array( 'tag_closers' => 'visit' ) );
process_wp_show( $tags, $context );

$this->assertSame( $markup, $tags->get_updated_html() );
$this->assertSame( $context_before->get_context(), $context->get_context(), 'data-wp-show directive changed context' );
}

public function test_directive_wraps_content_preceded_by_other_html_in_template_if_when_is_false() {
$markup = '<p>Some text</p><div data-wp-show="context.myblock.open">I should not be shown!</div>';

$tags = new WP_Directive_Processor( $markup );
$tags->next_tag( 'div' );

$context_before = new WP_Directive_Context( array( 'myblock' => array( 'open' => false ) ) );
$context = clone $context_before;
process_wp_show( $tags, $context );

$tags->next_tag( array( 'tag_closers' => 'visit' ) );
process_wp_show( $tags, $context );

$updated_markup = '<p>Some text</p><template data-wp-show="context.myblock.open"><div >I should not be shown!</div></template>';

$this->assertSame( $updated_markup, $tags->get_updated_html() );
$this->assertSame( $context_before->get_context(), $context->get_context(), 'data-wp-show directive changed context' );
}

public function test_directive_wraps_void_tag_in_template_if_when_is_false() {
$markup = '<img data-wp-show="context.myblock.open">';
$tags = new WP_Directive_Processor( $markup );
$tags->next_tag();

$context_before = new WP_Directive_Context( array( 'myblock' => array( 'open' => false ) ) );
$context = clone $context_before;
process_wp_show( $tags, $context );

$tags->next_tag( array( 'tag_closers' => 'visit' ) );
process_wp_show( $tags, $context );

$updated_markup = '<template data-wp-show="context.myblock.open"><img ></template>';

$this->assertSame( $updated_markup, $tags->get_updated_html() );
$this->assertSame( $context_before->get_context(), $context->get_context(), 'data-wp-show directive changed context' );
}

public function test_nested_directives_within_wp_show_with_truthy_value() {
$markup = <<<END
<div data-wp-show="context.myBlock.open">
I should be shown!
<div data-wp-show="context.myOtherBlock.open">I should be shown!</div>
<div data-wp-show="!context.myOtherBlock.open">I should not be shown!</div>
</div>
END;

$context_before = new WP_Directive_Context(
array(
'myBlock' => array( 'open' => true ),
'myOtherBlock' => array( 'open' => true ),
)
);
$context = clone $context_before;

$tags = new WP_Directive_Processor( $markup );

while ( $tags->next_tag( array( 'tag_closers' => 'visit' ) ) ) {
process_wp_show( $tags, $context );
}

$updated_markup = <<<END
<div data-wp-show="context.myBlock.open">
I should be shown!
<div data-wp-show="context.myOtherBlock.open">I should be shown!</div>
<template data-wp-show="!context.myOtherBlock.open"><div >I should not be shown!</div></template>
</div>
END;
$this->assertSame( $updated_markup, $tags->get_updated_html() );
$this->assertSame( $context_before->get_context(), $context->get_context(), 'data-wp-show directive changed context' );
}

public function test_nested_directives_within_wp_show_with_falsy_value() {
$markup = <<<END
<div data-wp-show="context.myBlock.open">
I should not be shown!
<div data-wp-show="context.myOtherBlock.open">I should be shown!</div>
<div data-wp-show="!context.myOtherBlock.open">I should not be shown!</div>
</div>
END;

$context_before = new WP_Directive_Context(
array(
'myBlock' => array( 'open' => false ),
'myOtherBlock' => array( 'open' => true ),
)
);
$context = clone $context_before;

$tags = new WP_Directive_Processor( $markup );

while ( $tags->next_tag( array( 'tag_closers' => 'visit' ) ) ) {
process_wp_show( $tags, $context );
}

$updated_markup = <<<END
<template data-wp-show="context.myBlock.open"><div >
I should not be shown!
<div data-wp-show="context.myOtherBlock.open">I should be shown!</div>
<template data-wp-show="!context.myOtherBlock.open"><div >I should not be shown!</div></template>
Copy link
Member

Choose a reason for hiding this comment

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

Should it convert div to template inside another template? That was the reason I wanted to see those tests, so they cover edge cases like that.

</div></template>
END;
$this->assertSame( $updated_markup, $tags->get_updated_html() );
$this->assertSame( $context_before->get_context(), $context->get_context(), 'data-wp-show directive changed context' );
}
}
39 changes: 39 additions & 0 deletions phpunit/directives/wp-directive-processor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,4 +125,43 @@ public function test_set_inner_html_invalidates_bookmarks_that_point_to_replaced
$successful_seek = $tags->seek( 'replaced' );
$this->assertFalse( $successful_seek );
}

public function test_wrap_in_tag_wraps_balanced_tag_correctly() {
$tags = new WP_Directive_Processor( self::HTML );

$tags->next_tag( 'section' );
$tags->wrap_in_tag( 'TEMPLATE' );
$this->assertSame( '<div>outside</div><template><section><div><img>inside</div></section></template>', $tags->get_updated_html() );
$this->assertSame( 'SECTION', $tags->get_tag() );
$this->assertFalse( $tags->is_tag_closer() );
}

public function test_wrap_in_tag_wraps_void_tag_correctly() {
$tags = new WP_Directive_Processor( self::HTML );

$tags->next_tag( 'img' );
$tags->wrap_in_tag( 'TEMPLATE' );
$this->assertSame( '<div>outside</div><section><div><template><img></template>inside</div></section>', $tags->get_updated_html() );
$this->assertSame( 'IMG', $tags->get_tag() );
}

public function test_wrap_in_tag_followed_by_two_subsequent_calls_of_get_updated_html_works_correctly() {
$tags = new WP_Directive_Processor( self::HTML );

$tags->next_tag( 'img' );
$tags->wrap_in_tag( 'TEMPLATE' );
$tags->get_updated_html();
$this->assertSame( '<div>outside</div><section><div><template><img></template>inside</div></section>', $tags->get_updated_html() );
$this->assertSame( 'IMG', $tags->get_tag() );
}

public function test_wrap_in_tag_fails_if_passed_void_tag() {
$tags = new WP_Directive_Processor( self::HTML );

$tags->next_tag( 'section' );

$this->assertFalse( $tags->wrap_in_tag( 'IMG' ) );
$this->assertSame( self::HTML, $tags->get_updated_html() );
$this->assertSame( 'SECTION', $tags->get_tag() );
}
}
41 changes: 41 additions & 0 deletions src/directives/attributes/wp-show.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
<?php
/**
* Process wp-show directive attribute.
*
* @package wp-directives
*/

/** Utility functions */
require_once __DIR__ . '/../utils.php';

/**
* Process wp-show directive attribute.
*
* @param WP_Directive_Processor $tags Tags.
* @param WP_Directive_Context $context Directive context.
*/
function process_wp_show( $tags, $context ) {
if ( $tags->is_tag_closer() ) {
return;
}

$value = $tags->get_attribute( 'data-wp-show' );
if ( null === $value ) {
return;
}

$show = evaluate( $value, $context->get_context() );
if ( $show ) {
return;
}

if ( 'TEMPLATE' === $tags->get_tag() ) {
return; // Don't wrap a `<template>` in a `<template>`.
}

$wrapper_bookmark = $tags->wrap_in_tag( 'TEMPLATE' );
$tags->seek( $wrapper_bookmark );
$tags->set_attribute( 'data-wp-show', $value );
$tags->next_tag();
$tags->remove_attribute( 'data-wp-show' );
}
78 changes: 78 additions & 0 deletions src/directives/class-wp-directive-processor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,84 @@ public function get_balanced_tag_bookmarks() {
return array( $start_name, $end_name );
}

/**
* Wrap the current node in a given tag.
*
* When positioned on a tag opener, locate its matching closer, and wrap everything
* in the tag specified as an argument. When positioned on a void element, wrap that
* element in the argument tag.
*
* Note that the internal pointer will continue to point to the same tag as before
* calling the function.
*
* @param string $tag An HTML tag, specified in uppercase (e.g. "DIV").
* @return string|false The name of a bookmark pointing to the wrapping tag opener
* if successful; false otherwise.
*
* @todo Allow passing in tags with attributes, e.g. <template id="abc">?
Comment on lines +144 to +148
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that we're only accepting an uppercase HTML tag such as DIV or TEMPLATE as the $tag argument. We don't accept it wrapped in angle brackets, or with attributes (<template attr="value>), as that would require us to apply HTML parsing to the $tag argument itself -- and would allow for many more edge cases for which we'd need to decide how to behave (E.g. what if we pass two tags (<template><div>) -- do we wrap in both? What if one of them is void (<template><img>)? What if there's a basic syntax error (<template)?)

We can eschew most of these questions by only allowing a tag name; handling of attributes can be done after the fact and separately in process_wp_show.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we could call the argument $tag_name to communicate that better? also I think we discussed this, but the part about "in uppercase" shouldn't matter, right?

*/
public function wrap_in_tag( $tag ) {
if ( $this->is_tag_closer() ) {
return false;
}

if ( self::is_html_void_element( $tag ) ) {
// _doing_it_wrong(
// __METHOD__,
// __( 'Cannot wrap HTML in void tag.' ),
// '6.3.0'
// );
return false;
}

$this->get_updated_html(); // Apply potential previous updates.
Copy link
Contributor

@dmsnell dmsnell Apr 19, 2023 •
edited

Choose a reason for hiding this comment

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

After applying the fix in dmsnell/wordpress-develop#8 WordPress/wordpress-develop#4345 and removing this call to get_updated_html() my test case, where I reproduced at least some of the errors we found in this PR, succeeded.

Copy link
Contributor

Choose a reason for hiding this comment

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

And this is the test I used to reproduce…

public function blahblahblah() {
	$p = new WP_Directive_Processor( '<div><img hidden></div>' );

	$p->next_tag();
	$p->next_tag('img');
	$p->set_attribute( 'inert', true );
	$p->remove_attribute( 'hidden' );
	$b = $p->wrap_in_tag( 'figure' );
	$this->assertSame( 'IMG', $p->get_tag() );
	$p->seek( $b );
	$this->assertSame( 'FIGURE', $p->get_tag() );
	$p->next_tag();
	$this->assertSame( 'IMG', $p->get_tag() );

	$this->assertSame( '<div><figure><img inert ></figure></div>', $p->get_updated_html() );
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you!

I just tested your patch (plus the removal of that get_updated_html() call inside wrap_in_tag(), and my changes from #215) against the Movies Demo. Unfortunately, the issue I found there still persists 😕

On a sidenote -- it strikes me as odd that we need to remove a get_updated_html() call. Since I've conceptually thought of it as a flushing method, I was silently assuming that there was no harm in calling it. Am I mistaken here? Or is there still something wrong with it that we need to fix?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, the tests that are currently failing on this branch are still failing with your patch applied and that get_updated_html call removed 😕

To verify that your patch was properly picked up, I copied the test from WordPress/wordpress-develop#4345 into phpunit/directives/wp-directive-processor.php and ran unit tests. That added test passed (which it didn't when I un-applied your patch).

Copy link
Contributor

Choose a reason for hiding this comment

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

it strikes me as odd that we need to remove a get_updated_html() call

yeah I've been working on figuring this out better

still failing with your patch applied

this is why I had tried to carefully word that my patch fixed my tests as I suspected we could still have more in this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For the record, I've filed a PR against wordpress-develop based on this one to make it a bit easier to reproduce and test fixes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Noting that @dmsnell managed to reduce that test case to be more minimal and has filed a PR with a tentative fix: WordPress/wordpress-develop#4371

Copy link
Contributor

Choose a reason for hiding this comment

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

and it's fixed now!


if ( self::is_html_void_element( $this->get_tag() ) ) {
// We don't have direct access to the start and end position of the
// current tag. As a workaround, we set a bookmark that we then
// release immediately.
$i = 0;
while ( array_key_exists( 'void' . $i, $this->bookmarks ) ) {
++$i;
}
$start_name = 'void' . $i;

$this->set_bookmark( $start_name );

$start = $this->bookmarks[ $start_name ]->start;
$end = $this->bookmarks[ $start_name ]->end + 1;
} else {
$bookmarks = $this->get_balanced_tag_bookmarks();
if ( ! $bookmarks ) {
return false;
}
list( $start_name, $end_name ) = $bookmarks;

$start = $this->bookmarks[ $start_name ]->start;
$end = $this->bookmarks[ $end_name ]->end + 1;

$this->release_bookmark( $end_name );
}

$tag = strtolower( $tag );
$this->lexical_updates[] = new WP_HTML_Text_Replacement( $start, $start, "<$tag>" );
$this->lexical_updates[] = new WP_HTML_Text_Replacement( $end, $end, "</$tag>" );

$this->seek( $start_name ); // Return to original position.
$this->release_bookmark( $start_name );

$i = 0;
while ( array_key_exists( $tag . $i, $this->bookmarks ) ) {
++$i;
}
$bookmark_name = $tag . $i;
$this->bookmarks[ $bookmark_name ] = new WP_HTML_Span(
$start,
$start + strlen( $tag ) + 2
);
return $bookmark_name;
}

/**
* Whether a given HTML element is void (e.g. <br>).
*
Expand Down
1 change: 1 addition & 0 deletions src/directives/wp-process-directives.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ function wp_process_directives( $tags, $prefix, $directives ) {
foreach ( $attributes as $attribute ) {
call_user_func( $directives[ $attribute ], $tags, $context );
}
$tags->get_updated_html();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since WP 6.2.1 -- which includes this fix -- has now been released, we might be able to remove this line now (i.e. revert 42ab641).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems to be working: 9c95d2d

}

return $tags;
Expand Down
2 changes: 2 additions & 0 deletions wp-directives.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ function () {
require_once __DIR__ . '/src/directives/attributes/wp-context.php';
require_once __DIR__ . '/src/directives/attributes/wp-class.php';
require_once __DIR__ . '/src/directives/attributes/wp-html.php';
require_once __DIR__ . '/src/directives/attributes/wp-show.php';
require_once __DIR__ . '/src/directives/attributes/wp-style.php';
require_once __DIR__ . '/src/directives/attributes/wp-text.php';

Expand Down Expand Up @@ -261,6 +262,7 @@ function process_directives_in_block( $block_content ) {
'data-wp-bind' => 'process_wp_bind',
'data-wp-class' => 'process_wp_class',
'data-wp-html' => 'process_wp_html',
'data-wp-show' => 'process_wp_show',
'data-wp-style' => 'process_wp_style',
'data-wp-text' => 'process_wp_text',
);
Expand Down