Opened 5 years ago
Closed 14 months ago
#50551 closed enhancement (maybelater)
Add esc_xml l10n helpers
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | Priority: | normal | |
Severity: | normal | Version: | |
Component: | I18N | Keywords: | has-patch needs-refresh |
Focuses: | Cc: |
Description
In #50117 we introduced a new esc_xml()
function.
For consistency with other esc_*()
functions, we should introduce esc_xml__()
, esc_xml_e()
, and esc_xml_x()
functions.
Note: this requires support for these to be added to tools like WP-CLI's i18n-command.
Attachments (2)
Change History (15)
This ticket was mentioned in Slack in #core-sitemaps by otto42. View the logs.
5 years ago
#3
@
5 years ago
The patch could be updated to replace all usages of esc_xml( __() )
with esc_xml__()
and such.
#4
@
5 years ago
50551.2.diff is the same as 50551.diff but with all occurrences of esc_xml( __() )
in core (all in sitemaps-related code) replaced by esc_xml__()
.
#5
in reply to:
↑ 2
@
5 years ago
Replying to pbiron:
I opened a PR for
wp i18n make-pot
, see https://github.com/wp-cli/i18n-command/pull/221
This PR has been merged and released https://github.com/wp-cli/i18n-command/releases/tag/v2.2.5
This ticket was mentioned in Slack in #core by justinahinon. View the logs.
5 years ago
#8
@
5 years ago
- Keywords needs-refresh added
Patch needs an update for the @since
annotations. I would also like to see this broken out into two patches, one adding the functions as per @swissspidy's first patch, and then the actual core usage from @pbiron in a separate patch so they can be committed separately.
#10
@
5 years ago
It doesn't seem like there's enough usage yet which makes me think that we should close this as maybelater for now.
Consistency isn't quite a valid point since we don't have wrappers for esc_textarea()
, esc_url()
or esc_js()
either.
This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.
5 years ago
#12
@
5 years ago
- Milestone changed from 5.6 to Awaiting Review
Thinking about this more holistically, I don't see consistency as an end goal to have, and I want us to think about what esc_xml()
communicates to developers in terms of its intended and/or proper usage. We have an existing problem in esc_html()
vs. esc_js()
, where the former means "escape any HTML in here" and the latter means "escape for (a very narrow subset of) JavaScript usage". This feels more like the esc_js()
case, and that's not a good thing because I see that function misused constantly. Even the docblock for esc_xml()
is vague: Escaping for XML blocks.
. The filter contained within does a better job: Filters a string cleaned and escaped for output in XML.
Functionally maybe this means the same thing, but the intention is different IMO and may mean that the function actually has a greater scope of escaping.
If we add translation functions, what does that communicate to developers? Is there general understanding that they are "just" wrappers to avoid a couple parens or does it end up saying "you should be translating fully formed XML"? The latter sounds ridiculous and I hope that wouldn't be the takeaway, but generally when adding functions I want to be more thoughtful about what it's telling developers is good practice and that's something I can see happening.
In any case, we can discuss further, for now I'm punting out of 5.6 at least and leaving it to @ocean90 et al about a maybelater
status.
I opened a PR for
wp i18n make-pot
, see https://github.com/wp-cli/i18n-command/pull/221