-
-
Notifications
You must be signed in to change notification settings - Fork 824
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix token regression - re-add support for empty tokens #21756
Conversation
(Standard links)
|
retest this please |
$variants = [ | ||
[ | ||
'string' => '{contact.individual_prefix}{ }{contact.first_name}{ }{contact.middle_name}{ }{contact.last_name}{ }{contact.individual_suffix}', | ||
'expected' => 'Mr. Anthony Anderson II', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so your hesitation is that this produces a double-space? Which would be invisible on most HTML outputs - but visible on text outputs?
I think if this produces the same output on 5.42, then I wouldn't worry about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied+trimmed the test to try it on 5.41. Same result with two spaces (for both TokenProcessor
and replaceGreetingTokens()
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @totten
jenkins, test this please |
1. You don't need to call evaluate() twice. It should be once after filling both rows+messages. 2. Add messages to clarify assertions
I had some issues where |
Overview
Fix token regression - re-add support for empty tokens since this was originally being accessed from
replaceGreetingTokens
which is no longer called bytokenCompatSubscriber
Before
{contact.first_name}{ }{contact.last_name}
leaves in{ }
After
{contact.first_name}{ }{contact.last_name}
removes{ }
Technical Details
I originally thought the behaviour was to remove those tokens when there was a string either side - the tests show that not to be working for
replaceGreetingTokens
- this syncs the behaviour of the 2 rather than trying to improve it.Comments
@totten @colemanw note this test specifically covers the 2 variants of prefix_id - but won't pass without #21705 merged