Problem/Motivation
The house style for our organization is to not include a trailing period in abbreviations (e.g. "Doing D.I.Y is fun."). However this seems not to be properly captured by the "Wrap Caps" filter, and the last initial is not wrapped.
The same problem behavior was apparently reported and fixed in version 7 here: Wrap Caps is inconsistent (2304643) - the code intended to fix the issue seems to have been carried forward to version 8, without diverging, but doesn't behave as it should (period separated abbreviations without a trailing period are not captured/wrapped), at least not in version 8. (The commentary in #2304643 includes abbreviations without trailing periods ("B.A") but those do not actually appear in the test-case, and weren't the focus of the issue, but rather interractions with punctuation, which was fixed)
Looking at the the regex on line 90 of src/Typogrify.php and testing it locally - I can see the solution is most of the way there but still looks for a period followed by possible white-space (\.\s?) - This period would not exist for the last initial of period-separated abbreviations without trailing periods hence it is not captured.
Here's the line in question:
(?:[[\p{Lu}]+\.\s?)+) # Followed by the same thing at least once more
We can make period optional on proceeding initials by appending the ? quantifier after \.
Steps to reproduce
1. Enter the following text into a new article using a text-format that has typogrify enabled with the "Wrap Caps" filter switched on.
"D.I.Y is a great way to save money on home renovation.
2. Save the article
3. Inspect element in your browser to view the rendered markup as follows
<span class="caps">D.I.</span>Y is a great way to save money on home renovation.
4. Note that the "Y" is not captured in the span.
Proposed resolution
Change line 90 of src/Typogrify.php from...
(?:[[\p{Lu}]+\.\s?)+) # Followed by the same thing at least once more
..to...
(?:[[\p{Lu}]+\.?\s?)+) # Followed by the same thing at least once more
Remaining tasks
Suggest code changes
Test locally
Create MR
Review by maintainers
Revisions/Changes
Merge
User interface changes
Rendered text before (with wrapped caps styled red)

Rendered text after (with wrapped caps still styled red)

API changes
N/A
Data model changes
N/A
| Comment | File | Size | Author |
|---|---|---|---|
| typogrify-caps-after-change.png | 5.06 KB | jacobupal | |
| typogrify-caps-before-change.png | 5.3 KB | jacobupal |
Issue fork typogrify-3540951
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
jacobupal commentedComment #4
jacobupal commentedComment #5
jacobupal commented(The failing tests on the issue branch don't seem to be related to the file I changed)
Comment #6
jacobupal commentedComment #7
benjifisher@jacobupal:
Thanks for working on this issue. I will have a look at the failing tests in a separate issue.
I would like to have test coverage for this change, mostly as a form of documentation. If you look at the bottom of
tests/src/Kernel/TypogrifySmartyPantsTest.php, it is pretty easy to add a test case toproviderTypogrifyExamples(). If the "before" text includes several abbreviations, then a single test case can cover many variations. Something like this:The test should fail with the current version of the code an pass with the updated version.
I am adding the issue tag for test coverage.
Comment #8
jacobupal commentedAdded a test using your example sentence, however it fails for the "TL.A", part:
or TL.A, itselfbecomesor <span class="caps">TL</class>.A, itselfand the ".A" is not included...I can't think of any acronyms which would actually have that format though... Usually they're either period separated or not. If there is some application for "TL.A" style acronyms, in some technical context or in another language I can look into updating the regex to accommodate it.
However, if it was just a typo, my appologies, I can adjust the test so we aren't testing for that case.
Comment #9
jacobupal commentedComment #10
benjifisher@jacobupal:
I updated the MR by merging with the latest 8.x-1.x. That updates the tests so that they work with the current version of PHPUnit.
The test you added is failing. Either the test needs updating or the code that it tests needs to be fixed. Probably the test should be fixed. As I said in Comment #7, "I would like to have test coverage for this change, mostly as a form of documentation." That is, I should be able to look at the test and see an example of "before" and "after" text, exactly as the module would transform it.
I also asked for a new test case in
TypogrifySmartyPantsTest.php. All you have to do is add another array (or row) to the data provider: the array elements are then passed as arguments totestTypogrify(). Instead, you added a new test totests/src/Unit/TypogrifyClassTest.php.In the issue summary, you wrote,
I do not think that is true. Issue #2304643 was fixed in Commit b9950a5, and the code was further modified for #2404819: small caps skips unicode uppercase characters in Commit 813123d. All of that was done before the 8.x-1.x branch was created. Based on what you wrote, I assumed that the 8.x and 7.x versions had diverged, but in fact they have the same code.
Comment #11
jacobupal commentedComment #12
jacobupal commentedWhen I said "The same problem was apparently reported and fixed in version 7" I meant that the issue history and status indicated that the desired behavior had been achieved, not that the code itself was any different, and as I said, the code itself had been carried forward. I can't account for why, even though the code was unchanged while the unwanted behavior seems to have persisted or reverted or when, if so. However, I can see why my wording could have been confusing, so I've amended the issue description for clarity.
I've also moved the test from
TypogrifyClassTest.phpto right place inTypogrifySmartyPantsTest.phpand amended the test case to reflect actual behavior.Comment #13
jacobupal commentedComment #14
benjifisherI added a test case based on the issue summary from #2304643: Wrap Caps is inconsistent and made a couple of other small changes to the tests. When I run the test-only changes in GitLab CI,
I am removing #2304643 as a related issue. I will Fix this one and soon I will release a new version of this module.
Comment #15
benjifisherComment #17
jacobupal commentedThanks for this!
(Just as a reflection: I finally figured out the confusion around how this related to the previous issue: I can see now that the OP used abreviations without trailing periods when they described the behavior ("B.A" and "B.F.A") - perhaps as a typo, perhaps as a stylistic habit and I relied on that description too much - whereas their actual example/test-case had periods at the ends of the abrevations all along ("B.A." and "B.F.A.")... so it was only their interractions with punctuation which was reported, fixed and tested).
Comment #18
jacobupal commentedComment #19
jacobupal commentedComment #21
benjifisherThe auto-generated comment is based on the commit message. The commit that fixes the issue is 3695e11.