Hi I have reviewed the Feeds 7.x-2.0-alpha8 and found its not following the drupal coding standard. So I have planed to create a patch on that and will fix all the possible issues.

Best Practices (Coding Standards)

Coding standard issues on feeds 7.x-2.0-alpha8
feeds.rules.inc

Line 40: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms [style_string_spacing]
   $info['feeds_import_'. $importer->id] = array(
Line 55: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms [style_string_spacing]
     $info['feeds_import_'. $importer->id]['variables'][$entity_type]['bundle'] = $processor->bundle();

feeds_ui.admin.inc
Line 1135: Using eval() or drupal_eval() in your module's code could have a security risk if the PHP input provided to the function contains malicious code. (Drupal Docs) [security_19]
 eval($form_state['values']['importer']);

common_syndication_parser.inc
Line 88: do not use mixed case (camelCase), use lower case and _ [style_camel_case]
function _parser_common_syndication_atom10_parse($feed_XML) {
Line 95: do not use mixed case (camelCase), use lower case and _ [style_camel_case]
 $base = $feed_XML->xpath("@base");
Line 102: do not use mixed case (camelCase), use lower case and _ [style_camel_case]
 $parsed_source['title'] = isset($feed_XML->title) ? _parser_common_syndication_title("{$feed_XML->title}") : "";
Line 104: do not use mixed case (camelCase), use lower case and _ [style_camel_case]
 $parsed_source['description'] = isset($feed_XML->subtitle) ? "{$feed_XML->subtitle}" : "";
Line 106: do not use mixed case (camelCase), use lower case and _ [style_camel_case]
 $parsed_source['link'] = _parser_common_syndication_link($feed_XML->link);
Line 113: do not use mixed case (camelCase), use lower case and _ [style_camel_case]
 foreach ($feed_XML->entry as $news) {
Line 189: do not use mixed case (camelCase), use lower case and _ [style_camel_case]
   if (!empty($feed_XML->author->name) && !$author_found) {
Line 245: do not use mixed case (camelCase), use lower case and _ [style_camel_case]
function _parser_common_syndication_RDF10_parse($feed_XML) {
Line 261: do not use mixed case (camelCase), use lower case and _ [style_camel_case]
 $namespaces = $feed_XML->getNamespaces(TRUE);
Line 264: do not use mixed case (camelCase), use lower case and _ [style_camel_case]
 foreach ($feed_XML->children($canonical_namespaces['rss'])->channel as $rss_channel) {
Line 275: do not use mixed case (camelCase), use lower case and _ [style_camel_case]
 foreach ($feed_XML->children($canonical_namespaces['rss'])->item as $rss_item) {
Line 354: do not use mixed case (camelCase), use lower case and _ [style_camel_case]
function _parser_common_syndication_RSS20_parse($feed_XML) {
Line 364: do not use mixed case (camelCase), use lower case and _ [style_camel_case]
 $parsed_source['title'] = isset($feed_XML->channel->title) ? _parser_common_syndication_title("{$feed_XML->channel->title}") : "";
Line 366: do not use mixed case (camelCase), use lower case and _ [style_camel_case]
 $parsed_source['description'] = isset($feed_XML->channel->description) ? "{$feed_XML->channel->description}" : "";
Line 368: do not use mixed case (camelCase), use lower case and _ [style_camel_case]
 $parsed_source['link'] = isset($feed_XML->channel->link) ? "{$feed_XML->channel->link}" : "";
Line 371: do not use mixed case (camelCase), use lower case and _ [style_camel_case]
 foreach ($feed_XML->xpath('//item') as $news) {

FeedsHTTPFetcher.inc
Line 150: Arrays should be formatted with a space separating each element and assignment operator [style_array_spacing]
    '#size'=> 30,

FeedsProcessor.inc
Line 229: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms [style_string_spacing]
         rules_invoke_event('feeds_import_'. $source->importer()->id, $entity);

Line 819: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms [style_string_spacing]
   $message .= '<pre>' . drupal_var_export($item). '</pre>';

feeds_mapper_profile.test
Line 86: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms [style_string_spacing]
   $this->importFile('profile_import', $this->absolutePath() .'/tests/feeds/profile.csv');


parser_csv.test
Line 45: Control statements should have one space between the control keyword and opening parenthesis [style_control_spacing]
   foreach($delimiters as $delimiterType => $delimiter) {
Line 51: The $string argument to t() should not begin or end with a space. (Drupal Docs) [i18n_11]
     $this->assertFalse($parser->lastLinePos(), t('CSV reports all lines parsed, with delimiter: ') . $delimiterType);
Line 64: Control statements should have one space between the control keyword and opening parenthesis [style_control_spacing]
   foreach($delimiters as $delimiterType => $delimiter) {
Line 83: The $string argument to t() should not begin or end with a space. (Drupal Docs) [i18n_11]
     $this->assertEqual(md5(serialize($rows)), md5(serialize($control_result)), t('Batch parsed result matches control result for delimiter: ') . $delimiterType);

feeds_mapper_profile.test
Line 86: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms [style_string_spacing]
   $this->importFile('profile_import', $this->absolutePath() .'/tests/feeds/profile.csv');

parser_csv.test
Line 45: Control statements should have one space between the control keyword and opening parenthesis [style_control_spacing]
   foreach($delimiters as $delimiterType => $delimiter) {
Line 51: The $string argument to t() should not begin or end with a space. (Drupal Docs) [i18n_11]
     $this->assertFalse($parser->lastLinePos(), t('CSV reports all lines parsed, with delimiter: ') . $delimiterType);
Line 64: Control statements should have one space between the control keyword and opening parenthesis [style_control_spacing]
   foreach($delimiters as $delimiterType => $delimiter) {
Line 83: The $string argument to t() should not begin or end with a space. (Drupal Docs) [i18n_11]
     $this->assertEqual(md5(serialize($rows)), md5(serialize($control_result)), t('Batch parsed result matches control result for delimiter: ') . $delimiterType);
CommentFileSizeAuthor
#32 2342143-32-phpcs-fixes.patch55.49 KBjoelpittet
#27 feeds-coding-standards-includes-2342143-26.patch14.86 KBMegaChriz
#25 interdiff-2342143-24-25.txt7.01 KBMegaChriz
#25 feeds-coding-standards-includes-2342143-25.patch31.44 KBMegaChriz
#24 interdiff-2342143-23-24.txt7.19 KBMegaChriz
#24 feeds-coding-standards-includes-2342143-24.patch29.01 KBMegaChriz
#23 feeds-coding-standards-includes-2342143-23.patch25.72 KBMegaChriz
#21 feeds-coding-standards-features-2342143-21.patch6.97 KBMegaChriz
#18 feeds-coding-standards-2342143-18.patch67.77 KBMegaChriz
#14 interdiff-2342143-13-14.txt1.96 KBMegaChriz
#14 feeds-coding-standards-base-2342143-14.patch44.53 KBMegaChriz
#13 interdiff-2342143-12-13.txt9.7 KBMegaChriz
#13 feeds-coding-standards-base-2342143-13.patch44.51 KBMegaChriz
#12 interdiff-2342143-11-12.txt1.79 KBMegaChriz
#12 feeds-coding-standards-base-2342143-12.patch37.05 KBMegaChriz
#11 interdiff-2342143-10-11.txt2.77 KBMegaChriz
#11 feeds-coding-standards-base-2342143-11.patch36.01 KBMegaChriz
#10 interdiff-2342143-9-10.txt2.46 KBMegaChriz
#10 feeds-coding-standards-base-2342143-10.patch37.05 KBMegaChriz
#9 feeds-coding-standards-base-2342143-9.patch34.92 KBMegaChriz
#8 interdiff-2342143-6-8.txt1.94 KBMegaChriz
#8 feeds-coding-standards-2342143-8.patch85.73 KBMegaChriz
#6 interdiff-2342143-5-6.txt62.75 KBMegaChriz
#6 feeds-coding-standards-2342143-6.patch85.17 KBMegaChriz
#5 interdiff-2342143-4-5.txt21.41 KBMegaChriz
#5 feeds-coding-standards-2342143-5.patch29.97 KBMegaChriz
#4 feeds-coding-standards-2342143-4.patch8.55 KBMegaChriz
#1 2342143-feeds_best_practices.patch10.49 KBRavindraSingh
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RavindraSingh’s picture

Fixed all the issues on this patch except

feeds_ui.admin.inc

severity: criticalreview: security_19Line 1135: Using eval() or drupal_eval() in your module's code could have a security risk if the PHP input provided to the function contains malicious code. (Drupal Docs) [security_19]
  eval($form_state['values']['importer']);

We can't change or avoid to use of eval() function but it can be protected by setting the appropriate permission to user.

It is a best practice to add a new permission in your module just for using PHP so it's more clear of the security risk of assigning the permission to a user role. You should also add a warning for any form elements where the PHP input is entered.
See the more detail on https://www.drupal.org/node/715010

twistor’s picture

Priority: Normal » Minor
Status: Active » Postponed

This isn't a high priority. I don't want to go breaking people's patches because of formatting issues.

About eval(): The link you referenced is about Drupal 6. See the note "Drupal 7 uses a more generic permission (use PHP for settings) that should be used from any modules that allow a user to use PHP code in their settings pages." This is exactly what Feeds does.

RavindraSingh’s picture

Yes, I agree with you, but coding standard and best practices we should always follow. I am not forcing anyone to use the patch but many time when your project get reviewed by someone can raise a question why you didn't create a patch. Only the concern.

Next thing I always like to follow #TheBestPractices.

MegaChriz’s picture

Status: Postponed » Needs review
FileSize
8.55 KB

Reroll.

MegaChriz’s picture

Adding relevant coding standards posted coming from the interdiff of #1127696-131: Attach multiple importers to one content type (in D7) + a few other coding standards improvements.

MegaChriz’s picture

A lot more coding standards improvements.

Status: Needs review » Needs work

The last submitted patch, 6: feeds-coding-standards-2342143-6.patch, failed testing. View results

MegaChriz’s picture

Status: Needs work » Needs review
FileSize
85.73 KB
1.94 KB
MegaChriz’s picture

Version: 7.x-2.0-alpha8 » 7.x-2.x-dev
Assigned: RavindraSingh » MegaChriz
FileSize
34.92 KB

For the past few months I have occasionally worked on fixing the coding standards. Since Feeds has so many files, it would be better to break down the fixes in multiple stages: first the files at the base (like feeds.install, feeds.module, feeds.pages.inc) and then a patch for each folder.

Here is a patch for the files at the base.

Some coding standards will be ignored.

MegaChriz’s picture

MegaChriz’s picture

Issue tags: -Fixed all the coding standard errors
FileSize
36.01 KB
2.77 KB

Found a few changes that were not right. Corrected these.

MegaChriz’s picture

MegaChriz’s picture

Properly documenting callbacks following the standards on https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...
And also fixing some more coding standard issues.

MegaChriz’s picture

"Separate the @see and @ingroup sections by a blank line."
Didn't know.

  • MegaChriz committed f40a255 on 7.x-2.x
    Issue #2342143 by MegaChriz, RavindraSingh, _Geertje: Fixed most coding...

MegaChriz’s picture

Title: Feeds 7.x-2.0-alpha8 is out of coding standard » Fix coding standards
Status: Needs review » Active

Committed #14. Back to active for fixing the remaining coding standard issues.

@_Geertje is credited because of his work in #1127696-111: Attach multiple importers to one content type (in D7).

MegaChriz’s picture

Status: Active » Needs review
FileSize
67.77 KB

Automated fixes by PHP_CodeSniffer. I adjusted some of the automated fixes as I found them not always to be correct.

  • MegaChriz committed 34a1571 on 7.x-2.x
    Issue #2342143 by MegaChriz: Fixed some coding standards that could...
MegaChriz’s picture

Status: Needs review » Active

Committed #18. Back to active for the remaining code style issues.

MegaChriz’s picture

Status: Active » Needs review
FileSize
6.97 KB

Code style fixes for the two feature modules included with Feeds.

  • MegaChriz committed 6bab108 on 7.x-2.x
    Issue #2342143 by MegaChriz: Fixed coding standard issues in the feature...
MegaChriz’s picture

Committed #21.

Here is a patch with some coding standards fixes for files in the includes folder.

MegaChriz’s picture

MegaChriz’s picture

I will ignore the "Multi-line assignment not indented correctly" rule. The coding standards want me to do something like this:

$this->total
  = $this->created
    = $this->updated
      = $this->deleted
        = $this->unpublished
          = $this->blocked
            = $this->skipped
              = $this->failed
                = 0;

  • MegaChriz committed c188387 on 7.x-2.x
    Issue #2342143 by MegaChriz: Fixed most coding standard issues in files...
MegaChriz’s picture

Committed #25.

Here are some coding standard fixes for files in the plugins folder.

NWOM’s picture

I believe after one of the last commits from this issue, the following bug is now reproducible: #2943805: Multiple Warnings: Declaration Of ~ should be compatible with FeedsConfigurable

DamienMcKenna’s picture

Status: Needs review » Reviewed & tested by the community
Parent issue: » #2911291: Plan for Feeds 7.x-2.0-beta5 release

Looks good.

joelpittet’s picture

Status: Reviewed & tested by the community » Fixed

Committed the patch in #27, thanks for tackling that @MegaChriz

  • joelpittet committed f6568e6 on 7.x-2.x authored by MegaChriz
    Issue #2342143 by MegaChriz, RavindraSingh, NWOM, twistor, DamienMcKenna...
joelpittet’s picture

Tacking on another round of cleanup --patch added after autofixing most with phpcbf --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml .

I'm posting to smoke test any issues through testbot before committing this as well.

  • joelpittet committed 56ac6cc on 7.x-2.x
    Issue #2342143 by MegaChriz, RavindraSingh, joelpittet, twistor,...
joelpittet’s picture

Status: Needs review » Fixed

Smoke test worked. PHP 7 tests are failing due to date module

Status: Fixed » Closed (fixed)

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