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
KSES: Allow Custom Data Attributes having dashes in their dataset name. #6429
base: trunk
Are you sure you want to change the base?
KSES: Allow Custom Data Attributes having dashes in their dataset name. #6429
Changes from 6 commits
5331865
0c7ad50
c10b5c9
606469a
00173ee
e53933d
8b7219c
5294a86
987fc65
422ed4e
dc859f1
0498bad
2187e17
0c1b6f9
aa63e2b
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.
Adds a summary so the details are all included in the description section of the developer docs (see
wp_kses()
as an example).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.
@peterwilsoncc are you recommending a different wording of the summary, or worried that the existing summary won't appear in the docs? The existing summary is up to two lines at the start of the docblock, so this will appear as-written in the patch.
For example,
class_name_updates_to_attributes_updates()
also has a two-line summary.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'm mainly suggesting different, shorter, phrasing.
I thought the summaries were intended to be a one-liner but just re-read the doc standards and it turns out I was mistaken.
I'll leave this up to you.
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.
Reworded in dc859f1. It may take some more rewording 🤷♂️
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'd like to see cases covering
data--leading
data-trailing-
anddata-middle--double
cases. The--
together in the middle is missing.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.
With this change it would be good to convert this to use a data provider and add a bunch more valid and invalid use cases.
It probably should have been a data provider all along but please do not look in to the history of this feature to figure out who didn't do that in the first place. ;)
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.
Converted in 5294a86
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.
WordPress can be stricter than the specs and browser implementations (especially that latter if they differ from the former), to an extent that's the entire purpose of KSES.
I don't think accounting for special characters is overly wise as it's too easy to miss situations in which a contributor or author role can break out of the intended behaviour.
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.
thanks @peterwilsoncc! in the code for this PR there are two stages: the first is determining if there's a data attribute, and then the second is locking down a smaller allowable subset. these tests, and this function, is to ensure that we can all agree on what is a data attribute before we then apply further constraints.
it doesn't have to be this way, of course, but I find that it's really easy to let things slide through when we conflate the two ideas. in this particular case it looks like everything else should be eliminated in
wp_kses_attr_check()
other than the specific data attributes, but I also had to double and triple check the logic to make sure we weren't letting through some data attributes only because they didn't fit the pattern we expected.so this was done for the reason of making it less likely to let something slip through. it doesn't have to be done this way, but it was the purpose for the structuring I made.
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.
Thanks @dmsnell that makes a lot of sense. 👯