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

WIP: Introduce directive processor #158

Closed
wants to merge 6 commits into from

Conversation

dmsnell
Copy link
Member

@dmsnell dmsnell commented Feb 21, 2023

The directive processor builds on the HTML Tag Processor to provide additional functionality, assuming normative markup generated from React, assuming that all tag directives contain a balanced closer.

This will break in practice, and is probably entirely wrong as-is. The purpose of this commit is to introduce the idea and explore using the code to get a feel for whether it will work.

@dmsnell dmsnell requested review from ockham and adamziel February 21, 2023 22:04
return false;
}

public function get_inner_html() {

Choose a reason for hiding this comment

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

It's only get_inner_html if we assume normative markup. I'd suggest this:

Suggested change
public function get_inner_html() {
public function get_balanced_tags_html() {

Alternatively, we could return false in next_balanced_closer if it detects any misnesting.

Choose a reason for hiding this comment

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

Also, the current tag is a void element.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks @adamziel - while not in the comments (but found in the commit message), because I was tossing this together quite quickly, I was taking this file to assume certain things:

assuming normative markup generated from React, assuming that all tag directives contain a balanced closer.

so my idea was simply to write it as if that were true and we can treat deviations as bugs. I don't think any of the directives are self-closing foreign elements, though if they are we can add that support.

this is more or less my exploration of a client API where I consider strictness needs to be lower overall. e.g. if we're looking for the inner HTML of a wp-context then it won't matter if we have void tags or unbalanced tags. everything inside of the directives is blank since we're jumping straight to the next wp-context tag.

Choose a reason for hiding this comment

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

If the markup is supposed to be normative then there should be no harm in baling out in case misnesting is detected?

Copy link

@adamziel adamziel Feb 22, 2023

Choose a reason for hiding this comment

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

But then again, assuming everything inside a directive is skippable like text means there’s no need to do these checks

Copy link
Member Author

Choose a reason for hiding this comment

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

we can't assume the contents are Text/Data. in fact, we expect them to be HTML, which means we still need to scan them to avoid the conflation of things like <wp-show><textarea>Use <wp-show when="…"> to conditionally show things</textarea></wp-show>

return substr( $this->html, $start, $end - $start );
}

public function set_inner_html( $new_html ) {

Choose a reason for hiding this comment

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

Ditto

Suggested change
public function set_inner_html( $new_html ) {
public function set_balanced_tags_html( $new_html ) {

$this->release_bookmark( 'start' );
return false;
}
$this->set_bookmark( 'end' );

Choose a reason for hiding this comment

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

Bookmarking tag closers is affected by a bug, here's a fix:

WordPress/wordpress-develop#4115

Copy link
Member Author

Choose a reason for hiding this comment

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

excellent catch and excellent fix! I didn't test any of the code I wrote here, or even check if it's statically correct.

@adamziel
Copy link

This will break in practice, and is probably entirely wrong as-is

@dmsnell I left a few comments but I don't see anything fundamentally wrong here. The first draft is strong.

The directive processor builds on the HTML Tag Processor to provide additional functionality, assuming
normative markup generated from React, assuming that all tag directives contain a balanced closer.

This will break in practice, and is probably entirely wrong as-is. The purpose of this commit is to
introduce the idea and explore using the code to get a feel for whether it will work.
@ockham ockham force-pushed the add/directive-processor branch from 4c71acf to bd36851 Compare March 2, 2023 10:23

$tag_name = $this->get_tag();
while ( $this->next_tag( array( 'tag_name' => $tag_name, 'tag_closers' => 'visit' ) ) ) {
if ( ! $this->is_tag_closer() ) {
Copy link

Choose a reason for hiding this comment

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

This needs to check for void tags

Copy link
Member Author

Choose a reason for hiding this comment

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

thank you. this was not intended to be a correct working implementation so there are definitely more problems with it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Carrying over my comment from #169

Note that since we're only visiting tags with the same name as the tag we're starting from, I think this case needs to be handled by bailing before even starting that loop. I.e. if we start at a <br>, the concept of a "matching closing tag" doesn't make sense, so we bail right away. If OTOH we start at a <div>, then we'll be only visiting other <div>s (and </div>s), so we don't need to check if we're encountering a void element.

}

public function get_inner_html() {
$this->set_bookmark( 'start' );
Copy link

Choose a reason for hiding this comment

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

set_bookmark may fail and that name may already be taken. I had the same issue in the HTML Processor. We need some way to set internal bookmarks in a different namespace.

Copy link
Member Author

Choose a reason for hiding this comment

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

correct, though this was not intended to be a correct working implementation. I wanted to keep things terse to explore the idea and not distract the main functionality with all the details

public function get_inner_html() {
$this->set_bookmark( 'start' );
if ( ! $this->next_balanced_closer() ) {
$this->release_bookmark( 'start' );
Copy link

Choose a reason for hiding this comment

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

How about wrapping this entire function in try { } finally { $this->release_bookmark('start'); } ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not very keen on using error-handling constructs like this as code conveniences. there's an obvious question it raises, "why is the try there?"

Copy link

Choose a reason for hiding this comment

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

because finally { } wouldn't work on its own 🤣

continue;
}

if ( 0 === $depth ) {
Copy link

Choose a reason for hiding this comment

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

Return false or throw if the tag name doesn't match?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that we're currently limiting the loop to only visit tags whose name does match.

return false;
}

public function get_inner_html() {
Copy link

Choose a reason for hiding this comment

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

set_outer_html will be challenging as we'll have to seek to an index that we'll also delete. In the HTML processor I ended up inventing "pinned" bookmarks that don't get updated or released when the lexical updates are applied.

Copy link

Choose a reason for hiding this comment

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

Actually scratch that, I tried seek()ing back to the original tag. That's not what you do here – I like it. Moving to the next tag makes things easier – I took a stab in 51bf340.

@ockham
Copy link
Collaborator

ockham commented Mar 2, 2023

I was trying to rebase #141 on this one and noticed it wasn't quite working for me. I added a bunch of unit test coverage, mostly taken from previous GB PRs that were going to implement set_content_between_balanced_tags to track down the issue.

The reason seems to be that we were missing some stuff in the GB compat layer (WordPress/gutenberg#48681 and WordPress/gutenberg#48692). With those PRs now merged and cherry-picked, the issue should be fixed by the release on GB 15.3 which I think should happen in the next couple of days.

@dmsnell
Copy link
Member Author

dmsnell commented Mar 3, 2023

I'm stepping away from this PR, kinda regret posting the code now. it was supposed to be a kind of playground to explore interface ideas and now it seems more coupled to running code. I'd give it away to one of you but I don't think I can, so maybe I'll just unsubscribe. please feel free to create a new PR using any of the commits as a new branch tip. let me know if you no longer rely on this and we can close it.

@ockham
Copy link
Collaborator

ockham commented Mar 20, 2023

let me know if you no longer rely on this and we can close it.

I'll go ahead and close this @dmsnell now that #169 has been merged -- hope that's okay! 😊

@ockham ockham closed this Mar 20, 2023
@ockham ockham deleted the add/directive-processor branch March 20, 2023 14:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants