pkp/pkp-lib#7916 Replaced html purifier with symfony html sanitizer #9002
Conversation
touhidurabir
force-pushed
the
i7916_main
branch
2 times, most recently
from
a91dde9 to
fd9be0c
Compare
There was a problem hiding this comment.
Thanks, @touhidurabir! I've given this a first review and added some comments. Could you move the work on the php-jwt update over to a separate PR? I've added some review comments separately on that to #9110.
classes/core/PKPString.php
Outdated
| $config->set('HTML.Allowed', Config::getVar('security', $configKey)); | ||
| $config->set('Cache.SerializerPath', 'cache'); | ||
| $purifier = new HTMLPurifier($config); | ||
| if (!$input) { |
There was a problem hiding this comment.
This is strictly supposed to deal with a null case, right? If so, then please use an exact test:
if ($input === null) {
return '';
}Otherwise, non-string values will get coerced into strings by the function's type hint and the if will not behave like it looks at first glance.
classes/core/PKPString.php
Outdated
| static $configKey; | ||
| static $allowedTagToAttributeMap; | ||
|
|
||
| if ($configKey !== $key) { |
There was a problem hiding this comment.
This is meant to cache repeat calls with the same $configKey, right? We only have two different configurations for $configKey we use in practice at the moment; at the moment flipping back and forth between two values of $configKey will cause the parsing to be repeated. I'd suggest storing the parsed data in an array by $configKey.
There was a problem hiding this comment.
Moved both the attributes tag map and sanitizer as static cache as both needed at the end .
classes/core/PKPString.php
Outdated
| }); | ||
| } | ||
|
|
||
| if(!isset($sanitizer)) { |
There was a problem hiding this comment.
This will instantiate a sanitizer for the first value of $configKey, and after that it will be re-used regardless of whether the $configKey matches.
There was a problem hiding this comment.
Moved both the attributes tag map and sanitizer as static cache as both needed at the end .
This PR aims to provide solution for #7916