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

Conversation

@SantosGuillamot
Copy link
Contributor

I was testing the recently added wp-text directive in SSR, and it doesn't seem to work in my case. I'm facing two different issues:

  • Getting Uncaught Error: Call to undefined method WP_HTML_Tag_Processor::set_inner_html().
  • With JavaScript disabled, this is the HTML I'm getting:

Screenshot 2023-03-21 at 10 50 20

In order to solve that, I made a couple of commits, but I am not sure if it is the best way to solve it:

Please, let me know if this is correct and feel free to make any changes needed 🙂

@SantosGuillamot SantosGuillamot requested a review from ockham March 21, 2023 09:57
@SantosGuillamot
Copy link
Contributor Author

Tests are failing, so it's probably not the correct solution 😄 Taking a look at it.

@ockham
Copy link
Collaborator

ockham commented Mar 21, 2023

In the process_directives_in_block function, I used the WP_Directive_Processor class instead of WP_HTML_Tag_Processor, because is the one containing the set_inner_html function.

That seems like the correct approach 🤔

I'll take a deeper look in a bit!

@SantosGuillamot
Copy link
Contributor Author

I believe the one breaking the tests is the other commit:

  • I changed the end bookmark of the set_inner_html function to move one character to the left.

);

$tags = new WP_HTML_Tag_Processor( $block_content );
$tags = new WP_Directive_Processor( $block_content );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry I missed this!

(wp-directives.php is the one file that doesn't have unit test coverage, as it's kind of instantiating everything, so it'd be more of a case for integration testing.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem at all 🙂 We will need integration testing at some point that should handle this.

@SantosGuillamot
Copy link
Contributor Author

Tests are passing now 🙂 Do you believe it is ready to be merged @ockham ? Or is there anything missing?

Copy link
Collaborator

@ockham ockham left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you, Mario! :shipit:

@SantosGuillamot SantosGuillamot merged commit 34f7669 into main-wp-directives-plugin Mar 21, 2023
@SantosGuillamot SantosGuillamot deleted the fix/wp-text-ssr branch March 21, 2023 11:19
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