Repository navigation
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
7a4779c to
7735a48
Compare
`wp_get_tooltip()` and `wp_get_toggletip()` concatenated caller-supplied button markup into a `sprintf()` format string, so literal percent signs in valid markup were read as conversion specifications. This caused fatal errors or silently corrupted output, including percent-encoded URLs. Build the generated button with its final label, icon, and popover target rather than with placeholders, then pass the button markup to `sprintf()` as a value. Caller-supplied button markup is no longer scanned or substituted. See #65914.
7735a48 to
7d884cb
Compare
joedolson
left a comment
joedolson
left a comment
There was a problem hiding this comment.
A couple changes to how I think this can be best handled.
The generated default button is now a plain skeleton, and every dynamic attribute is added in the single tag processor pass that already sets the ARIA attributes: the accessible name is only added when the markup does not already carry one, the toggle class and popover wiring are applied to every accepted button, and the dashicon span is only filled in for the generated button. This removes the string concatenation and lets the processor handle all attribute escaping. Also renames a test ID so it does not point at a specific plugin, and adds a test pinning that a custom button without an accessible name gains one while a caller supplied aria-label is never overwritten.
joedolson
left a comment
joedolson
left a comment
There was a problem hiding this comment.
With these changes and a couple additional comment changes, I think this is good to go.
|
Thank you for the approval and the doc cleanup. While reading your doc commit I noticed the same wording also sits in the wp_get_tooltip() and wp_get_toggletip() docblocks, both still say "Default standard button HTML". Since the default is now an empty string, should I push the same small fix to both? Happy to do it right away if you think it is worth it, or leave it to you. |
|
Good catch! Yes, please do update those, as well. |
Trac ticket: https://core.trac.wordpress.org/ticket/65914
This PR fixes a regression in
wp_get_tooltip()andwp_get_toggletip()where custom button markup passed via thebuttonargument is concatenated into asprintf()format string.Because of that, literal percent signs in valid button or anchor markup can be treated as format specifiers. This can cause fatal errors or silently corrupt output, for example percent-encoded URLs or button text like
100% done.The fix resolves the tooltip-supported placeholders in the button markup first, then passes the resulting button HTML to
sprintf()as a value instead of as part of the format string.Testing:
npm run test:php -- --group tooltipcomposer lint:errors -- src/wp-includes/general-template.php tests/phpunit/tests/general/wpGetTooltip.phpResults: