Make WordPress Core

Opened 5 years ago

Closed 14 months ago

#50551 closed enhancement (maybelater)

Add esc_xml l10n helpers

Reported by: swissspidy's profile swissspidy 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)

50551.diff (2.1 KB) - added by swissspidy 5 years ago.
50551.2.diff (6.4 KB) - added by pbiron 5 years ago.

Download all attachments as: .zip

Change History (15)

@swissspidy
5 years ago

This ticket was mentioned in Slack in #core-sitemaps by otto42. View the logs.


5 years ago

#2 follow-up: @pbiron
5 years ago

I opened a PR for wp i18n make-pot, see https://github.com/wp-cli/i18n-command/pull/221

#3 @swissspidy
5 years ago

The patch could be updated to replace all usages of esc_xml( __() ) with esc_xml__() and such.

@pbiron
5 years ago

#4 @pbiron
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 @pbiron
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

#6 @swissspidy
5 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 5.6

This ticket was mentioned in Slack in #core by justinahinon. View the logs.


5 years ago

#8 @helen
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.

#9 @pbiron
5 years ago

@helen Sure, I'll try to do that later today or tomorrow!

#10 @ocean90
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 @helen
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.

#13 @swissspidy
14 months ago

  • Milestone Awaiting Review deleted
  • Resolution set to maybelater
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.