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

SSR: Add wp-html and wp-text attribute directive processors #170

Merged
merged 5 commits into from
Mar 20, 2023

Conversation

ockham
Copy link
Collaborator

@ockham ockham commented Mar 6, 2023

Based on #169.

Per my understanding (and the discussion below), their only difference is that wp-html can contain HTML that is used unescaped, whereas wp-text escapes its value before rendering it.

@ockham ockham self-assigned this Mar 6, 2023
@ockham ockham changed the title SSR: Add wp-show attribute directive processor SSR: Add wp-text attribute directive processor Mar 6, 2023
.wp-env.json Outdated Show resolved Hide resolved
@ockham
Copy link
Collaborator Author

ockham commented Mar 6, 2023

@luisherranz Looking at #118 (comment), I'm wondering how (if?) wp-text and wp-html need to differ in terms of SSR.

One thing that comes to mind is escaping. Should wp-html run esc_html on the value it is passed? Are there any other differences? 🤔

@ockham ockham force-pushed the add/wp-text-ssr branch from 88fed8b to d29eefa Compare March 6, 2023 16:36
@luisherranz
Copy link
Member

luisherranz commented Mar 6, 2023

You can't escape the HTML of wp-html. This needs to work:

<?php
wp_store( array( 
  'state' => array(
    'myPlugin' => array(
      'html' => get_some_raw_html_from_somewhere(),
    ),
  ),
) );
?>

<div data-wp-html="state.myPlugin.html">Default content</div>

I am not an expert on the matter, but wp-text should be the safe one and wp-html the unsafe one. We are basically using dangerouslySetInnerHTML underneath in the client.

By the way, should we be more explicit about the risks of wp-html? wp-dangerous-html? 😄

EDIT: I've added that question to the list of decisions that need to be made of the Tracking Issue.

@ockham
Copy link
Collaborator Author

ockham commented Mar 6, 2023

I am not an expert on the matter, but wp-text should be the safe one and wp-html the unsafe one. We are basically using dangerouslySetInnerHTML underneath in the client.

Ah, makes sense! 😅 So I guess I'll add escaping to wp-text then 🤔

By the way, should we be more explicit about the risks of wp-html? wp-dangerous-html? 😄

Maybe 🤔 Close enough to React, I guess.

@ockham ockham force-pushed the add/directive-processor-and-unit-tests branch from 9f0c237 to bbc790f Compare March 7, 2023 08:28
@ockham ockham force-pushed the add/wp-text-ssr branch from d29eefa to fb1420a Compare March 7, 2023 08:30
@ockham ockham force-pushed the add/wp-text-ssr branch from fb1420a to 120a1fa Compare March 9, 2023 08:40
@ockham ockham force-pushed the add/directive-processor-and-unit-tests branch from 5457d8b to 054c579 Compare March 9, 2023 12:36
@ockham ockham force-pushed the add/wp-text-ssr branch 2 times, most recently from 5da7778 to ecf23f4 Compare March 14, 2023 14:03
@ockham ockham changed the title SSR: Add wp-text attribute directive processor SSR: Add wp-html and wp-text attribute directive processors Mar 14, 2023
Base automatically changed from add/directive-processor-and-unit-tests to main-wp-directives-plugin March 15, 2023 14:18
@ockham
Copy link
Collaborator Author

ockham commented Mar 15, 2023

Opening this for review, now that #169 is in 😊

@ockham ockham marked this pull request as ready for review March 15, 2023 14:48
@ockham ockham requested a review from a team March 15, 2023 14:48
Copy link
Collaborator

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

Looks great to me! ✅

Just adding one missing require_once, and it's ready to go. 😄

Oh, nevermind, I didn't notice it was already addressed.

wp-directives.php Show resolved Hide resolved
@ockham
Copy link
Collaborator Author

ockham commented Mar 20, 2023

Thank you @DAreRodz!

I'll rebase to change the prefix to data-wp- 😊

@ockham ockham merged commit ff2a0d6 into main-wp-directives-plugin Mar 20, 2023
@ockham ockham deleted the add/wp-text-ssr branch March 20, 2023 13:17
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