Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Original report:
Potential problem: FAPI element '#description' only accept filtered text. Hence, check_plain() missing from Line # 85 of modules/aggregator/aggregator.processor.inc
Adding a patch for the same.
----
Problem: No description is shown on the "Default processor settings" fieldset.
Solution: ensure the same description appears in D7 as in D8.
Comments
Comment #2
sandip27 CreditAttribution: sandip27 at Cybage Software Pvt Ltd. commentedComment #3
sandip27 CreditAttribution: sandip27 at Cybage Software Pvt Ltd. commentedComment #4
sandip27 CreditAttribution: sandip27 at Cybage Software Pvt Ltd. commentedComment #5
stefan.r CreditAttribution: stefan.r commentedIt looks to me like
$info = module_invoke('aggregator', 'aggregator_process', 'info');
was supposed to say:
$info = module_invoke('aggregator', 'aggregator_process_info')
...which returns a simple hardcoded string in $info['description']. No need to check_plain that.
Comment #6
sandip27 CreditAttribution: sandip27 at Cybage Software Pvt Ltd. commented@stefan.r, apparently you were correct, however, it still needs check_plain as per Drupal Coding Standards. Hence, providing a updated patch. Please review.
Thanks
Comment #8
stefan.r CreditAttribution: stefan.r commentedHmm where did you get that information (that all #descriptions need a check_plain)?
aggregator_process_info
Comment #9
sandip27 CreditAttribution: sandip27 at Cybage Software Pvt Ltd. commentedAs per one of helper module "coder" mentioned in here, https://www.drupal.org/coding-standards#helpermod It says, One should either use check_plain(), filter_xss() or similar to ensure the $variable is fully sanitized.
Thanks and appreciate your comments about the same.
Comment #10
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedSince this comes from a hook implementation (not from user input) I would think we don't want a check_plain() here since it would risk double-escaping. The example at https://api.drupal.org/api/drupal/modules!aggregator!aggregator.api.php/... shows that the description is translated text, so if there is user input involved it would be the developer's responsibility to sanitize that via normal t()-placeholders (@variable, %variable, etc). I guess it wouldn't hurt putting a note about that in the hook documentation though.
Comment #11
stefan.r CreditAttribution: stefan.r commented@sandip in my opinion the coder module is wrong in this particular case -- there is nothing to sanitize, since it's a string wrapped in t() without any user input:
t('Creates lightweight records from feed items.'),
Comment #12
sandip27 CreditAttribution: sandip27 at Cybage Software Pvt Ltd. commentedThank you @David and @Stefan. Please find attached updated patch to fix the call to correct hook.
Comment #13
stefan.r CreditAttribution: stefan.r commentedIt's a small UI change, looks OK to me. I guess this has never actually been right in D7, but it has already been fixed in D8.
Screenshots for reference:
Before:
After:
Drupal 8:
Comment #14
stefan.r CreditAttribution: stefan.r commentedComment #15
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedLooks great to me and is ready for commit.
My only question is:
Was the string translated even though it did not show up before or is this technically a new string?
Comment #16
stefan.r CreditAttribution: stefan.r commentedComment #18
stefan.r CreditAttribution: stefan.r commentedCommitted and pushed to 7.x, thanks!
Comment #19
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedIt might be a new string on Drupal sites, but it should have been on localize.drupal.org all along I think.