WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#32008 closed enhancement (maybelater)

Wrapper method for esc_attr

Reported by: welcher Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.2
Component: General Keywords: has-patch
Focuses: Cc:

Description

I am proposing a wrapper method for esc_attr() that simply echo's the output of esc_attr(). It would save time and keystrokes.

I don't believe that using esc_attr__ or esc_attr_e for this purpose is correct. They should be used for translating the attribute and that is not part of this use case.

Patch attached

Attachments (5)

32008.patch (440 bytes) - added by welcher 6 years ago.
32008.diff (440 bytes) - added by welcher 6 years ago.
Corrected file name
32008.2.diff (1.4 KB) - added by welcher 6 years ago.
Refreshed patch for 4.2 with additional wrappers
32008.3.diff (4.5 KB) - added by welcher 6 years ago.
Adding unit tests
32008.4.diff (1.8 KB) - added by welcher 6 years ago.
Removed unit tests, updated names to be e_esc_{type} and added wrapper for esc_textarea

Download all attachments as: .zip

Change History (13)

@welcher
6 years ago

@welcher
6 years ago

Corrected file name

#1 @welcher
6 years ago

  • Keywords has-patch dev-feedback added

#2 follow-up: @mordauk
6 years ago

I like this idea. When I first learned about the esc_attr() function, I just assumed that esc_attr_e() was the echo version. I'd love to have an echo version.

#3 in reply to: ↑ 2 @welcher
6 years ago

Replying to mordauk:

I like this idea. When I first learned about the esc_attr() function, I just assumed that esc_attr_e() was the echo version. I'd love to have an echo version.

@mordauk thanks!

I would like to extend the concept to other functions in the esc_* family as well. It would be handy to just call echo_url() or echo_html() and have the escaping done automatically.

A nice side effect of this is that WordPress developers who may not understand or know about the importance of escaping/sanitization would have it done for them.

Given the level of effort to implement this, it's a pretty nice gain from a security standpoint.

@welcher
6 years ago

Refreshed patch for 4.2 with additional wrappers

@welcher
6 years ago

Adding unit tests

#4 @jorbin
6 years ago

  • Keywords 2nd-opinion added
  • Milestone changed from Awaiting Review to 4.3

While I generally support unit tests and adding unit tests for new functions, I'm not sure we need unit tests just to confirm that echo is working.

As far as naming things, I don't know if I like 'echo_{type}' since it's not clear that things are being escaped. A couple of ideas:

e_esc_{type} - Prepend e to indicate echo. It could be really easy to confuse this with the esc_{type}_e functions though.
echo_esc_{type} - Makes it very clear, but doesn't save any characters. Does however make it possible for autocomplete if your editor supports it.

I'm torn. Would love to hear some other ideas. As of right now, I'm leaning towards the longer echo_esc_{type} Either way, moving this to 4.3 so that we make a decision here.

#5 follow-ups: @dd32
6 years ago

Personally I'm leaning away from adding it.

  • esc_{type}_e() is shorter than echo esc_{type}( __() ) which is why it exists
  • echo_esc_{type}() isn't any shorter than echo esc_{type}() and the extra brackets from above are not needed
  • e_esc_{type}() or esc_{type}_echo is too much like esc_{type}_e(), and seems like it'll cause confusion even further
  • I don't really like dropping the esc prefix in cases like echo_attr(), as it's less obvious what the function does, and easy to confuse with echo attr() in code review

Is auto-complete by an IDE really worth it here? it's the difference between echo_ and echo esc having to be typed for triggering the auto complete..

#6 in reply to: ↑ 5 @welcher
6 years ago

Thanks for your comments @dd32!

I agree with leaving esc in the function name and about esc_{type} and echo_{type} being easily confused in code reviews - I'll update the patch accordingly and remove the unit tests as per @jorbin

However, I would argue that prepending e_ is less confusing than it seems for a couple of reasons:

  1. The functions make complete sense when read left to right. They echo the output of esc_{type}().
  2. Translations functions have _e at the end of the method name and can be read as if they are mashing esc_{type}() and _e() together - clearly this is not the case but the effect is similar.
  3. There are paradigms in place in core now where prepending a function changes the result/output. i.e the_title() and get_the_title()

Replying to dd32:

Personally I'm leaning away from adding it.

  • esc_{type}_e() is shorter than echo esc_{type}( __() ) which is why it exists
  • echo_esc_{type}() isn't any shorter than echo esc_{type}() and the extra brackets from above are not needed
  • e_esc_{type}() or esc_{type}_echo is too much like esc_{type}_e(), and seems like it'll cause confusion even further
  • I don't really like dropping the esc prefix in cases like echo_attr(), as it's less obvious what the function does, and easy to confuse with echo attr() in code review

Is auto-complete by an IDE really worth it here? it's the difference between echo_ and echo esc having to be typed for triggering the auto complete..

@welcher
6 years ago

Removed unit tests, updated names to be e_esc_{type} and added wrapper for esc_textarea

#7 in reply to: ↑ 5 @jorbin
6 years ago

Replying to dd32:

Personally I'm leaning away from adding it.

  • esc_{type}_e() is shorter than echo esc_{type}( __() ) which is why it exists
  • echo_esc_{type}() isn't any shorter than echo esc_{type}() and the extra brackets from above are not needed
  • e_esc_{type}() or esc_{type}_echo is too much like esc_{type}_e(), and seems like it'll cause confusion even further
  • I don't really like dropping the esc prefix in cases like echo_attr(), as it's less obvious what the function does, and easy to confuse with echo attr() in code review

Is auto-complete by an IDE really worth it here? it's the difference between echo_ and echo esc having to be typed for triggering the auto complete..

I think you are right, auto-complete by an IDE isn't really that much of a benifit. However, this is one area that WordPress functions lack consistency. In many places we have echo and return variants (as pointed out above, it's often with the return variant being prefixed with get_.

That said, e_esc_{type} is confusing and way to close to esc_{type}_e. We don't use e_ anywhere else in the codebase while we do use _e and the context is going to be different. A prefix that is the exact inverse of a suffix shouldn't have a completely different context (behavior change vs. translation).

I'm tempted to close this as maybelater unless a quality name can be decided upon. We can always revisit this if/when a better name has been decided.

#8 @obenland
6 years ago

  • Keywords dev-feedback 2nd-opinion removed
  • Milestone 4.3 deleted
  • Resolution set to maybelater
  • Status changed from new to closed

I agree, it's too close to the translation functions syntax which is confusing enough for developers as it is, see @mordauk's comment from earlier.

For template tags we have get_the_ and the_, but this is not applicable here, which is fine. I think it's not too much to ask to type out echo.

Note: See TracTickets for help on using tickets.