![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png){kind=logo}
Opened 15 years ago
Last modified 4 weeks 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 “<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
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)
Change History (54)
#2
@
15 years ago
- Cc 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
@
15 years ago
- Version 3.3.1 → 3.2.1
Version number indicates when the bug was initially introduced/reported.
#6
@
13 years ago
- Keywords needs-patch needs-unit-tests added
- Milestone Awaiting Review → Future Release
Unit tests in particular would move this along.
#7
@
12 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
#10
@
12 years ago
- Resolution fixed
- Status closed → reopened
Ticket still valid. Just didn't need the broken tests.
#12
@
12 years ago
Were those tests actually broken? Because they seemed to be testing still-outstanding texturize issues.
#13
@
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
@
12 years ago
Okay. In the future, failing tests should be broken out into individual tests and marked with @ticket for the open ticket.
#15
@
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()
#19
@
11 years ago
Algorithm brainstorm:
- Make a string that is stripped of all elements and shortcodes.
- Make an array of the elements and shortcodes, indexed by string position.
- Apply all formatting changes to the text-only string.
- Compare the formatted string to the unformatted string.
- 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
@
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.
#21
@
11 years ago
FYI, we are discussing this particular issue over at the following thread:
http://wordpress.stackexchange.com/questions/195190/apostrophe-in-a-possessive-appears-as-an-open-single-smart-quote-instead-of-a-cl
#22
@
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?
#23
@
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
@
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
@
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...
#26
@
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
@
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%).
#31
@
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).
#33
@
6 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#', '>’s', $content);
This ticket was mentioned in Slack in #core-editor by lkraav. View the logs.
6 years ago
#37
@
4 weeks 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>‘s here. wptexturize( "The word is '<a href=\"http://example.com/\">quoted</a>'" ); // …‘<a …>quoted</a>‘ // Desired output (Correct): // <strong>He</strong>’s here. // The word is ‘<a …>quoted</a>’
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_tagscases, 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_htmltest_historic_texturize_around_html_casestest_historic_apostrophe_after_inline_formatting_tagtest_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‘go’). 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), deprecatedacronym, 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!
@
4 weeks 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 weeks 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>‘s(should be’s)The word is '<a href="…">quoted</a>'→ trailing‘(should be’)
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‘go’). - Excludes
kbd(already a no-texturize tag), deprecatedacronym, 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
@
4 weeks 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
@
4 weeks 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
@
4 weeks 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
@
4 weeks 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:
↓ 44
@
4 weeks 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
@
4 weeks 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:
- Am I looking at the block editor or the front end? This tells you whether
wptexturizeis even involved (it only runs on the front end). - 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?
- 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.
- 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
- after a CLOSING tag (
- 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.
Related: #18575