Simple Node Importer is a simple module which imports nodes using CSV files. It provides the functionality for mapping UI to map CSV columns to it's corresponding entity field.

Project Page:Simple Node Importer

Git repository: git clone --branch 7.x-1.x https://git.drupal.org/sandbox/sourdrup/2828039.git simple_node_importer

Manual reviews of other projects: on 5th December
https://www.drupal.org/node/2831523#comment-11810553
https://www.drupal.org/node/2829972#comment-11810571
https://www.drupal.org/node/2811601#comment-11810587

CommentFileSizeAuthor
#3 error.png53.84 KBGomesh
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sourdrup created an issue. See original summary.

PA robot’s picture

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.

Gomesh’s picture

Status: Active » Needs review
FileSize
53.84 KB

Steps to reproduce the problem while reviewing the module .

1. Select the content type where csv need to be import .
2. then go to simple node import content and add the csv file .
3. Importing the content shows , unable to import few entries . Please go to resultion centre and re-import the file .
4. But after edit the file and added the missing entry , and try to upload the new file .
5. I got the error , I also attached the screenshot .

prasannag’s picture

I have tried to upload a csv file with the 4 fields/columns(Title, Body, Tags and Image).
Few nodes are missing from import after the batch process.
This is happening when importing taxonomy tags field with multiple values.

Sanjay Chauhan’s picture

Hi @sourdrup,

I'd made a little code review.

simple_node_importer_ajax_breif_note_callback() and simple_node_importer_mapping_form()
- There are Lots of HTML code inside PHP, Kindly use Theme function or create template file.

simple_node_importer_menu()
-The Drupal 6 and 7 menu system stores menu item titles and descriptions in English. This allows the system to cache the data, but display to users using various languages on demand. For this to work, you should not use t() on the title or description of menu items in your hook_menu() implementation. Additionally, you should attempt to use a literal string (rather than a dynamic string) for these two keys, so the translation template extractor can find the string you used.

Regards,
Sanjay Chauhan

rajveergangwar’s picture

Priority: Major » Normal

Hi
Below are my review:

1) line 252 please use t() for "Kindly contact Admin to select the content ......."
2) Please delete
variable_set('comment_simple_node', COMMENT_NODE_CLOSED);
variable_set('node_options_simple_node', array('status'));

these variable in hook_uninstall

3) Please use t() in you tpl files for
content-info-note.tpl

  • Mandotory Fields are:

  • Note: In Multivalue Field List '()' braces contains maximum allowed values.

mapping-help-text-info.tpl

<div class="form-item form-type-item" id="edit-helptext">
Note:
<ul>
  <li>Content type Field(s) are the fields of selected content type present in existing site.</li>
  <li>CSV Column(s) are the column name provided in the uploaded CSV file.</li>
  <li>User can select from right side 'CSV Column(s)' to assign it's value to the corresponding Content Field.</li>
  <li>To avoid import failure, map your CSV columns to the appropriate Content type fields.</li>
  <li>For boolean type fields, use 'y' or '1' for TRUE and 'n' or '0' for FALSE in source file.</li>
  <li>For multivalued field values, provide each value in separate column in CSV file. Select all the columns for respective field as shown below: <br /><a href=<?php print $filepath; ?> target='_blank'><img src=<?php print $filepath; ?> alt='mappingUI' width='25%' ></a></li>
<?php if ($allowed_date_format != NULL) { ?>
  <li>For this content type, allowed Date format should be <?php print $allowed_date_format; ?></li>
</ul>
<?php } else { ?>
</ul>
<?php } ?>
</div>

4) please remove patch file.

rajveergangwar’s picture

Status: Needs review » Needs work

changing status

patilvishalvs’s picture

Manual Review

No duplication
No: There is a Feeds module available, it allows you to map CSV columns with entity fields.

sourabh.singhal’s picture

Hi patilvishalvs,

Kindly visit the project page of my module Simple Node Importer and see how it is different from the Feeds Module.

Thanks

sourabh.singhal’s picture

Hi Sanjay Chauhan,

I have fixed all the issues you have mentioned in #5.

Thanks

sourabh.singhal’s picture

