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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sandip27 created an issue. See original summary.

sandip27’s picture

sandip27’s picture

Status: Active » Needs review
sandip27’s picture

Issue summary: View changes
stefan.r’s picture

It 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.

sandip27’s picture

@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

Status: Needs review » Needs work
stefan.r’s picture

Hmm where did you get that information (that all #descriptions need a check_plain)?

+++ b/modules/aggregator/aggregator.processor.inc
@@ -72,7 +72,7 @@ function aggregator_aggregator_remove($feed) {
+    $info = module_invoke('aggregator', 'aggregator_process');

aggregator_process_info

sandip27’s picture

As 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.

David_Rothstein’s picture

Title: Potential problem: Missing check_plain() from FAPI element '#description'. » Potential problem: Missing check_plain() from FAPI element '#description', and wrong hook is invoked (hook_aggregator_process() rather than hook_aggregator_process_info())

Since 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.

stefan.r’s picture

Title: Potential problem: Missing check_plain() from FAPI element '#description', and wrong hook is invoked (hook_aggregator_process() rather than hook_aggregator_process_info()) » Potential problem: wrong hook is invoked (hook_aggregator_process() rather than hook_aggregator_process_info())

@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.'),

sandip27’s picture

Thank you @David and @Stefan. Please find attached updated patch to fix the call to correct hook.

stefan.r’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
168.13 KB
146.84 KB
147.55 KB

It'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:

stefan.r’s picture

Assigned: sandip27 » Unassigned
Fabianx’s picture

Issue tags: +Pending Drupal 7 commit

Looks 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?

stefan.r’s picture

Title: Potential problem: wrong hook is invoked (hook_aggregator_process() rather than hook_aggregator_process_info()) » Wrong hook is invoked (hook_aggregator_process() rather than hook_aggregator_process_info())

  • stefan.r committed 0c770aa on 7.x
    Issue #2774725 by sandip27: Wrong hook is invoked (...
stefan.r’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Pending Drupal 7 commit

Committed and pushed to 7.x, thanks!

David_Rothstein’s picture

Was the string translated even though it did not show up before or is this technically a new string?

It might be a new string on Drupal sites, but it should have been on localize.drupal.org all along I think.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.