Make WordPress Core

Opened 12 years ago

Closed 8 years ago

Last modified 8 years ago

#21613 closed enhancement (fixed)

format_to_edit runs esc_textarea if $richedit param is set to false, not true

Reported by: vhauri's profile vhauri Owned by: drewapicture's profile DrewAPicture
Milestone: 4.4 Priority: normal
Severity: normal Version:
Component: Formatting Keywords:
Focuses: Cc:

Description

Currently, the docs on format_to_edit() indicate that it runs the content through esc_textarea (which in turn runs htmlspecialchars() ) if the $richedit param is set to true. The code, however, runs the filter if the param is not set (or passed as false, see line 1270).

1255 /**
1256  * Acts on text which is about to be edited.
1257  *
1258  * The $content is run through esc_textarea(), which uses htmlspecialchars(
1259  * to convert special characters to HTML entities. If $richedit is set to t
1260  * it is simply a holder for the 'format_to_edit' filter.
1261  *
1262  * @since 0.71
1263  *
1264  * @param string $content The text about to be edited.
1265  * @param bool $richedit Whether the $content should not pass through htmls
1266  * @return string The text after the filter (and possibly htmlspecialchars(
1267  */
1268 function format_to_edit( $content, $richedit = false ) {
1269   $content = apply_filters( 'format_to_edit', $content );
1270   if ( ! $richedit )
1271     $content = esc_textarea( $content );
1272   return $content;
1273 }
1274 

My thought is the if statement should evaluate whether $richedit is true, rather than false, and therefore apply the esc_textarea function only when explicitly passed as a param. This would, however, result in unexpected behavior for anyone currently passing only the default $content param and getting sanitized output.

Attachments (1)

21613.diff (441 bytes) - added by vhauri 12 years ago.
patch changes when esc_textarea is called on the content (i.e. when $richedit is true rather than false)

Download all attachments as: .zip

Change History (10)

@vhauri
12 years ago

patch changes when esc_textarea is called on the content (i.e. when $richedit is true rather than false)

#1 @vhauri
12 years ago

  • Keywords has-patch added

#2 follow-up: @SergeyBiryukov
12 years ago

Currently, the docs on format_to_edit() indicate that it runs the content through esc_textarea (which in turn runs htmlspecialchars() ) if the $richedit param is set to true.

The docs actually say that htmlspecialchars() will be applied if $richedit is false:
http://core.trac.wordpress.org/browser/tags/3.4.1/wp-includes/formatting.php#L1256

Current behaviour appears to be proper.

#3 in reply to: ↑ 2 ; follow-up: @vhauri
12 years ago

Replying to SergeyBiryukov:

Currently, the docs on format_to_edit() indicate that it runs the content through esc_textarea (which in turn runs htmlspecialchars() ) if the $richedit param is set to true.

The docs actually say that htmlspecialchars() will be applied if $richedit is false:
http://core.trac.wordpress.org/browser/tags/3.4.1/wp-includes/formatting.php#L1256

Current behaviour appears to be proper.

Well, the code comment was recently updated (between v 3.3.3 and 3.4) to reflect the fact that the behavior is backwards( 'Whether the $content should pass through htmlspecialchars().' has become 'Whether the $content should NOT pass through htmlspecialchars().'). I'm not sure the variable name $richedit makes sense anymore...perhaps $dont_encode = false would make more sense. Either way, having a variable that a) isn't really descriptive and b) written as a double negative is probably not ideal for clarity.

Last edited 12 years ago by vhauri (previous) (diff)

#4 in reply to: ↑ 3 @SergeyBiryukov
12 years ago

Replying to vhauri:

Well, the code comment was recently updated (between v 3.3.3 and 3.4)

Related: [19982]

#5 @nacin
10 years ago

  • Component changed from General to Formatting
  • Keywords 2nd-opinion added

#6 @chriscct7
8 years ago

  • Keywords needs-docs added; has-patch 2nd-opinion removed
  • Milestone changed from Awaiting Review to Future Release
  • Owner set to chriscct7
  • Severity changed from minor to normal
  • Status changed from new to accepted
  • Type changed from defect (bug) to enhancement

I agree the double negative in the docs is really annoying. Perhaps there's a way it can be fixed.

#7 @DrewAPicture
8 years ago

  • Keywords needs-docs removed
  • Milestone changed from Future Release to 4.4
  • Owner changed from chriscct7 to DrewAPicture
  • Status changed from accepted to assigned

I got this.

#8 @DrewAPicture
8 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 34727:

Formatting: Rename the $richedit parameter in format_to_edit() to $rich_text.

Previously, it was necessary to explain in a double-negative that $richedit being false would prevent $content from being passed through esc_textarea(). The updated $rich_edit name and documentation now better reflects the intent of the parameter.

Fixes #21613.

#9 @chriscct7
8 years ago

Cool, thanks @drewapicture!

Note: See TracTickets for help on using tickets.