Make WordPress Core

Opened 15 years ago

Last modified 3 months ago

#18549 assigned defect (bug)

wp_texturize incorrectly curls closing quotes after inline HTML end tags

Reported by: justincwatt Owned by: miqrogroove
Priority: normal Milestone: Future Release
Component: Formatting Version: 3.2.1
Severity: normal Keywords: wptexturize has-patch has-unit-tests
Cc: Focuses:

Description

The following source HTML:

The word is "<a href="http://example.com/">quoted</a>".
The word is '<a href="http://example.com/">quoted</a>'
The word is '<a href="http://example.com/">quoted.</a>'
The word is '<a href="http://example.com/">quoted</a>'.
The word is '<a href="http://example.com/">quot</a>'d

Gets incorrectly transformed by wp_texturize() as:

The word is &#8220;<a href="http://example.com/">quoted</a>&#8220;.
The word is &#8216;<a href="http://example.com/">quoted</a>&#8216;
The word is &#8216;<a href="http://example.com/">quoted.</a>&#8216;
The word is &#8216;<a href="http://example.com/">quoted</a>&#8216;.
The word is &#8216;<a href="http://example.com/">quot</a>&#8216;d

Note: all the double/single quotes in the above examples that should be closing are instead opening)

This renders in the browser like this:

The word is “quoted“.
The word is ‘quoted‘
The word is ‘quoted.‘
The word is ‘quoted‘.
The word is ‘quot‘d

The problem here is that wp_texturize splits the text on all start/end tags, which makes sense for block-level tags, but not inline-tags like <em> and <a href="">.

formatting.php line 67:

$textarr = preg_split('/(<.*>|\[.*\])/Us', $text, -1, PREG_SPLIT_DELIM_CAPTURE);

However if you change it to only split the content on block-level tags, you'll need something more sophisticated/complex than a regular expression to avoid curling quotes within html.

Attachments (9)

18549.diff​ (6.3 KB ) - added by wonderboymusic 12 years ago.
18549.2.diff​ (7.7 KB ) - added by miqrogroove 12 years ago.
Tweak the unit tests.
18549.3.diff​ (6.6 KB ) - added by miqrogroove 12 years ago.
Restore tests that fail.
18549_wptexturize.diff​ (23.2 KB ) - added by gitlost 11 years ago.
An implementation of miqrogroove's algorithm.
18549_tests.diff​ (10.7 KB ) - added by gitlost 11 years ago.
Unittests.
18549_wptexturize.2.diff​ (22.7 KB ) - added by gitlost 11 years ago.
Individual statics instead of arrays.
18549_wptexturize.3.diff​ (26.0 KB ) - added by gitlost 11 years ago.
Some performance tweaks.
18549_wptexturize.4.diff​ (34.1 KB ) - added by gitlost 10 years ago.
Refresh for 4.7. Includes unit tests.
18549-comprehensive-inline-quote-context.diff​ (10.0 KB ) - added by dpknauss 4 months ago.
Verified: targets src/wp-includes/formatting.php, 4 test methods, both helpers (incl. the fast-path).

Download all attachments as: .zip

Change History (54)

#2 @Ammaletu
15 years ago

  • Cc Ammaletu added
  • Version 3.2.1 → 3.3.1

I can confirm this issue. Haven't had time yet to write a test case or patch, but I took a look at the plugin "InTypo" (which I have been using for years to change the English quote characters to German ones). This plugin completely replaces wptexturize and does not show this bug except in one minor case. What InTypo does: It first replaces the closing quotes and then the opening ones. This way, when you get to replacing the opening quotes, the closing quotes are already safely replaced.

The code looks like this:

$dynamic_character = '/"([\.,;\!\?\s\)\]])/'
$dynamic_replacement = $closing_quote . '$1'