Status: Needs work » Needs review

Hi rajveergang,

I have fixed all the issues you have mentioned in #6.

Thanks

sourabh.singhal’s picture

Hi prasannag,

I have tried to reproduce the error which you mentioned in #4 but fails. I tried 5 files with different scenario but always imported successfully.

Can you please more descriptive or can provide steps to produce.

Thanks

Jabastin Arul’s picture

Please review your code on "https://pareview.sh" site. We found some issue in your code. Here I have attached the link for your error code.

https://pareview.sh/node/162

Jabastin Arul’s picture

Status: Needs review » Needs work
sourabh.singhal’s picture

Status: Needs work » Needs review

Hi Jabastin Arul,

All the errors showing on PAreview.sh has been fixed.
pareview.sh

Thanks for reviewing.

sourabh.singhal’s picture

Hi @Gomesh,

For #1,

Sorry for late response, actually as per our road map for this, there is no functionality to edit the importer node and can resubmit the new file.
Functionality work flows as:
1. From Resolution Center you can download CSV for failed nodes.
2. Start new import again and upload the file.

I know, on resolution center page under resolution center table, links available for edit, will drive you in confusion, apologies for ignoring that, I have updated the page, now Edit link is no more there.

Thanks for detailing

In case of any issues or suggestions, you're most welcome :)

Ujwala_D’s picture

Hi @sourdrup,

I have used this module. It seems working fine. PA review issues has been fixed.

Thanks

sourabh.singhal’s picture

Thanks Ujwala_D,

For giving your valuable time in reviewing this module.

sourabh.singhal’s picture

Issue summary: View changes
Issue tags: +PAreview: review bonus
sourabh.singhal’s picture

Issue summary: View changes
Issue tags: -PAreview: review bonus
sourabh.singhal’s picture

Issue summary: View changes
Issue tags: +PAreview: review bonus
sourabh.singhal’s picture

Issue summary: View changes
Issue tags: -PAreview: review bonus
sourabh.singhal’s picture

Issue summary: View changes
sourabh.singhal’s picture

Issue summary: View changes
Issue tags: +PAreview: review bonus
sriharsha.uppuluri’s picture

Issue tags: -PAreview: review bonus

You are supposed to add this tag when you review other project applications.

Removing the bonus tag for now. Please add it back once you have reviewed other modules.

sourabh.singhal’s picture

Issue tags: +PAreview: review bonus

Hi @sriharsha.uppuluri ,
Please check the links at the top of issue page,
there are 3 automatic and 3 manual reviews done, all for seperate projects.

Please review again and update us back.
Thanks for the review.

Automatic reviews of other projects: on 1st December
https://www.drupal.org/node/2796471#comment-11805327
https://www.drupal.org/node/2831897#comment-11805329
https://www.drupal.org/node/2831720#comment-11805337

Manual reviews of other projects: on 5th December
https://www.drupal.org/node/2831523#comment-11810553
https://www.drupal.org/node/2829972#comment-11810571
https://www.drupal.org/node/2811601#comment-11810587

prasannag’s picture

Hi sourdrup,
Thanks for fixing issue mentioned at #4.
No more issues now. Working as mentioned.

nikhilsukul’s picture

Hi @sourdrup,

Just reviewed your module. It is working fine for me too.

klausi’s picture

Issue summary: View changes

Removed automated reviews from the issue summary, not relevant for review bonus.

klausi’s picture

Assigned: Unassigned » visabhishek
Status: Needs review » Needs work
Issue tags: -PAreview: review bonus +PAreview: security

Git errors:

