Tag Processor: Add get_updated_html as a non-toString method of stringifying the markup #44597
Conversation
adamziel
added
[Type] Enhancement
adamziel
changed the title
| @@ -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 ); | |||
There was a problem hiding this comment.
why make this change here?
There was a problem hiding this comment.
Mostly because (string) $p is not readable, imho, and can be a bit confusing in PHP land.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@getdave we have hundreds of string casts in Core but most of them fall into a few categories:
in general we don't have many classes in Core; among those, only a handful provide |
This is what I was getting at. Thanks for being more specific here. |
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 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. |
| 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 | ||
| ); | ||
| } |
There was a problem hiding this comment.
More of nitpicks, but couple things:
- Docblock @Covers seems wrong.
- What is the exact purpose of this testcase? Does it test something that is not covered by the next testcase?
A part of #44410
What?
Adds a
get_updated_htmlas a more WordPress-style way of retrieving the updated markup than(string) $p.Why?
The point about
(string) $pfeeling weird came up multiple times in the original WP_HTML_Walker PR #42485Testing Instructions
Confirm the CI checks are green
cc @dmsnell @azaozz @getdave