Opened 6 years ago
Last modified 5 months ago
#4539 reopened defect (bug)
Abbreviated year followed by punctuation or markup doesn't texturize
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| Priority: | low | Milestone: | Future Release |
| Component: | Formatting | Version: | 2.5 |
| Severity: | normal | Keywords: | has-patch westi-likes 3.4-early |
| Cc: | jon@…, norbertm, bjo |
Description
An abbreviated year followed by punctuation or markup doesn't texturize properly.
e.g. (Bruce Sterling, '97) is texturized as (Bruce Sterling, ‘97) when the apostrophe should be texturized as ’
e.g. <li>Casino Royale '06</li> is texturized as <li>Casino Royale, ‘06</li> when the apostrophe should be texturized as ’
Attachments (14)
Change History (68)
- Keywords has-patch added
- Owner changed from anonymous to jmstacey
- Status changed from new to assigned
- Version changed from 2.2.1 to 2.5
- Keywords needs-patch needs-testing added; has-patch removed
I tried applying this against trunk 10289 but it didn't seem to fix the issue for me.
- Keywords has-patch tested added; needs-patch needs-testing removed
my mistake, I was testing against the wrong site. D'oh.
The attached fixes the curly quotes and s and the issue above.
- Component changed from General to Formatting
- Keywords needs-patch added; has-patch tested removed
broken patch
- Milestone changed from 2.9 to Future Release
Added Test cases for this to wordpress-tests.
Moving to Future Release until it has a working patch
- Cc jon@… added
- Keywords has-patch needs-testing added; needs-patch removed
Here's another go ready for testing. This patch was created against r14075.
comment:10
in reply to:
↑ 9
westi — 3 years ago
comment:11
westi — 3 years ago
- Milestone changed from Future Release to 3.0
comment:12
westi — 3 years ago
I ran the unit tests against the patch (http://svn.automattic.com/wordpress-tests/wp-testcase/test_includes_formatting.php) with the following results:
- TestWPTexturize::test_quotes_before_s - these tests now pass
- TestWPTexturize::test_quotes - still fail
- TestWPTexturize::test_quotes_before_numbers - simple case passes quoted case doesn't.
So we are heading in the right direction but still need some more work.
I'll commit the current patch but leave the ticket open for improvements.
comment:13
westi — 3 years ago
comment:14
scribu — 3 years ago
- Priority changed from normal to low
comment:15
nacin — 3 years ago
- Keywords needs-patch added; has-patch needs-testing removed
- Milestone changed from 3.0 to 3.1
Going to punt this along for continued work.
comment:16
norbertm — 3 years ago
Working on a patch that makes all failing test cases pass.
comment:17
norbertm — 3 years ago
I was able to make 2 of the failing test cases pass by reorganizing the order of patterns in r16195.
Needs another patch for test_quotes() with HTML tags and tag attributes in it.
comment:18
norbertm — 3 years ago
- Cc norbertm added
comment:19
norbertm — 3 years ago
Technically related to #15241.
comment:20
norbertm — 3 years ago
Almost done with the simplified patch that resolves all issues.
comment:21
norbertm — 3 years ago
Submitted a patch above with regexps rewritten from scratch that makes all existing test cases pass, enables disabled test cases to run, including skipped tests and I also added several new cases that all pass.
The only edge case it does not handle is when you want to combine units of degrees, specifically minutes and seconds with the same class of quotes, e.g. ' with ' and " with ". I don't think that can correctly be fixed.
The patch also adds support for quotes spanning multiple html tags and/or shortcodes.
It also fixes #15241.
comment:22
scribu — 3 years ago
- Keywords has-patch added; needs-patch removed
comment:23
westi — 3 years ago
comment:24
westi — 3 years ago
I switched the escaped slashes back to using a double quoted string for the string with single quotes in in test_other_html so as to follow the WP Coding Standards.
comment:25
westi — 3 years ago
- Resolution set to fixed
- Status changed from accepted to closed
comment:26
PeteMall — 3 years ago
- Keywords has-patch removed
- Milestone changed from Awaiting Triage to 3.1
comment:27
nacin — 3 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
See my comment in #15241.
comment:28
nacin — 3 years ago
comment:29
norbertm — 3 years ago
comment:30
norbertm — 2 years ago
Added new snapshot.
Includes fix for #15444.
Follows WP coding standards.
Improved test coverage.
comment:31
nacin — 2 years ago
Cross-referencing #10606 as another one of those texturize bugs.
comment:32
follow-up:
↓ 33
norbertm — 2 years ago
Updated patch and included further tests. Fixes #10606 as well.
Aiming to improve performance by getting rid of the two preg_replaces() within the while()s with no success so far (we need the count parameter that str_replace() does not have). Is this a performance sensitive function or cached?
comment:33
in reply to:
↑ 32
westi — 2 years ago
Replying to norbertm:
Is this a performance sensitive function or cached?
The output of this isn't cached
comment:34
follow-up:
↓ 35
norbertm — 2 years ago
Let me see if I can further optimize it.
Any other tickets/concerns related to this?
comment:35
in reply to:
↑ 34
nacin — 2 years ago
comment:36
nacin — 2 years ago
Another: #14491.
comment:37
norbertm — 2 years ago
Thanks for collecting related tickets. Here is another patch.
- No change required for #4116. Added tests.
- Added tests for #6969 (incomplete).
- Fixes HTML comments (#8912, #10033). Tests included.
- #15241, #15444 already covered.
Waiting for feedback on #14491.
Optimized code for performance by replacing two preg_replace()s to str_replace()s (and while()s to if()s).
WPTexturize test results: OK (21 tests, 130 assertions)
I think it's getting mature. Will post a followup once I'm done.
Any other thoughts?
comment:38
norbertm — 2 years ago
Working in #14491 which needs some rethinking.
comment:39
norbertm — 2 years ago
Next dev snapshot. Adds test cases and fix for #14491.
There is one issue left. How do you convert
'Class of '99'
into
&8216;Class of &8217;99&8217;
and not
&8216;Class of &8216;99&8217;
if you still want to convert '99' into &8216;99&8217;?
Obviously it depends on the starting quote but that's a leftover quote to be evaluated later to allow quote state to persist across tags and shortcodes. Still thinking about this.
comment:40
norbertm — 2 years ago
comment:41
norbertm — 2 years ago
Completed the review. I think it can't become much better than this. It's pretty well covered with tests too. If you have suggestions please post it, otherwise I think the patch could be applied and the fully covered tickets mentioned in the second post above can be closed together with this.
comment:42
nacin — 2 years ago
How's the performance?
comment:43
nacin — 2 years ago
- Keywords has-patch added
comment:44
xibe — 2 years ago
Then I'll paste my test-post from #10606, simply to present that some mundane punctuation uses still just don't work (only for closing quotes):
- text "[link]".
- text "[link]", text.
- text "[link]"; text.
- text "[link]"- text.
- text "text [link]".
- text "text [link]"...
- text "text [link]"text.
All these have the opening quote applied instead of the closing quote (and then the locale is applied, the closing quote in fr-FR being "« ").
- text "text number".
- text "text number" text.
These last two have the correct closing quote applied, but the locale is not taken into account.
I did provide test cases for all of these, a year ago now. If they're wrong, let me know about it, I'll update them.
Test blog is running WordPress 3.1-beta1-16732.
comment:45
norbertm — 2 years ago
@xibe - It hasn't been committed to SVN yet. The patch is available above. It makes your test cases submitted in #10606 pass. To make sure I've included new tests for the simplified strings that you just submitted. All pass now.
comment:46
norbertm — 2 years ago
@nacin - As for performance, it's hard to tell because it highly depends on the strings that are passed to it. The original test suite did not pass and many tests were skipped so there's nothing to compare against. What would you suggest for benchmarking this? Please keep in mind that the original function returns incorrect results so we may be comparing code that generates incorrect vs correct output.
As for optimization, I was able to replace a preg_replace() to an str_replace() and a while() loop to a single if() by realizing the same could be accomplished with these better performing functions/constructs.
comment:47
westi — 2 years ago
- Keywords 3.2-early added
- Milestone changed from 3.1 to Future Release
@norbertm: Thanks for all your hard work on this.
I think it is best left for implementation in 3.2 so we get a good deal of time for the changes to soak in trunk.
comment:48
westi — 2 years ago
#9306 should be reviewed when the changes here are reviewed and committed.
comment:49
azaozz — 2 years ago
See #8912 re: commented HTML in the post content.
comment:50
westi — 18 months ago
- Keywords westi-likes 3.4-early added; 3.2-early removed
comment:51
xibe — 16 months ago
Hello!
May I ask for renewed interest in this ticket? Is that patch from norbertm 1 year ago still applicable (somehow I doubt it)? May I help in any way (not much of a coder, but I can add more tests to my batch from #11099)?
Thanks all!
comment:52
xibe — 15 months ago
Tiny patch for those situation where a quote ends with digits.
comment:53
bjo — 7 months ago
- Cc bjo added
I still have the issue with the wrong quotes in the case
text "[link]", text.
comment:54
SergeyBiryukov — 5 months ago
Related: #22971

Note that the patch I attached also includes the patch I created for Ticket #1258.