Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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);
Comment | File | Size | Author |
---|---|---|---|
#32 | 2342143-32-phpcs-fixes.patch | 55.49 KB | joelpittet |
#27 | feeds-coding-standards-includes-2342143-26.patch | 14.86 KB | MegaChriz |
| |||
#25 | interdiff-2342143-24-25.txt | 7.01 KB | MegaChriz |
#25 | feeds-coding-standards-includes-2342143-25.patch | 31.44 KB | MegaChriz |
| |||
#24 | interdiff-2342143-23-24.txt | 7.19 KB | MegaChriz |
Comments
Comment #1
RavindraSingh CreditAttribution: RavindraSingh commentedFixed all the issues on this patch except
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
Comment #2
twistor CreditAttribution: twistor commentedThis 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.
Comment #3
RavindraSingh CreditAttribution: RavindraSingh commentedYes, 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.
Comment #4
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedReroll.
Comment #5
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedAdding 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.
Comment #6
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedA lot more coding standards improvements.
Comment #8
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedComment #9
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedFor 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.
Comment #10
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedComment #11
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedFound a few changes that were not right. Corrected these.
Comment #12
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedOkay, one more...
Comment #13
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedProperly 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.
Comment #14
MegaChriz CreditAttribution: MegaChriz as a volunteer commented"Separate the @see and @ingroup sections by a blank line."
Didn't know.
Comment #17
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedCommitted #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).
Comment #18
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedAutomated fixes by PHP_CodeSniffer. I adjusted some of the automated fixes as I found them not always to be correct.
Comment #20
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedCommitted #18. Back to active for the remaining code style issues.
Comment #21
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedCode style fixes for the two feature modules included with Feeds.
Comment #23
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedCommitted #21.
Here is a patch with some coding standards fixes for files in the includes folder.
Comment #24
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedComment #25
MegaChriz CreditAttribution: MegaChriz at WebCoo commentedI will ignore the "Multi-line assignment not indented correctly" rule. The coding standards want me to do something like this:
Comment #27
MegaChriz CreditAttribution: MegaChriz at WebCoo commentedCommitted #25.
Here are some coding standard fixes for files in the plugins folder.
Comment #28
NWOM CreditAttribution: NWOM commentedI 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
Comment #29
DamienMcKennaLooks good.
Comment #30
joelpittetCommitted the patch in #27, thanks for tackling that @MegaChriz
Comment #32
joelpittetTacking 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.
Comment #34
joelpittetSmoke test worked. PHP 7 tests are failing due to date module