WP_HTML_Tag_Processor: Make get_attribute reflect attribute set via set_attribute, even without updating

[ockham]

What?

This adds a unit test to make sure get_attribute() picks up an attribute added by set_attribute().

Why?

This seems like a contract we might want to enforce via a unit test.

It didn't work until recently, when #46598 was merged.

How?

See code.

We might want to consider adding an assertion like that to other set_attribute() tests as well; or adding a separate test case just for the set_attribut/get_attribute pair.

Testing Instructions

See unit test.

[ockham]

Ah, it looks like #46598 is going to fix this.

Maybe we can add some get_attribute test coverage there.

[ockham]

Closing this one; discussing adding some test coverage over there: #46598 (comment)

[anton-vlasenko]

Ah, it looks like #46598 is going to fix this.

Maybe we can add some get_attribute test coverage there.

@ockham
#46598 is blocking #46510.
I'd appreciate it if these tests could be added in the scope of another PR.
Thanks.

[ockham]

Ah, it looks like #46598 is going to fix this.
Maybe we can add some get_attribute test coverage there.

@ockham #46598 is blocking #46510. I'd appreciate it if these tests could be added in the scope another PR. Thanks.

Okay! I guess I can re-open this PR to add the assertion.

[anton-vlasenko]

Thanks!
I appreciate it, @ockham.

[adamziel]

Good spot!

[adamziel]

E2E failures seem irrelevant so I restarted the tests

[dmsnell]

wanted to reach out and chat more synchronously on this but I'll leave a note here for everyone's sake.

it doesn't seem right to me to say that you are "doing it wrong" here but I've been tossing around the question of whether this is intended behavior for the tag processor. once we flush out changes by calling get_updated_html() I think the internal pointer advances and we're ready for the next tag.

does it reset? should it reset? I don't know.

what's the use-case for reading an attribute we just set? we had to have access to the value to set it before calling get_updated_html() so I'm curious why we would then turn around and ask for it back.

I think this is a behavior we didn't codify, what all happens when calling get_updated_html() - I guess the pointer should still point at the tag we were at because we should be able to call that function any time, but on the same note, the tag processor would need to back up and re-parse the current tag if we do go back like this.

[ockham]

Thank you for weighing in @dmsnell! I had a hunch that this had to do with the internal pointer advancing 😊

IMO, if there's a get_xyy / set_xyz pair, there's going to be a strong expectation that setting something will result in the corresponding getter retrieving that value for the thing that was just set. Realizing that that's not the case, and the reasons for it, requires deeper understanding of the inner workings of the class.

And while your reasons make sense, I think it's another example of how people's habits and expectations could be so strong that they might go as far as to even change the behavior of the class to match those expectations. This is why I think we should match them already 😬

This is assuming that people will actually encounter a need for set_attribute() followed by get_attribute(), and admittedly, my use case was prompted by a unit test (running the wp-bind directive processor on a wp-bind:src attribute, and verifying that it adds a src attribute).

But here's a more real-world example: For the Interactivity API, whenever we encounter a tag, we watch out for wp- (attribute) directives on that tag, and run the corresponding directive processors for each of them. It's absolutely possible that a tag has multiple such directives; there might be a wp-bind:some-attribute followed by a wp-show or so. They should all have access to the current tag (including adding or modifying attributes), without the need to rewind the tag pointer after they've run.

[adamziel]

once we flush out changes by calling get_updated_html() I think the internal pointer advances and we're ready for the next tag.

@dmsnell get_updated_html() explicitly rewinds the internal pointer after flushing the changes specifically to stay at the same tag. I built this behavior with __toString() in mind as an innocent $p.'' should not consume the next tag. Since get_updated_html() is just a human-readable name for __toString(), I think it's safe to say that not advancing the pointer to the next tag is a defined and expected behavior.

[dmsnell]

so @adamziel are you saying we have a bug then?
maybe I misunderstood the whole thing - I initially thought these tests were failing and @ockham was giving us a failing test, but maybe I was confused when I first looked.

if that's the case this is good to go I think.

