Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
HTML API: Add normalization functions. #7331
HTML API: Add normalization functions. #7331
Changes from 4 commits
d372c97
07e89bb
754118e
9c6e34c
7f5783e
2aa89fa
7029b25
00a5773
99799b7
db24c11
7b8aa53
18b5005
47f7f08
92558b9
6167887
95756e5
fca6481
b37b312
dd4ff16
588020f
0abf367
37b7fe8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this make sense to throw an exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried to avoid throwing exceptions in use code. Tell me more about the value of potentially crashing vs. returning
null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I guess to give more information about why it is returning
null
. Maybe_doing_it_wrong()
then would be better? It would be helpful to get feedback in code for what is documented:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah okay I see now. another thought I had was resetting to the beginning, parsing, and returning to the previous location, which involves double-parsing if already mid-way through a document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've called
wp_trigger_error()
in these cases.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about processing instructions? Shouldn't they get a special treatment?
For example,
<html><body><?php foo(); ?>
is interpreted as:Seems like it should get serialized back in the same way? Maybe not since the browser serializes this as
<!--?php foo(); ?-->
. But maybe that should be an option?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah good catch: it should also serialize the PI node tag name, which would match what you wrote. looks like this needs a review of all of the invalid comment syntax
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these should all be updated now. if something lingers I'd like to fix it, but ultimately if we botch an invalid comment, I'm guessing it's not the end of the world.
these will go into test cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While not required, it seems a space is usually added here in the wild, right? (e.g. Prettier does this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a good thought. with double-quoted attributes it's not relevant, but with unquoted attribute values it becomes relevant. we don't need that since we control quoting. maybe it's best to add it in anyway for the same of other tools.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly here, it would be helpful to know why it returned
null
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a tougher question because it would conflate the basic
?string
return value. practically I think this can only occur if the HTML is unsupported (in which case we really shouldn't return any processed string) or we've run out of bookmarks (which should be unrealistically rare - and that reminds me, I found 2500 bookmarks sufficient to parse everything in my set of ~300k HTML documents, and I intend on upping the default value to support that for 6.7).suppose you know why this failed: what would you do in response?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could also use
_doing_it_wrong()
here too to communicate that information, I suppose. Or rather,wp_trigger_error()
would be the more relevant function. If I knew why it failed then I wouldn't have to figure out why it failed. True it probably wouldn't impact the result in the end, but for debugging it would be useful.Looking at where
last_error
is set, it seems to always coincide with throwing an exception anyway. So in practice would thisif
statement ever be entered?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the exceptions thrown internally are caught and shut down parsing, but does not crash. unsupported content exceptions are returned via
get_unsupported_exception()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've called
wp_trigger_error()
in these cases.