Skip to content
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

(dev/core#3436) MultipleRecordFieldsListing.tpl - JS strings should us JS escaping #23499

Merged
merged 1 commit into from
May 20, 2022

Conversation

totten
Copy link
Member

@totten totten commented May 19, 2022

Overview

Suppose you make a multi-record custom-data group with a funny name

Screenshot from 2022-05-18 21-53-38

The good thing is that doesn't execute this. But it does do excessive escaping in one place.

Before

Most widgets+screens showing this name will display characters like < as <. Notice the tab-bar and the button-bar.

However, there's an outlier in the middle which displays < as &lt;:

Screenshot from 2022-05-18 21-54-37

After

Like everywhere else, the center of the page displays < as <. (It looks the same as when it was typed.)

Screenshot from 2022-05-18 21-55-37

Comments

Noticed this well reading https://lab.civicrm.org/dev/core/-/issues/3436, though it's a bit tangential to that problem.

@civibot
Copy link

civibot bot commented May 19, 2022

(Standard links)

@civibot civibot bot added the master label May 19, 2022
@totten
Copy link
Member Author

totten commented May 19, 2022

This also simplifies list of strings outputted for the file. Before, civistrings reported two similar strings:

#: templates/CRM/Profile/Page/MultipleRecordFieldsListing.tpl
msgid "No records of type \\'%1\\' found."
msgstr ""

#: templates/CRM/Profile/Page/MultipleRecordFieldsListing.tpl
msgid "No records of type '%1' found."
msgstr ""

Now it just reports the cleaner string.

@totten
Copy link
Member Author

totten commented May 19, 2022

Actually, this may also fix dev/core#3436. I'm not very experienced with Transifex, so I'm not sure how to identify which variants of the strings were at play in the problem-report... but... I tried hacking my system to force/mock some funny characters:

diff --git a/CRM/Core/I18n.php b/CRM/Core/I18n.php
index d1da1fe698..4834f4f0f8 100644
--- a/CRM/Core/I18n.php
+++ b/CRM/Core/I18n.php
@@ -796,6 +796,10 @@ function ts($text, $params = []) {
   static $i18n = NULL;
   static $function = NULL;
 
+  if ($text === "No records of type '%1' found." || $text === "No records of type \\'%1\\' found.") {
+    $text = "No records'quote<b>\" e '%1' found."; // Hacky override of translation
+  }
+
   if ($text == '') {
     return '';
   }

With this funny translation, I could compare before/after:

  • Before this patch, the table is always empty, and there are JS errors
    Screenshot from 2022-05-18 22-39-48
  • After this patch, the table appears properly; no JS errors
    Screenshot from 2022-05-18 22-39-31

@totten totten changed the title MultipleRecordFieldsListing.tpl - JS strings should us JS escaping (dev/core#3436) MultipleRecordFieldsListing.tpl - JS strings should us JS escaping May 19, 2022
@@ -33,7 +33,7 @@
{literal}
<script type="text/javascript">
(function($) {
var ZeroRecordText = {/literal}'{ts 1=$customGroupTitle|escape}No records of type \'%1\' found.{/ts}'{literal};
var ZeroRecordText = {/literal}"{ts escape='js' 1=$customGroupTitle}No records of type '%1' found.{/ts}"{literal};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you might as well make it {ts escape='js' 1=$customGroupTitle|smarty:nodefaults} - since it's a good guess this would break under strict smarty too

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eileenmcnaughton Good idea. I've r-run with that and force-pushed.

Also, the branch should be amenable to 5.50, so I'll switch it.

@totten totten changed the base branch from master to 5.50 May 19, 2022 21:48
@civibot civibot bot added 5.50 and removed master labels May 19, 2022
@eileenmcnaughton
Copy link
Contributor

As discussed, we should port this to 5.49 & it can go out in any point releases that happen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants