I am just trying resolve a random bug so decided to run some modules through the Coder module to eliminate that as a possible issue. Saw few issues with the D7 version that would be great to get into an upcoming release.

The issues it brings up are bigger than simple spacing issues.

CommentFileSizeAuthor
#5 1078842-05-fix_coder_warnings.patch1.58 KBFreso
#3 1078842-02-fix_string_concat.patch1.19 KBFreso

Comments

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Active » Postponed (maintainer needs more info)

If you could please detail the specific problems, that would help.

mgifford’s picture

mgifford
he/him
Primary language English
commented

Certainly Dave. It's pretty easy to just enable the module & run it, but.

pathauto.inc
Line 534: Potential problem: d() only accepts filtered text, be sure to use c(), f() or similar to ensure your $variable is fully sanitized.
    drupal_set_message($message);

pathauto.admin.inc
Line 28: Potential problem: FAPI elements '#title' and '#description' only accept filtered text, be sure to use c(), f() or similar to ensure your $variable is fully sanitized.
      '#title' => $groupheader,
Line 37: Potential problem: FAPI elements '#title' and '#description' only accept filtered text, be sure to use c(), f() or similar to ensure your $variable is fully sanitized.
      '#title' => $patterndescr,
Line 55: Potential problem: FAPI elements '#title' and '#description' only accept filtered text, be sure to use c(), f() or similar to ensure your $variable is fully sanitized.
          '#title' => $itemlabel,
Line 208: Potential problem: FAPI elements '#title' and '#description' only accept filtered text, be sure to use c(), f() or similar to ensure your $variable is fully sanitized.
      '#title' => $details['name'],
Line 359: The $message argument to drupal_set_message() should be enclosed within t() so that it is translatable.
      drupal_set_message('No new URL aliases to generate.');

Maybe it's an issue with the Coder module, but either way it would be nice if it were resolved. This is a pretty essential module for the community.

Freso’s picture

Freso commented
Title: It is important to run this through Coder » Coder warning: Fix string concatenation
Priority: Normal » Minor
Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new 1078842-02-fix_string_concat.patch1.19 KB

Here's my output:

freso@nayru /s/h/l/h/drupal7> drush coder-review pathauto
sites/all/modules/pathauto/pathauto.module:
No Problems Found

sites/all/modules/pathauto/pathauto.js:
No Problems Found

sites/all/modules/pathauto/pathauto.test:
+-1: [normal] Include the CVS keyword $Id$ in each file. This should be in the format // $Id$ or // $Id$

sites/all/modules/pathauto/pathauto.api.php:
No Problems Found

sites/all/modules/pathauto/pathauto.install:
+-1: [normal] Include the CVS keyword $Id$ in each file. This should be in the format // $Id$ or // $Id$

sites/all/modules/pathauto/pathauto.admin.inc:
+-1: [normal] Include the CVS keyword $Id$ in each file. This should be in the format // $Id$ or // $Id$
+26: [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.
+35: [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.
+53: [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.
+149: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
+201: [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.
+352: [normal] The $message argument to drupal_set_message() should be enclosed within t() so that it is translatable.

sites/all/modules/pathauto/pathauto.inc:
+-1: [normal] Include the CVS keyword $Id$ in each file. This should be in the format // $Id$ or // $Id$
+498: [critical] Potential problem: drupal_set_message() only accepts filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized.

sites/all/modules/pathauto/pathauto.pathauto.inc:
+-1: [normal] Include the CVS keyword $Id$ in each file. This should be in the format // $Id$ or // $Id$

Status Messages:
Coder found 1 projects, 8 files, 5 critical warnings, 7 normal warnings, 0 warnings were flagged to be ignored

Does your result diverge greatly from this?

Well, all the CVS keyword warnings are deprecated. The "potential problems" are just that, potential. Not an issue here. In fact, the only real issue I can see from the above, is +149: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms - which isn't a very serious warning. None the less, a patch is attached.

Freso’s picture

Freso commented
Title: Coder warning: Fix string concatenation » Coder warning: Fix string concatenation and add t() call
Status: Needs review » Needs work

Ah, okay. There's a t() that needs fixing as well. New patch coming up shortly.

Freso’s picture

Freso commented
Status: Needs work » Needs review
StatusFileSize
new 1078842-05-fix_coder_warnings.patch1.58 KB
mgifford’s picture

mgifford
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Applies nicely to the repository & seems to work fine in one of my installs.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Yep looks good to me too. We'll have to make sure how much of this applies to the 6.x-2.x and 6.x-1.x branches as well.

Freso’s picture

Freso commented
Version: 7.x-1.x-dev » 6.x-2.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Alright. I pushed my commit for 7.x-1.x. It looks like Coder has some applicable issues with 6.x-2.x as well, so moving there. Are we using 6.x or 7.x style string concatenation for 6.x-2.x again?

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Coding standards apply universally across all versions, plus it's also easier to diff between branches if they're using the same concatenation spacing standard (which I should have been already using). So feel free to fix any other finds.

Freso’s picture

Freso commented

Coding standards apply universally across all versions,

Not 100% true. Up until (and including, to some degree) Drupal 6, the standard is $foo .' and some eggs' from D7 onwards, the standard is $foo . ' and some eggs'. Due to the change for 7.x, 6.x contrib's coding standard can be either one. (See #245115: Fix Drupal's awkward coding standards for the . operator.)

I'm inclined to say that we keep pre-6 concat style for 6.x-1.x, but use D7-style concat for 6.x-2.x. (If that's not what we're doing already.)

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

(we're already using the current standard in every branch)

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Committed the second hunk to 6.x-2.x dev.
http://drupalcode.org/project/pathauto.git/commit/5cebcda

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Version: 6.x-2.x-dev » 7.x-1.x-dev
Status: Patch (to be ported) » Fixed

Status: Fixed » Closed (fixed)

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

mgifford’s picture

mgifford
he/him
Primary language English
commented

Thanks Dave!