[dmsnell]

okay well I thought there was bug here, because if you slightly change the order of operations @ockham you will discover this still isn't quite what you expect.

$p = new WP_HTML_Tag_Processor( '<div>' );
$p->next_tag();
$p->set_attribute( 'id', 'test' );
$p->get_attribute( 'id' ) === NULL;

it's another example of how people's habits and expectations could be so strong that they might go as far as to even change the behavior of the class to match those expectations.

In the docblock comments we have language about attribute updates being enqueued. this makes me wonder if we should rename these functions $p->enqueue_set_attribute

Or just deal with it.

Or add a call to get_updated_html() inside get_attribute() to flush out pending changes, which is also something that would be nice to avoid.

🤔

so we either change the class to match those expectations or make it more obvious that those expectations shouldn't apply. this class no more serves HTTP requests than it provides a DOM API.

there's a problem with immediately updating the document once we call set_attribute and that's that we may still have more set_attribute calls which might change the same attribute or make changes to a location before the one we're currently setting.

---

I think we should be able to check first if an attribute update is enqueued and return its value before looking at the parsed values which should solve this. let me prep a patch.

[ockham]

Just to cycle back to this comment:

maybe I misunderstood the whole thing - I initially thought these tests were failing and @ockham was giving us a failing test, but maybe I was confused when I first looked.

To be clear, this is what I did initially. However, shortly after I had filed this PR, #46598 was merged, and after I rebased the PR, the original error was gone -- #46598 had fixed it 😊

[ockham]

I think we should be able to check first if an attribute update is enqueued and return its value before looking at the parsed values which should solve this.

I think this sounds good!

let me prep a patch.

I saw that you've pushed a commit to add the failing test (9e38e42), but I guess you haven't had time yet to work on the fix. I can do this in the next couple of days unless you'll beat me to it 😊

[ockham]

let me prep a patch.

I saw that you've pushed a commit to add the failing test (9e38e42), but I guess you haven't had time yet to work on the fix. I can do this in the next couple of days unless you'll beat me to it 😊

Fixed in b559cd4. Still need to add logic (and test coverage) for class attributes.

[adamziel]

I left a note about the unit tests but it looks good to me otherwise. Thank you @ockham for addressing all the feedback!

[dmsnell]

sorry for the last-minute changes but I pushed 5e25270e66d54dba8a8aa6e9909402523001d782 in which I rearranged a few things stylistically and also reverted the behavioral change, because I find it confusing to preserve add_class/remove_class calls after set_attribute or remove_attribute has occurred.

this is negotiable, but I didn't see any clear reason why we changed the behavior (though I may have missed it while reading through the comments). if the reason is that it was a side-effect of rearranging the code then I think the change I've proposed is not worse than what it was before or after that change.

now, when calling the transactional swaps set_attribute or remove_attribute for class, all enqueued class-builder changes are wiped out and the new value goes in. any calls to the builder after this point augments the updated value. I believe this maps most closely to how successive edits to any other attribute work.

let me know how you feel. if you don't like what I changed then we can talk about it. the stylistic things are also up for grabs, but I think they are mostly new paint on the framework you built.

[dmsnell]

do we have a reason to change the behavior? I think it's probably important to ensure that set_attribute and remove_attribute updates wipe out class builder method changes, as I can reason about what it means to call add_class after setting the class attribute, but I can't reason an obvious outcome of calling add_class before the transactional swap of set_attribute

[dmsnell]

Squashed earlier work and rebased against latest trunk from 5e25270e66d54dba8a8aa6e9909402523001d782 to fafafce0d5d667a4a883ccb12bebd3fb03b1e59d

Second commit on the rebase contains the changes I made in my work today, which we can discuss, revert, or accept.

[adamziel]

I think it's mostly there – I left a few final notes 👍 (have we been repeating this for two weeks now? :D)

[ockham]

Okay, I've added the missing assertion error messages. We're all green -- looks like we can finally 🚢 this 🎉

Thank you @dmsnell @adamziel for your help! Great collab'ing with you 😊