[D7] Simple CSV Importer

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxredorbit2450325git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Automated Review

Review of the 7.x-1.x branch (commit 2abe722):
http://pareview.sh/pareview/httpgitdrupalorgsandboxredorbit2450325git

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

No: Causes module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance.:

1. (+) Similar to feeds module.
2. Should use more comments.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Automated Review

Passed

Manual Review

Individual user account

[Yes: Follows / No: Does not follow] the guidelines for individual user accounts.

No duplication

[No: Causes] module duplication and/or fragmentation.

Master Branch

[Yes: Follows] the guidelines for master branch.

Licensing

[Yes: Follows] the licensing requirements.

README.txt/README.md

[Yes: Follows] the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

[Yes: Follows] the guidelines for project length and complexity.

Secure code

[Yes: Meets the security requirements. / No: List of security issues identified.]

Coding style & Drupal API usage

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. (+) It is similar to feeds module as acknowledged by author but can it be explained further why feeds was not developed further to provide this missing/broken functionality from feeds?

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

This review uses the Project Application Review Template.

The import process requires an "id" as a column to report. It is also forced to be mapped to somewhere - What should it be? Currently I am creating field called field_unique so that I can test it. Please advise.

Hi redorbit,

Small point really, consider splitting the permissions for configuring a import away from using the importer. There maybe situations where these are not the same user/role. Good work

Hi redorbit,
The module is already available for importing the nodes using CSV file. Please explain the difference of your module from Feeds and Node Import module.

Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the Feeds module issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process.

If that fails for whatever reason please get back to us and set this back to "needs review".

Hi,
in your requirements you should mention PHP version also because Drupal 7 support PHP 5.2 or higher, but you have used classes with namespaces which support only PHP 5.3 or higher.

Hi Ayaz,
thank you. You are right, and I must apologize that I did not care for the PHP requirements.
We implemented this module to run on a PHP 5.4 server. As I just checked, this PHP version has an end-of-life date of Sep 3, 2015. End-of-life for 5.2 was Jan 6, 2011. This may be an excuse.

There's Migrate module too.

Hi,

In your simple_csv_importer.module file please correct the following error

line#138:- Inline comments must end in full-stops, exclamation marks, colons, question marks, or closing parentheses

Thanks

I was able to map two columns ("Place" and "Number") to content type of article with fields title and body, however I got this error before the nodes were saved:

/drupal-7.43/sites/all/modules/2450325/lib/Drupal/simple_csv_importer/Content.php(49): Id column not found: "Place".

Thank you very much ddarras2012 for your observation. We have never seen this error message before, and it was hard to find the reason.

Actually, there have been two problems:

1. The message is wrong. Now it will be Id column "Place": The value must not be empty.'
2. The check whether the value is empty was wrong. It also rejected the value "0" that slould be a legal value.

I do hope that this has been your problem, and it should be fixed by now.

Besides, I also fixed some complaints by the Code Sniffer. It looks like the rules have been modified in the meantime.

Hi,

https://pareview.sh/node/1294

Please , clear the warnings

Thanks

Hi Santhosh Fernando,

thank you for your comment. However:

DrupalPractice has found some issues with your code, but could be false positives.

and this is one of them. The constructor is called in line 149.

    if (!self::$instance) {
      self::$instance = new GlobalData();
    }

Best regards
Siegfried

Hi Santhosh Fernando,

thank you for your comment. However, the warning is a false positive. The constructor is called in line 149.

Manual Review

Individual user account

Yes: Follows

No duplication

No. All of the functionality of this module is covered by the
feeds module. The author acknowledges this, but it is not clear what
advantages, if any, this module has over feeds. It is rather limited
in its field type support. It also required users to know the machine
names of the fields attached to the content type even when the
configuration forces users to enter a content type. If the content
type is known the text area for field mapping should be converted to
a list of column names and a pick list of fields from the content
type. There is only room for a single configuration so users would
need to change their settings each time i they wished to import more
than one content type.

Master Branch

Yes: Follows

Licensing

Yes: Follows

README.txt/README.md

No: Does not follow. Contains extra sections: Features,
Usage, Extended Functionality.

Code long/complex enough for review

Yes: Follows

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage - Codesniffer errors

1. Expected "} else {\n"; found "}\n else {\n"
2. Line indented incorrectly
3. Missing file doc comment
4. Missing @category tag in class comment
5. Missing @package tag in class comment
6. Missing @author tag in class comment
7. Missing @license tag in class comment
8. Missing @link tag in class comment
9. Opening brace of a class must be on the line after the
   definition
10. Private member variable "vocabulary" must be prefixed with
    an underscore
11. Private member variable "language" must be prefixed with an
    underscore
12. Private member variable "defaultLanguage" must be prefixed
    with an underscore
13. Expected 1 space after the longest variable name
14. Opening brace should be on a new line
15. Line exceeds 85 characters
16. The variable names for parameters $name (1) and $old_tid
    (2) do not align
17. The comments for parameters $name (1) and $old_tid (2) do
    not align
18. The variable names for parameters $old_tid (2) and $node
    (3) do not align
19. The comments for parameters $old_tid (2) and $node (3) do
    not align
20. Missing @return tag in function comment
21. Closing parenthesis of a multi-line IF statement must be on
    a new line
22. Multi-line IF statement not indented correctly
23. Opening parenthesis of a multi-line function call must be
    the last content on the line
24. Closing parenthesis of a multi-line function call must be
    on a line by itself
25. TRUE, FALSE and NULL must be lowercase; expected "true" but
    found "TRUE"
26. Missing @return tag in function comment
27. Private method name "TaxoInfo::readTerm" must be prefixed
    with an underscore
28. Missing @return tag in function comment
29. Private method name "TaxoInfo::findOrCreateTerm" must be
    prefixed with an underscore
30. Private method name "TaxoInfo::changeTerm" must be prefixed
    with an underscore

1. In addition to the code sniffer results, there are several commented out dom statements that should be removed entirely

This application would grant the reporter of this issue the vetted role and he/she would be able to opt in the security coverage review for your projects, but the reporter did not contribute code to this project.