Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tag Processor: Add get_updated_html as a non-toString method of stringifying the markup #44597

Merged
merged 2 commits into from
Oct 18, 2022

Conversation

adamziel
Copy link
Contributor

A part of #44410

What?

Adds a get_updated_html as a more WordPress-style way of retrieving the updated markup than (string) $p.

Why?

The point about (string) $p feeling weird came up multiple times in the original WP_HTML_Walker PR #42485

Testing Instructions

Confirm the CI checks are green

cc @dmsnell @azaozz @getdave

@adamziel adamziel added [Type] Enhancement A suggestion for improvement. Developer Experience Ideas about improving block and theme developer experience labels Sep 30, 2022
@adamziel adamziel self-assigned this Sep 30, 2022
@adamziel adamziel changed the title Add get_updated_html as a non-toString method of stringifying the markup Tag Processor: Add get_updated_html as a non-toString method of stringifying the markup Sep 30, 2022
@@ -294,7 +314,7 @@ public function test_set_attribute_prevents_xss( $attribute_value ) {
* over the content and because we want to look at the raw values.
*/
$match = null;
preg_match( '~^<div test=(.*)></div>$~', (string) $p, $match );
preg_match( '~^<div test=(.*)></div>$~', $p->get_updated_html(), $match );
Copy link
Member

Choose a reason for hiding this comment

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

why make this change here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Mostly because (string) $p is not readable, imho, and can be a bit confusing in PHP land.

Copy link
Member

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

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

I'm pre-empting approval but I don't feel convinced that we should update the tests. Essentially this work is a convenience for the fact that string-casting apparently isn't all that common. To me that's fine to add as a helper, but it doesn't change the essence of what this is.

it may not matter, particularly in the tests, which method we call. I'm musing about the nature of this class, even though practically they are the same. still, it leaves a weird feeling in me to add in user-space what is provided natively by the platform and then to push people into that user-space wrapper.

Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

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

Thanks for following up here.

@dmsnell your points are well made. Do we have an existing precedent for using toString in this way? What I'm asking is how common it is in WordPress-land and thus if we rely on it will the majority of folks be familiar or just plain confused?

If there is no prior example and we're the first then it's a code style thing in which cases I'll defer to more experienced PHP contributors (@azaozz) to make the call on whether this is necessary.

@dmsnell
Copy link
Member

dmsnell commented Oct 4, 2022

@getdave we have hundreds of string casts in Core but most of them fall into a few categories:

  • things we expect to be strings
  • things we don't know are strings, but we want to coerce as such in case they aren't
  • return values which are usually strings but might have failure values that aren't

in general we don't have many classes in Core; among those, only a handful provide __toString(). of those, I'm not sure if any are in active use the way this would be. WP_Theme implements the method.

@getdave
Copy link
Contributor

getdave commented Oct 4, 2022

in general we don't have many classes in Core; among those, only a handful provide __toString(). of those

This is what I was getting at. Thanks for being more specific here.

@azaozz
Copy link
Contributor

azaozz commented Oct 7, 2022

I'm not sure if any are in active use the way this would be (cast a class instance to a string)

Don't think so. There are few that might be used that way but never seen anybody do that.

Generally casting an object to a string is a JS thing; all objects there have a built-in .toString(). That's not the case for PHP. As far as I've seen casting in PHP is mostly associated with "weak typing" and is used internally when you do stuff like if ( 1 == 'string' ). Do you actually know what happens when casting is used? The result depends not only on the value but also on the type of the casted var. That makes casting a little bit uncertain. It can guarantee the type but not the value.

Another big reason not to cast a class instance to a string is readability. It is not self-documenting. Would be many times better to have a (proper, expected) function call that would return the output, as all the other similar classes everywhere.

@adamziel adamziel merged commit d406de5 into trunk Oct 18, 2022
@adamziel adamziel deleted the tag-processor/get_updated_html branch October 18, 2022 22:39
@github-actions github-actions bot added this to the Gutenberg 14.4 milestone Oct 18, 2022
Comment on lines +178 to +191
public function tostring_returns_updated_html() {
$p = new WP_HTML_Tag_Processor( '<hr id="remove" /><div enabled class="test">Test</div><span id="span-id"></span>' );
$p->next_tag();
$p->remove_attribute( 'id' );

$p->next_tag();
$p->set_attribute( 'id', 'div-id-1' );
$p->add_class( 'new_class_1' );

$this->assertEquals(
$p->get_updated_html(),
(string) $p
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

More of nitpicks, but couple things:

  1. Docblock @Covers seems wrong.
  2. What is the exact purpose of this testcase? Does it test something that is not covered by the next testcase?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Developer Experience Ideas about improving block and theme developer experience [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants