Make WordPress Core

Opened 13 years ago

Closed 10 years ago

Last modified 10 years ago

#17571 closed defect (bug) (invalid)

bad double quote replacement in wp_texturize

Reported by: csanquer's profile csanquer Owned by:
Milestone: Priority: normal
Severity: minor Version:
Component: Formatting Keywords: has-patch dev-feedback wptexturize
Focuses: Cc:

Description

Bug seen in Wordpress 3.1.2. with buddypress 1.2.8

Some database fields that contain a string like this 'a string with \"escaped double quotes\"' are displayed this way : 'a string with >>escaped double quotes>> ' instead of 'a string with <<escaped double quotes>>'

The bug come from wp_texturize function in wp-includes/formatting.php .

I join a patch to resolve this issue.

Attachments (5)

wordpress_quote_replacement.patch (1.1 KB) - added by csanquer 13 years ago.
17571.patch (1.2 KB) - added by kurtpayne 12 years ago.
Refreshed patch as of 3.3beta4. Handling escaped apostrophes, too.
17571_unit_test.patch (1.1 KB) - added by kurtpayne 12 years ago.
Unit test
17571.2.patch (2.4 KB) - added by kurtpayne 12 years ago.
Refreshed for 3.4
17571.3.patch (2.4 KB) - added by kurtpayne 12 years ago.
Just address the curling issue, do not strip slashes

Download all attachments as: .zip

Change History (22)

#1 @SergeyBiryukov
13 years ago

  • Keywords reporter-feedback added

Where exactly do you see those double-escaped values?

#2 @qdinar
13 years ago

SergeyBiryukov, he says about not double escaping, but that direction of quotes are not correct. This bug makes much work to me and probably to other wp users.

i can show a test case:

"<a href="http://example.com">123</a>"

shown as

«123«

(in wp 3.0.4)

#3 @qdinar
13 years ago

another example:

"<a href="http://example.com">123</a>".

becomes «123«.

#4 @SergeyBiryukov
13 years ago

  • Keywords needs-unit-tests added; reporter-feedback removed

Related: #18549

@kurtpayne
12 years ago

Refreshed patch as of 3.3beta4. Handling escaped apostrophes, too.

@kurtpayne
12 years ago

Unit test

#5 @kurtpayne
12 years ago

  • Cc kpayne@… added

#6 follow-up: @kurtpayne
12 years ago

  • Keywords needs-patch added; has-patch removed

Patch is no longer valid for 3.4.0. This was invalidated by #19602. The example string:

"<a href="http://example.com">123</a>"

Is broken down to:

Array
(
    [0] => "
    [1] => <a href="http://example.com">
    [2] => 123
    [3] => </a>
    [4] => "
)

The beginning and ending quote are treated equally by the replacement regexes, so they both come out as an opening curly quote.

@kurtpayne
12 years ago

Refreshed for 3.4

#7 in reply to: ↑ 6 @kurtpayne
12 years ago

  • Keywords has-patch added; needs-patch removed

Replying to kurtpayne:

This was invalidated by #19602.

I was wrong about this. This ticket has 2 example strings given that produce undesired output. One is HTML ("<a href="...">link</a>") and one is escaped quotes (double \"quotes\").

This patch will not work with the HTML input. There is a separate ticket for this at #4539.

This patch will work with the escaped input. The patch has been refreshed for 3.4 and the unit tests pass, too.

#8 @SergeyBiryukov
12 years ago

  • Milestone changed from Awaiting Review to 3.4

#9 @SergeyBiryukov
12 years ago

  • Component changed from General to Formatting

#10 follow-up: @nacin
12 years ago

  • Keywords needs-unit-tests removed

Patch is fine, but I'm curious — Why should quotes ever be escaped, and why would we want to then go ahead and curl them? Obviously that they're curled in the same direction is the bug we're trying to fix, but I'm just wondering why they shouldn't just become &quot;.

#11 in reply to: ↑ 10 @kurtpayne
12 years ago

Replying to nacin:

Patch is fine, but I'm curious — Why should quotes ever be escaped?

Original reporter claims "Some database fields that contain a string like this."

Are you concerned that having wptexturize() eat "backslash-quote" and spit out "curly-quote" could be seen as a bug?

Obviously that they're curled in the same direction is the bug we're trying to fix

Alternate patch forthcoming.

@kurtpayne
12 years ago

Just address the curling issue, do not strip slashes

#12 @kurtpayne
12 years ago

  • Keywords dev-feedback added

#13 @nacin
12 years ago

  • Keywords wptexturize added
  • Milestone changed from 3.4 to Future Release

While it is backed up by unit tests, it is too late to make a change for an edge-case issue with texturize.

I didn't understand the nature of the bug report here originally. The issue appears to be that \" gets curled in the incorrect manner. When are backslashes ever going to be used? If it's ending up in the database, we should be looking at storage and retrieval, not texturize. Should the quote be curled in this case at all? Should the backslash be quietly stripped, or should it be kept? Are there any performance implications of the negative lookahead here?

#14 @miqrogroove
10 years ago

I agree with nacin's concerns. This appears to be a case of "garbage in, garbage out" where wptexturize is working as expected on bizarre data.

I vote wontfix.

#15 @miqrogroove
10 years ago

  • Keywords close added

#16 @miqrogroove
10 years ago

  • Resolution set to invalid
  • Status changed from new to closed

#17 @DrewAPicture
10 years ago

  • Keywords close removed
  • Milestone Future Release deleted
Note: See TracTickets for help on using tickets.