Basically, it replaces every quote character with a closing quote that is followed by a space, a closing bracket or a number of punctuation marks. That is not perfect yet, for example wptexturize handles { and < for opening quotes, so these should be handled here as well. Also probably any punctuation marks from different languages (are there any missing?). And finally this fails if the closing quote is the very last character of the post, a case that could be handled by adding \Z.

Could this be the basis for a patch?

#3 @SergeyBiryukov
15 years ago

  • Version 3.3.1 → 3.2.1

Version number indicates when the bug was initially introduced/reported.

#4 @SergeyBiryukov
14 years ago

#23827 was marked as a duplicate.

#5 @nacin
13 years ago

  • Keywords wptexturize added

#6 @nacin
13 years ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone Awaiting Review → Future Release

Unit tests in particular would move this along.

#7 @miqrogroove
13 years ago

All of this seems to fall within the scope of #4539, which already has unit tests. Can we close this one as a duplicate?

This ticket was mentioned in IRC in #wordpress-dev by miqrogroove. ​View the logs.


12 years ago

@miqrogroove
12 years ago

Tweak the unit tests.

#9 @wonderboymusic
12 years ago

  • Owner set to wonderboymusic
  • Resolution → fixed
  • Status new → closed

In 28764:

In wptexturize(), adjust for the treatment of abbreviated years at the end of quotations.
Silence some unit tests that have never passed and may no longer be applicable.

Props miqrogroove.
Fixes #18549.

#10 @miqrogroove
12 years ago

  • Resolution fixed
  • Status closed → reopened

Ticket still valid. Just didn't need the broken tests.

#11 @miqrogroove
12 years ago

#29287 was marked as a duplicate.

#12 @nacin
12 years ago

Were those tests actually broken? Because they seemed to be testing still-outstanding texturize issues.

#13 @miqrogroove
12 years ago

wonderboymusic wanted all the tests to pass, so we disabled the tests for this ticket and others that are still open.

#14 @nacin
12 years ago

Okay. In the future, failing tests should be broken out into individual tests and marked with @ticket for the open ticket.

@miqrogroove
12 years ago

Restore tests that fail.

#15 @miqrogroove
12 years ago

Added 18549.3.diff to update failing tests. The following tests are expected to fail:

test_texturize_around_html()
test_quotes_after_numbers()
test_entity_quote_cuddling()

#16 @miqrogroove
12 years ago

#30881 was marked as a duplicate.

#17 @miqrogroove
12 years ago

#31160 was marked as a duplicate.

#18 @miqrogroove
12 years ago

#31931 was marked as a duplicate.

#19 @miqrogroove
11 years ago

Algorithm brainstorm:

  1. Make a string that is stripped of all elements and shortcodes.
  2. Make an array of the elements and shortcodes, indexed by string position.
  3. Apply all formatting changes to the text-only string.
  4. Compare the formatted string to the unformatted string.
  5. Copy each formatting change to the original input OR insert the elements and shortcodes back into the formatted string.

This is based on the assumptions that we can't simply parse around elements, we can't use inline placeholders for the elements if we want to keep the existing regular expressions, and elements can be positioned anywhere in the text.

#20 @miqrogroove
11 years ago

Missing from previous comment:

  • It will be necessary to retain certain elements such as blocks which are effectively separate sentences even when they are not separated by whitespace.

#22 @paulschreiber
11 years ago

We just hit this twice today in 4.3. Example:
<strong>Read more: </strong>"<a href="​http://blah.com/test">Something (else)</a>"</p>

Can someone try and fix this for 4.4?

@gitlost
11 years ago

An implementation of miqrogroove's algorithm.

@gitlost
11 years ago

Unittests.

#23 @gitlost
11 years ago

This patch is an implementation of miqrogroove's algorithm to remove inline HTML and shortcodes while processing quotes, and then to reinstate them, using routines which wrap str_replace/preg_replace to keep track of offset changes.

It seems to run around 30-100% slower than the current version, though it can actually run faster occasionally depending on the data.

One incompatibility is thrown up by the unittests, in the "crazy" shortcode test.

#24 @miqrogroove
11 years ago

That's a great start. Some of the new functions seem a bit long and that may be where we can do some optimizing. Also, if renaming the variables is really important, I think we should eventually break that out to a separate patch for clarity. I will try to help out before the betas. Going to be tied up with non-wp stuff for a few weeks.

#25 @gitlost
11 years ago

Thanks very much for responding. If by renaming the variables you mean the $regexs array then no, it isn't really important(!) and didn't make sense anyway (no reason to deref an array needlessly) - was just getting embarrassed by the number of static variables being declared - I'll upload a new diff. The patch is just a starting point as you say and I look forward to your help when you get the chance...

@gitlost
11 years ago

Individual statics instead of arrays.

#26 @miqrogroove
11 years ago

Next thought: Adding this much code to wptexturize doesn't make sense in the big picture. Let's think about how to make the algorithm reusable. There is a good chance of reusing this pattern when normalizing whitespace in wpautop and similar situations.

#27 @gitlost
11 years ago

Unfortunately I don't know if there is much chance of reusing this algorithm in other situations, as the much simpler and faster solution of using numbered placeholders (as for <pre> tags in wpautop) is usually good enough - indeed it would kinda more or less work for wptexturize() as well (eg substituting a placeholder plus a space for opening inline tags).

The amount of code is very off-putting I agree, but in its defence it's very mechanical, repetitive code, and unlikely to have unforeseen consequences.

Anyway I'll upload the latest version, which has some speed tweaks - one being to add back the standard replace stuff if there's no wptexturize_replace_init() match (more code bloat!), another is adding the study option to wptexturize_replace_init()'s regex, another is moving the "atomic" adjustments to wptexture_replace_final(). It also takes the liberty of rejigging _wptexturize_pushpop_element() which was a bit slow.

It seems now to run < 20% slower on most inputs (except very small ones with inline tags/shortcodes, where it can rise to 50%).

@gitlost
11 years ago

Some performance tweaks.

#28 @gitlost
11 years ago

  • Keywords has-patch added; needs-patch needs-unit-tests removed

#29 @johnbillion
11 years ago

#33446 was marked as a duplicate.

#30 @wonderboymusic
11 years ago

  • Owner changed from wonderboymusic to miqrogroove
  • Status reopened → assigned

@gitlost
10 years ago

Refresh for 4.7. Includes unit tests.

#31 @gitlost
10 years ago

Refresh for WP 4.7. Includes the unit tests as well. The patch is also available as a plugin at ​pap-texturize, the download link is ​pap-texturize-1.0.0.zip. The plugin version also includes a fix for #29882 (quotes inside quotes curling incorrectly).

#32 @pento
7 years ago

#47724 was marked as a duplicate.

#33 @slamorte
7 years ago

For those that can't patch core or don't want to, I have a simple content filter that solves a common issue: an italicized name followed by a possessive, e.g. "As reported in CNN's latest article."

$content = preg_replace('#>&\#8216;s\b#', '>&#8217;s', $content);

This ticket was mentioned in ​Slack in #core-editor by lkraav. ​View the logs.


6 years ago

#35 @ellatrix
3 years ago

#48602 was marked as a duplicate.

#36 @ellatrix
3 years ago

#40148 was marked as a duplicate.

#37 @dpknauss
4 months ago

This ancient wp_texturize() issue is still reproducible on trunk today.

The core problem is unchanged from the original report: when a straight quote or apostrophe immediately follows a closing inline HTML tag, wptexturize() has lost the word context across the tag boundary and curls it as an opening quote.

// Current output (Wrong):
wptexturize( "<strong>He</strong>'s here." );
// <strong>He</strong>&#8216;s here.
wptexturize( "The word is '<a href=\"http://example.com/\">quoted</a>'" );
// …&#8216;<a …>quoted</a>&#8216;
// Desired output (Correct):
//   <strong>He</strong>&#8217;s here.
//   The word is &#8216;<a …>quoted</a>&#8217;

Proposed patch

The attached patch keeps the existing wptexturize() tokenizer and adds a small amount of cross-token state: it records when the preceding text ended in a word/sentence/closing-quote context, and when the next token is a quote immediately after a closing inline element, it classifies that quote as closing (apostrophe or closing double quote) instead of opening.

It deliberately does not revive the old strip / format-text-only / reinsert replacement engine from the earlier patch series (comments around #19 and the wptexturize_replace_*() routines). That engine was the historical performance concern. This change avoids that barrier entirely — no extra passes, no string reconstruction — and the only hot-path addition is a per-token check guarded by a cheap character test, in the same spirit as the existing str_contains( $curl, "'") guards.

Performance tests in two local environments — a WordPress 7.0 site and a wordpress-develop trunk checkout (Trac: [62536] Github: ​e269998) — both showed the fix adds only a small, sub-millisecond overhead on realistic content, nowhere near the cost of the old replacement-engine approach that previously stalled the ticket. (Packaged into a plugin, it performs surprisingly well, too.)

More technical details follow below under "Performance."

Coverage

All three historically discussed families are handled and pinned with tests:

  • The original report's quote-around-inline-HTML cases (end of string, period, comma, semicolon, dash, ellipsis, quote spanning text + tag, quote-then-word).
  • The later data_inline_end_tags cases, including two that the old engine itself had marked as unfixable (<em>"John"</em>'s, <em>'John'</em>'s).
  • The modern apostrophe/possessive case from Gutenberg #42345 (<strong>He</strong>'s, rock'n'roll, entity-adjacent input).

Four new methods are added to Tests_Formatting_wpTexturize:

  • test_historic_quotes_around_inline_html
  • test_historic_texturize_around_html_cases
  • test_historic_apostrophe_after_inline_formatting_tag
  • test_historic_inline_tag_quote_requires_adjacency

The full focused class passes locally against current trunk, and PR (​https://github.com/WordPress/wordpress-develop/pull/12249 #12249) runs it in CI — which should surface the same results:

OK (361 tests, 469 assertions)

PHPCS is clean against the WordPress-Core ruleset (phpcs.xml.dist):

... 2 / 2 (100%)

Deliberate behaviour and scope

  • Adjacency rule. A quote immediately after a closing inline tag is treated as closing; a space before the quote keeps normal opening-quote behaviour (<strong>He</strong> 'go' stays &#8216;go&#8217;). This resolves the inherent ambiguity in the way the original report expects.
  • Excluded on purpose: kbd (core already treats it as a no-texturize tag), deprecated acronym, and ] from the trailing-context class. Including ] flipped the existing "crazy" shortcode fixture's expected output, so the current behaviour is preserved. The replacement engine and its low-level unit tests are not ported, and the quote-inside-quote behaviour tracked in #29882 is out of scope here.

Performance

The patch adds a fast-path helper so the common case is a direct character test rather than a Unicode regex per text token. Local microbenchmarks (not canonical core numbers — they exclude the rest of the filter/template stack) show per-call overhead in line with normal wptexturize() work on realistic content, and nothing resembling the old replacement-engine cost profile. These are best read as structural evidence that the change stays on wptexturize()'s existing single-pass shape.

A narrower alternative option

If maintainers prefer a smaller first step, a minimal variant also exists that fixes only the apostrophe-after-closing-inline-tag case (no double-quote / quote-around-link handling). I can attach it if that scope is preferred. A rendered-output plugin prototype also exists as a proof-of-concept workaround, but the correct home is core, which is why the patch is offered here. This work was assisted by two AI models, all human-directed and reviewed.

For typography nerds and others interested in the broader history of this ticket: ​https://www.liquidweb.com/blog/apostrophes-and-quotation-marks/ I'd love to make this old post of mine obsolete!

Last edited 4 months ago by dpknauss (previous) (diff)

@dpknauss
4 months ago

Verified: targets src/wp-includes/formatting.php, 4 test methods, both helpers (incl. the fast-path).

This ticket was mentioned in ​PR #12249 on ​WordPress/wordpress-develop by ​@dpknauss.


4 months ago
#38

  • Keywords has-unit-tests added

## What

wptexturize() curls a straight quote/apostrophe to an *opening* quote when it immediately follows a closing inline tag, because it has lost the word context across the tag boundary:

  • <strong>He</strong>'s here. → …</strong>&#8216;s (should be &#8217;s)
  • The word is '<a href="…">quoted</a>' → trailing &#8216; (should be &#8217;)

This resurfaced in WordPress/gutenberg#42345, which was closed correctly as a core wptexturize() issue.

## How

Keeps the existing tokenizer and adds a small amount of cross-token state: when a quote token immediately follows a closing inline tag and the preceding text ended in a word/sentence/closing-quote context, the quote is classified as closing. The historical strip/format/reinsert replacement engine is intentionally not revived. A fast-path helper keeps the common case off the per-token Unicode regex.

## Scope / deliberate choices

  • A space before the quote keeps normal opening-quote behavior (<strong>He</strong> 'go' stays &#8216;go&#8217;).
  • Excludes kbd (already a no-texturize tag), deprecated acronym, and ] from the trailing-context class (including ] changed the existing "crazy" shortcode fixture, so current behavior is preserved).
  • Out of scope: the quote-inside-quote behavior tracked in Trac #29882.

## Testing

Adds four methods to Tests_Formatting_wpTexturize covering the original report's quote-around-inline-HTML cases, the later data_inline_end_tags cases (including two the old engine marked unfixable), and the modern apostrophe case. Full focused class is green on current trunk:

OK (361 tests, 469 assertions)

PHPCS is clean against the WordPress-Core ruleset (phpcs.xml.dist).

A minimal apostrophe-only variant exists if maintainers prefer a narrower first step.

---

Developed with AI assistance (Anthropic Claude via Claude Code, and OpenAI Codex) under maintainer review.

#39 @miqrogroove
4 months ago

Interesting. So instead of assuming we can't parse around elements, assume the set of elements to parse around is limited to the closing end of an element that is by default inline rather than block. This ignores cases involving comments, creative CSS, and some weird inline situations such as trying to use a different font for the quotes.

The one question on my mind is about the new regex in _wptexturize_is_inline_closing_tag. Does that duplicate some other code in wptexturize and/or kses? If so, can it be de-duplicated or at least noted as such?

#40 @dpknauss
4 months ago

No duplication with wptexturize() — it never parses tag names; it just splits text from opaque <…> tokens. It does overlap wp_kses_split2(), which already pulls an element name out of a tag, but that's a private helper and far broader (open/close tags, attributes, custom elements)... so calling kses here would be way too much and come with a big performance cost.

The inline-tag list has no counterpart to share either; AFAIK the closest thing in core is the block-level list in wpautop(), which is its inverse.

#41 @miqrogroove
4 months ago

Yeah that's good context for what's being duplicated. If it can't be de-duplicated, then I'd like to drill down on the performance implications. That function looks readable and easy to use, but is the regex even necessary? The function's only use comes inside a if ( '<' === $first ) { block, so it seems a little bit redundant to use ^<\/([a-z] inside an expensive preg call. I have two different ideas for improving this.

I'm thinking substr is adequate to check the first 2 chars are identical to </ and then a 2nd substr can grab the middle phrase minus the last char. This eliminates [a-z0-9]* which is greedy and more generic than necessary in this case.

If the overriding concern is allowing for whitespace inside of a closing tag, then what I would do instead is replace all of [a-z][a-z0-9]* with a more concise [a-z]++ for better performance. Likewise, I would replace \s* with \s*+ to make it possessive. This helps to reduce backtracking in PCRE without regard for whether or not PCRE is optimizing the anchors internally. This has been a non-trivial requirement in previous versions.

#42 @dpknauss
4 months ago

Ha, you're right — the best regex is no regex.

That helper only ever runs on tokens that start with <, and the list of allowed inline tags already checks the tag name. So the pattern was doing work that was already covered. I removed the preg_match() and just read the name out directly:

if ( '</' !== substr( $text, 0, 2 ) || '>' !== substr( $text, -1 ) ) {
    return false;
}
$tag = strtolower( rtrim( substr( $text, 2, -1 ), " \t\n\r\f\x0B" ) );
return in_array( $tag, $inline_tags, true );

Now the allowed-tags list is the only thing validating the name. Two details are kept on purpose: rtrim() still allows spaces before the > (like </strong >), and because I don't trim the front, a space right after </ (like </ strong>) is still rejected — same as before.

The output doesn't change — the full Tests_Formatting_wpTexturize suite still passes (361 tests, 469 assertions). It's marginally faster too (~9% on that guard in a microbenchmark), but that's a tiny saving and negligible at the wptexturize() level, since the helper only runs after the existing context check. Simpler code, not a big speed boost.

I didn't use the possessive-quantifier idea ([a-z]++, \s*+) — that only helps if you keep the regex, and dropping it is simpler and a little faster. (However: [a-z]++ by itself would stop matching numbered tags like h1. That's fine here because the allowed list is all letters, but worth noting.)

