Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

using printf/sprintf in some cases makes escaping less than ideal #556

Closed
drrobotnik opened this issue Aug 7, 2014 · 2 comments
Closed

Comments

@drrobotnik
Copy link

There are a few areas of _s that use sprintf/printf that obfuscate readability and make it harder to escape as late as possible.

<?php
      // Show an optional term description.
      $term_description = term_description();
      if ( ! empty( $term_description ) ) :
        printf( '<div class="taxonomy-description">%s</div>', $term_description );
      endif;

      // Identical functionality, better escaping, more readable
      if ( ! empty( $term_description ) ) :
      ?><div class="taxonomy-description"><?php esc_html_e($term_description); ?></div><?php
      endif;
?>

This is a simple example but _s_posted_on() is kind of a nightmare to read.

@obenland
Copy link
Member

obenland commented Aug 7, 2014

WordPress allows HTML in term descriptions, so it's not advisable to escape them.

It's debatable if switching in and out of PHP really increases readability, but we recently reduced the use of printf()/sprintf() in _s_posted_on(). See fc9910c. I think in its current state it makes sense.

@drrobotnik
Copy link
Author

This ticket was inspired based on feedback I received on an ongoing VIP project and based on escaping all the things. If esc_html is incorrect it should still be escaped:

$term_description should be escaped. For example, with esc_html(), or if HTML tags need to be supported, with strip_tags() or wp_kses_post().
printf( '<div class="taxonomy-description">%s</div>', $term_description );

It is debatable that going in and out of php is more readable. It is my opinion that it is more readable. You have in the _sposted_on() example you still have three nested sprintf functions that are stored to a variable and passed into another (inception). For people using this as a starter theme, they'll most likely need to change some part of the output, it does become confusing.

What is the argument against going in and out of php? There are many instances where _s goes in and out of php for readability.

siliconforks pushed a commit to siliconforks/_s that referenced this issue Jan 29, 2015
Now uses the new archive template tags and makes archive template
titling way simpler!

Added shims for backwards compatibility, which can be removed once
WordPress 4.3 was released.

See https://core.trac.wordpress.org/changeset/30223
Closes Automattic#556.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants