WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 4 weeks ago

Last modified 4 weeks ago

#17571 closed defect (bug) (invalid)

bad double quote replacement in wp_texturize

Reported by: 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 3 years ago.
17571.patch (1.2 KB) - added by kurtpayne 3 years ago.
Refreshed patch as of 3.3beta4. Handling escaped apostrophes, too.
17571_unit_test.patch (1.1 KB) - added by kurtpayne 3 years ago.
Unit test
17571.2.patch (2.4 KB) - added by kurtpayne 2 years ago.
Refreshed for 3.4
17571.3.patch (2.4 KB) - added by kurtpayne 2 years ago.
Just address the curling issue, do not strip slashes

Download all attachments as: .zip

Change History (22)

comment:1 SergeyBiryukov3 years ago

  • Keywords reporter-feedback added

Where exactly do you see those double-escaped values?

comment:2 qdinar3 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)

comment:3 qdinar3 years ago

another example:

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

becomes «123«.

comment:4 SergeyBiryukov3 years ago

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

Related: #18549

kurtpayne3 years ago

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

kurtpayne3 years ago

Unit test

comment:5 kurtpayne3 years ago

  • Cc kpayne@… added

comment:6 follow-up: kurtpayne2 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.

kurtpayne2 years ago

Refreshed for 3.4

comment:7 in reply to: ↑ 6 kurtpayne2 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.

comment:8 SergeyBiryukov2 years ago

  • Milestone changed from Awaiting Review to 3.4

comment:9 SergeyBiryukov2 years ago

  • Component changed from General to Formatting

comment:10 follow-up: nacin2 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;.

comment:11 in reply to: ↑ 10 kurtpayne2 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.

kurtpayne2 years ago

Just address the curling issue, do not strip slashes

comment:12 kurtpayne2 years ago

  • Keywords dev-feedback added

comment:13 nacin2 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?

comment:14 miqrogroove4 months 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.

comment:15 miqrogroove3 months ago

  • Keywords close added

comment:16 miqrogroove4 weeks ago

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

comment:17 DrewAPicture4 weeks ago

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