CommentFileSizeAuthor
#2 1533448.patch2.28 KBlotyrin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lotyrin’s picture

Assigned: Unassigned » lotyrin
lotyrin’s picture

Assigned: lotyrin » Unassigned
FileSize
2.28 KB

Initial start.

Remaining output from coder:

core/modules/user/user.module:
 +993: [critical] Potential problem: FAPI elements '#title' and '#description' only accept filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized.
 +1031: [critical] Potential problem: FAPI elements '#title' and '#description' only accept filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized.

core/modules/user/user.test:
 +2076: [normal] global variables should start with a single underscore followed by the module and another underscore

core/modules/user/user.admin.inc:
 +77: [critical] Potential problem: FAPI elements '#title' and '#description' only accept filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized.

core/modules/user/user.pages.inc:
 +75: [normal] global variables should start with a single underscore followed by the module and another underscore
 +100: [critical] Potential problem: drupal_set_message() only accepts filtered text, be sure all !placeholders for $variables in t() are fully sanitized using check_plain(), filter_xss() or similar.
 +106: [critical] Potential problem: drupal_set_message() only accepts filtered text, be sure all !placeholders for $variables in t() are fully sanitized using check_plain(), filter_xss() or similar.
 +323: [critical] Potential problem: FAPI elements '#title' and '#description' only accept filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized.

Not sure what to do about the complaint about global names. I'm guessing that doesn't apply to core global space?

All of the string filtering still needs checked out. I imagine most if not all of them are false positives, but this issue is a good time to quadruple check.

lotyrin’s picture

Status: Active » Needs review
lotyrin’s picture

Status: Needs review » Needs work
tstoeckler’s picture

They are all fine. The only ones I would find debatable are:

+100: [critical] Potential problem: drupal_set_message() only accepts filtered text, be sure all !placeholders for $variables in t() are fully sanitized using check_plain(), filter_xss() or similar.
+106: [critical] Potential problem: drupal_set_message() only accepts filtered text, be sure all !placeholders for $variables in t() are fully sanitized using check_plain(), filter_xss() or similar.

Both of those cases use a !url placeholder for inserting a URL into t(). I think we usually use @url for that, so that !foo means we specifically want to allow HTML/unfiltered text, but that is only an asthetical question, as the URLs that are generated contain no unsafe output anyway. Don't know if we want to change that.

NROTC_Webmaster’s picture

As a note #1326666: Clean up API docs for user module is still open and this should not contain any docblock changes until that is fixed.

lotyrin’s picture

Assigned: Unassigned » lotyrin
lotyrin’s picture

Assigned: lotyrin » Unassigned

Unassigning because I'm not sure I'll have time for these issues and I want to make sure people know they can jump in without stepping on my toes.

Lars Toomre’s picture

As part of #1800046: [META] Add missing type hinting to core docblocks, fix Drupal.Commenting.FunctionComment.Missing*, I created a sub-issue #1800174: Add missing type hinting to User module docblocks to address the addition of missing type hinting to the User module. A first patch is posted there addressing about a dozen function docblocks. Reviews are welcome.

TravisCarden’s picture

Status: Needs work » Postponed

Postponing till feature freeze. If you want to help in the meantime, please work on the blockers on the meta issue. Thanks!

TravisCarden’s picture

Issue summary: View changes
Status: Postponed » Active
george.d.peterson’s picture

Assigned: Unassigned » george.d.peterson

Working on this in Austin

valthebald’s picture

Issue tags: +dcwroc2014

We are going to work on this issue during DCWroclaw 2014

Status: Active » Needs review

rbodych queued 2: 1533448.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 2: 1533448.patch, failed testing.

TravisCarden’s picture

Version: 8.0.x-dev » 8.1.x-dev
Assigned: george.d.peterson » Unassigned
Priority: Normal » Minor
Status: Needs work » Postponed
Issue tags: -Novice

Channeling @xjm: Thanks for all the work here so far. See #1518116-86: [meta] Make Core pass Coder Review. This issue is postponed until the meta issue is either closed or reopened.

valthebald’s picture

Issue tags: -dcwroc2014
tatarbj’s picture

Status: Postponed » Closed (duplicate)

Closing in favor of #2571965: [meta] Fix PHP coding standards in core. In this issue the coding standards will be fixed on a sniff-per-sniff basis rather than a module-per-module basis.