Opened 10 years ago
Closed 10 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)
Change History (13)
#2
follow-up:
↓ 3
@
10 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
@
10 years ago
Replying to mordauk:
I like this idea. When I first learned about the
esc_attr()
function, I just assumed thatesc_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.
#4
@
10 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:
↓ 6
↓ 7
@
10 years ago
Personally I'm leaning away from adding it.
esc_{type}_e()
is shorter thanecho esc_{type}( __() )
which is why it existsecho_esc_{type}()
isn't any shorter thanecho esc_{type}()
and the extra brackets from above are not needede_esc_{type}()
oresc_{type}_echo
is too much likeesc_{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 withecho 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
@
10 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:
- The functions make complete sense when read left to right. They
echo
the output ofesc_{type}()
. - Translations functions have
_e
at the end of the method name and can be read as if they are mashingesc_{type}()
and_e()
together - clearly this is not the case but the effect is similar. - There are paradigms in place in core now where prepending a function changes the result/output. i.e
the_title()
andget_the_title()
Replying to dd32:
Personally I'm leaning away from adding it.
esc_{type}_e()
is shorter thanecho esc_{type}( __() )
which is why it existsecho_esc_{type}()
isn't any shorter thanecho esc_{type}()
and the extra brackets from above are not needede_esc_{type}()
oresc_{type}_echo
is too much likeesc_{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 withecho attr()
in code reviewIs auto-complete by an IDE really worth it here? it's the difference between
echo_
andecho esc
having to be typed for triggering the auto complete..
@
10 years ago
Removed unit tests, updated names to be e_esc_{type} and added wrapper for esc_textarea
#7
in reply to:
↑ 5
@
10 years ago
Replying to dd32:
Personally I'm leaning away from adding it.
esc_{type}_e()
is shorter thanecho esc_{type}( __() )
which is why it existsecho_esc_{type}()
isn't any shorter thanecho esc_{type}()
and the extra brackets from above are not needede_esc_{type}()
oresc_{type}_echo
is too much likeesc_{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 withecho attr()
in code reviewIs auto-complete by an IDE really worth it here? it's the difference between
echo_
andecho 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
@
10 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
.
Corrected file name