Updated in the PR: ​https://github.com/WordPress/wordpress-develop/pull/12249

#43 follow-up: @sabernhardt
4 months ago

  • Milestone → Future Release

The apostrophe (single quote) had a separate ticket, #43810, but both quotes probably belong on the same ticket.

#44 in reply to: ↑ 43 @dpknauss
4 months ago

Replying to sabernhardt:

The apostrophe (single quote) had a separate ticket, #43810, but both quotes probably belong on the same ticket.

#43810 lumps two different issues together. The first is the same as the one here on this ticket. The second is different. I left a comment there.

Here's a good diagnostic breakdown that clears the confusion over what we're looking at in different scenarios:

  1. Am I looking at the block editor or the front end? This tells you whether wptexturize is even involved (it only runs on the front end).
  2. Am I looking at characters next to a tag edge or not? This tells you whether it's this bug (#18549) or not.

Wrong-looking curly quote?

  1. WHERE do you see it?
  • Only in the editor, fine on the front end → not this bug. (Editor doesn't curl quotes; nothing's actually wrong.)
  • On the front end / Preview, but looked fine in the editor → it's wptexturize. Go to 2.
  1. Is the quote right up against a tag edge? (E.g. just after </strong> or just inside <strong> — typically from bolding.)
  • YES → this bug: context lost across the tag boundary.
    • after a CLOSING tag (</strong>'s) → fixed by #18549 / PR12249 (GitHub)
    • after an OPENING tag (<strong>'ve) → the open-tag follow-up
  • NO → probably normal curling of an ambiguous quote. (E.g. an apostrophe right after a space) → not this bug.

Also: Look at actual underlying characters in play — many fonts deliberately obscure the differences between "straight" and "curled" apostrophes, single and double quotes. The root problem behind this ticket is the English language and keyboard convention of using a single apostrophe for both left and right single quotes. The character by itself provides no context. And sometimes people use an incorrect but visually similar character:

' ‘ ’ ` ´ ′ ″ " 〃 ‴ ⁗

To cope with situations like '7' versus 7', wptexturize_primes() resolves them into ‘7’ and 7′, but it can't handle 5'6" — that becomes 5’6″. The first (foot) prime is mistaken for a right single quote or apostrophe while the second double (inches) prime is correctly interpreted.

Last edited 3 months ago by dpknauss (previous) (diff)

#45 @miqrogroove
4 months ago

This is good work. Yes, the performance change might seem small, but it also scales better in unusual cases.

Note: See TracTickets for help on using tickets.