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

Render Note field tokens correctly - they are already HTML. #13283

Merged
merged 1 commit into from
Mar 6, 2019

Conversation

aydun
Copy link
Contributor

@aydun aydun commented Dec 14, 2018

Overview

Extracted from #13174 and #12012 Small bug fix so that tokens of Note fields are rendered correctly. They are already HTML so don't convert them to HTML again.

@civibot
Copy link

civibot bot commented Dec 14, 2018

(Standard links)

@civibot civibot bot added the master label Dec 14, 2018
@@ -225,6 +226,10 @@ public function fill($format = NULL) {
if ($entity == 'activity' && $field == 'details') {
$htmlTokens[$entity][$field] = $value;
}
elseif (\CRM_Utils_Array::value('data_type', \CRM_Utils_Array::value($field, $entityFields['values'])) == 'Memo') {
// Memo fields aka custom fields of type Note are html.
$htmlTokens[$entity][$field] = $value;
Copy link
Contributor

Choose a reason for hiding this comment

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

@seamuslee001 Could you review from a security perspective? Are we ok to insert html directly into tokens?

Copy link
Member

@colemanw colemanw Jan 22, 2019

Choose a reason for hiding this comment

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

@aydun this is more likely to be considered secure if you use the CRM_Utils_String::purifyHTML() function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @colemanw - changed now

Copy link
Contributor

Choose a reason for hiding this comment

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

@colemanw @seamuslee001 with this change is it mergeable?

@aydun
Copy link
Contributor Author

aydun commented Jan 25, 2019

Hmm - the test is not failing locally.

Jenkins retest please.

@aydun
Copy link
Contributor Author

aydun commented Jan 30, 2019

test this please

@aydun aydun mentioned this pull request Feb 25, 2019
@seamuslee001
Copy link
Contributor

Given Aydun has responded to Coleman's comment and it looks sensible to me i'm good with merging this

@seamuslee001 seamuslee001 merged commit d03484b into civicrm:master Mar 6, 2019
@aydun
Copy link
Contributor Author

aydun commented Mar 7, 2019

Thanks @seamuslee001

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

Successfully merging this pull request may close these issues.

5 participants