Skip to content

Conversation

@dmsnell
Copy link
Member

@dmsnell dmsnell commented Feb 13, 2023

Status

I don't like this anymore and have replaced any desire to introduce it with some form of an HTML template built from the Tag Processor. I've included a quick demo in a comment below

WP_HTML::template(
	'<a href="</%url>"></%title></a>',
	array( 'url' => $url, 'title' => 'Wonders beyond your dreams' )
);

Been wanting to look at adding a helper like this for the common need of creating new markup.

E.g.

Before

if ( current_user_can( 'create_users' ) ) :
	?>
	<a href="<?php echo esc_url( network_admin_url( 'user-new.php' ) ); ?>" class="page-title-action"><?php echo esc_html_x( 'Add New', 'user' ); ?></a>
	<?php
endif;

After

if ( current_user_can( 'create_users' ) ) {
	echo WP_HTML::make_tag(
		'a',
		array(
			'href'  => network_admin_url( 'user-new.php' ),
			'class' => 'page-title-action'
		),
		esc_html_x( 'Add New', 'user' )
	);
}

This is a good place to examine the needs in the data markup. If this takes HTML strings as the inner markup, then we should leave escaping to the caller, because we don't want to escape <strong> into &lt;strong>, but if we leave it up to the caller then people will forget to escape it.

Before

<form method="post" action="<?php echo $referer ? esc_url( $referer ) : ''; ?>" style="display:inline;">
	<?php submit_button( __( 'No, return me to the theme list' ), '', 'submit', false ); ?>
</form>

After

echo WP_HTML::make_tag(
	'form',
	array(
		'method' => 'post',
		'action' => $referer ?: false,
		'style'  => 'display: inline;'
	),
	submit_button( __( 'No, return me to the theme list' ), '', 'submit', false )
);

Before

<<?php echo $safe_type; ?> controls="controls" preload="none" width="<?php echo (int) $theme_width; ?>"
	<?php
	if ( 'video' === $safe_type ) {
		echo ' height="', (int) $theme_height, '"';
	}
	?>
></<?php echo $safe_type; ?>>

After

echo WP_HTML::make_tag(
	$safe_type,
	array(
		'controls' => true,
		'preload'  => 'none',
		'width'    => (int) $theme_width,
		'height'   => 'video' === $safe_type ? (int) $theme_height : false
	)
);

cc: @adamziel @ockham @gziolo @azaozz

@adamziel
Copy link
Contributor

@dmsnell This is related: https://core.trac.wordpress.org/ticket/50867

@@ -0,0 +1,28 @@
<?php

class WP_HTML_Spec {
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

Copy link
Member Author

Choose a reason for hiding this comment

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

this is something I've meant to create in the HTML processor branches I've been working in. the idea is to move utilities which related to HTML-defined terminology into here.

that would be:

  • is_void_element()
  • is_html_element()
  • is_format_element()
  • etc…

is_html_element() will be important for HTML parsing because foreign elements may be self-closing, but HTML elements may not be, and therefore we have to ignore the trailing / in something like an <img /> tag and look for a closing tag if it's not a void element, such as with <div /> (because while it looks like a self-closer it isn't), but <wp-self-closer /> is due to it being a foreign element.

$is_void = WP_HTML_Spec::is_void_element( $tag_name );
$html = $is_void ? "<{$tag_name}>" : "<{$tag_name}>{$data}</{$tag_name}>";

$p = new WP_HTML_Tag_Processor( $html );
Copy link
Contributor

Choose a reason for hiding this comment

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

This surprised me, as I was fully expecting manual stitching of the strings, but I think it makes total sense. Tag Processor handles boolean values, escaping, and probably a few more cases we would otherwise have to reimplement here.

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 considered it and it might make sense to construct it. this is more of an exploration of sharing the tag processor for this utility. I think initially I was looking at returning the processor itself, which would make it possible to modify after creating. the thought of that was for conditional attributes, but here that's already possible by setting an attribute to false

<?php