Review of the 7.x-1.x branch (commit f6e33af):

  • Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
    
    FILE: /home/klausi/pareview_temp/simple_node_importer.module
    --------------------------------------------------------------------------
    FOUND 2 ERRORS AFFECTING 2 LINES
    --------------------------------------------------------------------------
     316 | ERROR | [x] Equals sign not aligned with surrounding assignments;
         |       |     expected 1 space but found 2 spaces
     546 | ERROR | [x] Equals sign not aligned correctly; expected 1 space
         |       |     but found 2 spaces
    --------------------------------------------------------------------------
    PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
    --------------------------------------------------------------------------
    
    Time: 347ms; Memory: 12Mb
    
  • No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

  1. simple_node_importer_uninstall(): no need to drop the node_resolution table here, drupal core will automatically do that via the schema when the module is uninstalled.
  2. simple_node_importer_schema((): all database table names should be prefixed with your module name to avoid name clashes with other modules.
  3. simple_node_importer_type_installed_instances(): doc block is wrong, this is not a hook? The just describe what the function is doing in a sentence. See https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...
  4. simple_node_importer.module: why do you require_once all inc files in the global module scope? That means all include files are loaded on every single page request. You should only require them when they are actually needed, for example in page callbacks.
  5. simple_node_importer_node_resolution_center(): do not call theme() at the end of your page callbacks, just return the render array. Drupal core will render it later for you and is easier for other module to alter it.

There are multiple security vulnerabilities in this module and as part of our git admin training I'm assigning this to visabhishek so that he can take a look. If he does not find anything I'm going to post the vulnerability details in one week. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

addonsolutions’s picture

Hi visabhishek,

I got this Potential problem on line #258.

use drupal_set_message(check_plain()) instand of drupal_set_message() for security point of view.

addonsolutions’s picture

Hi visabhishek,

I got some another issue on this project.

1) Notice: Undefined index: values in simple_node_importer_field_upload_csv_validator()
if user have not selected "Select Content Type" field and trying to upload csv.

2) and change function name taxonomy_term_save instead of taxonomy_save_term on Line #225.

visabhishek’s picture

Hi addonsolutions,

Thanks for your help, Please allow me some time to finding the security issues.
As you know this is an assignment of git admin training, So I have to find the things as already mentioned by klausi.

visabhishek’s picture

Assigned: visabhishek » Unassigned

Hi ,
My findings are :

1: Security vulnerability: On path simplenode/%/%node/delete is vulnerable to CSRF exploits. You need to either use a confirmation form or CSRF tokens on GET requests. See http://epiqo.com/de/all-your-pants-are-danger-csrf-explained

2: Please use t() for all User facing text
Example :

drupal_set_message("Node import completed! Import status: $import_status");
  if (empty($node)) {
    $type = 'Simple Node Importer';
    $message = 'Node object is not valid.';
    watchdog($type, $message, array(), WATCHDOG_ERROR, NULL);
  }

3: arg(1), arg(2) should be sanitize before printing into #markup.

Example :

$form['cancel'] = array(
      '#markup' => l(t('Cancel'), 'simplenode/' . arg(1) . '/' . arg(2) . '/delete', array('attributes' => array('class' => array('cancel-button')))),
      '#weight' => 50,
    );

4: $record['title'] and $record['body'] should be sanitize

function simple_node_importer_make_nodes($record, &$context) {
  global $user;
  $node = new StdClass();
  $node->type = $record['type'];
  $node->title = ($record['title']) ? $record['title'] : $node->result[] = $record;
  $node->uid = $user->uid;
  $node->language = 'en';
  $node->body[LANGUAGE_NONE][0]['value'] = ($record['body']) ? $record['body'] : '';
  $field_names = array_keys($record);

Please let me know if I missed anything.

klausi’s picture

@visabhishek: yay, great that you found the CSRF issue!

Unfortunately your other points are wrong:

Point 2: you must NOT use t() for watchdog(). Please read the documentation on watchdog().

Point 3: the usage of arg() in l() is not a problem because l() will call check_plain() on the path, so no security issue there. When you suspect a problem in such a case I suggest trying to exploit it, then you quickly realize why it would not work :-)

Point 4: simple_node_importer_make_nodes() must NOT sanitize the data because it does not print anything to HTML. It only saves stuff to the database. Remember: "When handling data, the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database" from https://www.drupal.org/node/28984 . So no security issue here.

I found simple_node_importer_node_resolution_center(): the $author user name is printed unsanitized to table markup here. Usually that is not a problem because Drupal core does not allow script tags in user names, but third party modules can easily use external user names and we have to always treat them as untrusted user provided data. #theme => 'username' is probably a good solution here.

PA robot’s picture

Status: Needs work » Closed (won't fix)

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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