class WP_HTML {
public static function make_tag( $tag_name, $attributes = null, $data = '' ) {
Copy link
Contributor

@adamziel adamziel Feb 14, 2023

Choose a reason for hiding this comment

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

In a seperate PR I'd love to add make_class_name that would do what classnames() does in React.

Copy link
Member Author

Choose a reason for hiding this comment

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

class WP_CSS {
	public static function make_class( $class_names ) {
		
	}
}

this is another reason it could be useful to expose the processor rather than returning a string. the need to call ->get_updated_html() on it is the downside.

$panel = WP_HTML::make_tag( 'div', array( 'class' => 'wp-block-group' ) );
if ( $is_fullscreen ) {
	$panel->add_class( 'fullscreen' );
}

Copy link
Contributor

@adamziel adamziel Feb 15, 2023

Choose a reason for hiding this comment

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

the need to call ->get_updated_html() on it is the downside.

Not at all, __toString() is still there!

Here's the three alternatives I can think of, but with more class names:

make_class

$class_name = WP_HTML::make_class( 'wp-block-group', [ 
    'fullscreen' => $is_fullscreen,
    'bold' => $is_bold,
    'large' => $is_large
], $is_modal ? 'is-modal' : 'is-regular' );
WP_HTML::make_tag( 'div', [ 'class' => $class_name ] );

Returning a Tag Processor

$panel = WP_HTML::make_tag( 'div', array( 'class' => 'wp-block-group' ) );
if ( $is_fullscreen ) {
	$panel->add_class( 'fullscreen' );
}
if ( $is_bold ) {
	$panel->add_class( 'bold' );
}
if ( $is_large ) {
	$panel->add_class( 'large' );
}
if ( $is_modal) {
	$panel->add_class( 'is-modal' );
} else {
	$panel->add_class( 'is-regular' );
}

Manual wrapping in a tag processor

$panel = new WP_HTML_Tag_Processor(
    WP_HTML::make_tag( 'div' )
);
// ... The same as above

Copy link
Contributor

Choose a reason for hiding this comment

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

Now, let's consider conditional attributes:

Attributes array

$attributes = [
    'src' => $src
    'alt' => $has_alt ? $alt : false,
];
if ( ! $only_aria ) {
    $attributes['aria-title'] = $title;
} else {
    $attributes['title'] = $title;
}
WP_HTML::make_tag( 'div', $attributes );

Returning a Tag Processor

$panel = WP_HTML::make_tag( 'div', array( 'src' => $src ) );
if ( $has_alt ) {
	$panel->set_attribute( 'alt', $alt );
}
if ( $only_aria ) {
	$panel->set_attribute( 'aria-title', $title );
} else {
	$panel->set_attribute( 'title', $title );
}

Manual wrapping in a tag processor

$panel = new WP_HTML_Tag_Processor(
    WP_HTML::make_tag( 'div', array( 'src' => $src ) )
);
// ... The same as above

Copy link
Contributor

@adamziel adamziel Feb 15, 2023

Choose a reason for hiding this comment

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

What stands out:

  • The array approach is more concise and seems to support all possible use-cases
  • The tag processor approach still accepts an array of attributes but then requires using another API to handle conditionals

I'd go for make_class + an array of attributes and let folks create a new tag processor manually if they wish so.

Copy link
Contributor

@adamziel adamziel Feb 15, 2023

Choose a reason for hiding this comment

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

Here's a wild idea:

echo WP_HTML_Builder::create()
  ->add_tag( 'form', [ 'class' => 'main' ] )
  ->add_child( 'input', [ 'type' => 'text', 'placeholder' => 'Your query' ] ) 
  ->add_sibling( 'button', [ 'class' => 'primary' ], 'Submit' )
  ->parent()
  ->add_sibling( 'img', [ 'src' => 'logo.png' ] )
;
/*
<form class="main">
    <input type="text" placeholder="Your query" />
    <button class="primary">Submit</button>
</form>
<img src="logo.png" />
*/

Copy link
Contributor

Choose a reason for hiding this comment

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

This would play nicely with the future HTML Processor:

$p = new WP_HTML_Processor('<h1 title="main">Site title</h1>');
$p->next_tag();
$p->insert_html_after(
    WP_HTML_Builder::create()
      ->add_tag( 'form', [ 'class' => 'main' ] )
      ->add_sibling( 'img', [ 'src' => 'logo.png' ] )
);

@adamziel
Copy link
Contributor

adamziel commented Feb 14, 2023

This is a good place to examine the needs in the data markup. If this takes HTML strings as the inner markup, then we should leave escaping to the caller, because we don't want to escape into <strong>, but if we leave it up to the caller then people will forget to escape it.

Both use cases are valid. I'd go for two separate methods: WP_HTML::make_tag would escape $data, and WP_HTML::make_tag_unsafe would not escape it.

@dmsnell
Copy link
Member Author

dmsnell commented Feb 14, 2023

separate methods: WP_HTML::make_tag would escape $data, and WP_HTML::make_tag_unsafe would not escape it.

this is a good idea. could also be something like WP_HTML::make_tag_with_inner_html

WP_HTML::tag( $tag_name, $attributes, $inner_text )
WP_HTML::tag_with_html( $tag_name, $attributes, $escaped_inner_html )

@luisherranz
Copy link
Member

Just a small note in case it has escaped your attention so far: you should make sure that these statements can be chained together:

echo WP_HTML::make_tag(
    'a',
    array(
      'href'  => '/some-page'
      'class' => 'some-class'
    ),
    WP_HTML::make_tag(
      'span',
      array(
        'class' => 'inner-class'
      ),
      "Navigate to Some Page."
    )
  );

and that the API can accept multiple children:

echo WP_HTML::make_tag(
    'a',
    array(
      'href' => '/some-page'
    ),
    array(
      "Navigate to ",
      WP_HTML::make_tag(
        'strong',
        array(),
        "Some Page"
      ),
      "."
    )
  );

@adamziel
Copy link
Contributor

adamziel commented Feb 15, 2023

@luisherranz I find it easy to make a mistake when composing many multiline function calls. I'd rather go for builder pattern, then:

echo WP_HTML_Builder::create()
  ->add_tag( 'form', [ 'class' => 'main' ] )
  ->add_child( 'input', [ 'type' => 'text', 'placeholder' => 'Your query' ] ) 
  ->add_sibling( 'button', [ 'class' => 'primary' ], 'Submit' )
  ->parent()
  ->add_sibling( 'img', [ 'src' => 'logo.png' ] )
;

I don't like the linear flow and the need to backtrack to parent, though.


Actually, let's talk about moonshots. Is there any way we could support a JSX-like syntax directly in PHP?

$html = (
    <form class="main" action={$url}>
        <input type="text" value={$query} />
        <button class="primary">{__('Submit')}</button>
    </form>
);

On the surface, PHP would throw a syntax error. Well, what if it wasn't a part of PHP syntax then?

$html = make_html(<<<HTMLX
    <form class="main" action={$url}>
        <input type="text" value={$query} />
        <button class="primary">{__('Submit')}</button>
    </form>
HTMLX, get_declared_vars());

Then, the future HTML processor could take care of the relevant substitutions.

Downsides: no syntax highlighting, slight performance penalty, perhaps it would be too easy to omit the closing > without noticing.

@luisherranz
Copy link
Member

I find it easy to make a mistake when composing many multiline function calls I'd rather go for builder pattern

I ask about that because it's one of the best ways to represent an HTML tree with declarative syntax, as demonstrated by JSX, and it could be the target for abstractions. The builder pattern is imperative and therefore more difficult to align with an existing HTML tree because you have to figure out the steps to rebuild that tree instead of just iterating over it.

Anyway, I'm not saying we need one and not the other. I'm just saying that if you go with the initial syntax proposed by Dennis, it'd be nice to support nested tags.

@adamziel
Copy link
Contributor

adamziel commented Feb 15, 2023

The builder pattern is imperative and therefore more difficult to align with an existing HTML tree

Yeah I don't love it either. I wonder what's the approach that connects the best of all worlds 🤔

Anyway, I'm not saying we need one and not the other. I'm just saying that if you go with the initial syntax proposed by Dennis, it'd be nice to support nested tags.

👍 I'm not saying it has to be one or the other, too – I'm just thinking out loud. We might very well end up with the declarative version.

@dmsnell
Copy link
Member Author

dmsnell commented Feb 15, 2023

you should...it'd be nice to

these two are very different 😉

I'm reluctant to talk about supporting arrays, multiple children, builder patterns, chained functions, JSX syntax, or other things because this is quintessentially text; while arrays, and array()s in particular (can be surprisingly dramatic), can so quickly and easily trash performance.

if we make it easy to build HTML triplets then I think it's a quick descent into people interpreting that as a resolve to build the entire app structurally, which while great from a convenience perspective, again could be a death blow to practical usability for the render and reader. great in JavaScript where each reader has excess CPU cores to handle a single page, pretty heavy-handed here in PHP on the server with thousands of request per CPU core.

it'd be nice to support nested tags.

note that nested tags are supported, but we have to eagerly flush them out, use string concatenation instead of array packing.

WP_HTML::tag_with_inner_html(
	'ul',
	null,
	WP_HTML::tag( 'li', null, 'one' ) . WP_HTML::tag( 'li', null, 'two' )
);

this doesn't prevent one from using an array structure in user-space, but it avoids making it appear as if it's the way to do it.

Not at all, __toString() is still there!

And don't I know it! But apparently WP doesn't like standards or embracing native platform support 😆 so I'm told this is taboo.


echo WP_HTML::make_tag(
    'a',
    array(
      'href' => '/some-page'
    ),
    array(
      "Navigate to ",
      WP_HTML::make_tag(
        'strong',
        array(),
        "Some Page"
      ),
      "."
    )
  );

this is a great example to discuss because it highlights one of the things I'm expressly worried about people doing. we've mixed text that needs to be escaped with text that needs to avoid being escaped. that is, "Navigate to " looks like a plain string here since it's not wrapped in anything like esc_html(), but make_tag() is returning a string with HTML embedded within it, and if escaped, will turn <strong> into &lt;strong>.

in a memory-hungry system we can defer all escaping to the render of the tree, but in the string API we have to delineate between these cases and ensure that the API makes this obvious and hard to miss (this is why I've proposed that the simpler WP_HTML::tag() performs encoding as the default so that we risk introducing a render defect instead of a security exploit.

probably worth noting that all existing cases where this is used is full of both rendering bugs and in many cases, security risks. it's not worse than before.

@luisherranz
Copy link
Member

luisherranz commented Feb 16, 2023

these two are very different

Oh, please don't pay too much attention to my English. Those are somewhat equivalent in Spanish 😄

@adamziel
Copy link
Contributor

@dmsnell I didn't realize you were after a foundational API to power other tooling. In this case obviously let's go for strings and then consider alternatives later on in any higher layers 👍

@adamziel
Copy link
Contributor

adamziel commented Feb 20, 2023

Here's the example @luisherranz shared, but using just strings:

echo WP_HTML::tag_with_inner_html(
    'a',
    array(
      'href' => '/some-page'
    ),
    (
        "Navigate to ".
        WP_HTML::tag( 'strong', array(), "Some Page About <p>-aragraphs").
        "."
    )
);

The outer tag allows unsafe contents since it's constructed with tag_with_inner_html(). The inner tag escapes the text content, as it's built with the default tag() method.

Honestly, I think this PR is nearly there – what do you think @dmsnell?

@dmsnell
Copy link
Member Author

dmsnell commented Feb 20, 2023

there's no need to pass array() in our examples. null is common and normative in PHP code with non-final optional parameters, and is how this method is currently written.

I didn't realize you were after a foundational API to power other tooling

me either! but I'm still aware that we're working in Core itself and people will use things we build in glorious ways we never imagined, so I want to be cognizant of how what we introduce might be used.

this is currently just a bit of a space to explore the idea.

@dmsnell
Copy link
Member Author

dmsnell commented Apr 10, 2023

I have a new idea based on the tag processor which is much better than this I think. It's an actual templating language using placeholders that are technically incorrect tag-closers.

Before

if ( current_user_can( 'create_users' ) ) :
	?>
	<a href="<?php echo esc_url( network_admin_url( 'user-new.php' ) ); ?>" class="page-title-action"><?php echo esc_html_x( 'Add New', 'user' ); ?></a>
	<?php
endif;

After

if ( current_user_can( 'create_users' ) ) {
	echo wp_render_html_template(
		'<a href="</%url>" class="page-title-action"></%add_user></a>',
		array(
			'url'      => network_admin_url( 'user-new.php' ),
			'add_user' => _x( 'Add New', 'user' )
		)
	);
}

whereby in this new template the escaping is handled automatically, including the knowledge that we need to percent-escape the URL if it contains characters that need escaping.

this is build on the tag processor and therefore can be fully aware of the context in which each template element is found. the template placeholders cannot connote content in HTML because they are transformed into comments in the DOM (as they are invalid tag closers). in attributes they are allowed, but hopefully would be rare enough to cause a problem/they are only a problem if someone unintentionally writes them inside one of these templates.

@dmsnell dmsnell closed this Apr 12, 2023
@dmsnell dmsnell deleted the html-api/add-make-tag branch April 12, 2023 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants