Data Export Import

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.
• Comments: there should be a space after "//", see http://drupal.org/node/1354#inline
  
  test.php:17://echo "dataset file: ".$dataset_file."\n";
• ./includes/taxonomy_terms.inc: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  
                                     // 'depth' => $file_content_as_array['vocabulary_terms'][$existing_vocabulary_id][$existing_term->tid]->depth,
                                 // 'depth' => $file_content_as_array['vocabulary_terms'][$existing_vocabulary_id][$existing_term->tid]->depth,
• ./data_export_import.module: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  
                                                              //'description' => 'Export a dataset.',
                                                            //'page callback' => 'data_export_import_callback',
                                                            // 'page arguments' => array('', 'backup_migrate_ui_manual_backup_quick', TRUE),
                                                            //'access arguments' => array('access content'),
                                                            //'description' => 'Import a dataset',
                                                            // 'page arguments' => array('', 'backup_migrate_ui_manual_restore', TRUE),
                                                                 //'description' => 'Set where datasets are stored',
                                                                 // 'page arguments' => array('', 'backup_migrate_ui_manual_restore', TRUE),
                                                             //'description' => 'Dataset profiles',
                                                             // 'page arguments' => array('', 'backup_migrate_ui_manual_restore', TRUE),
                                                              //'description' => 'Scheduled exports and imports',
                                                              // 'page arguments' => array('', 'backup_migrate_ui_manual_restore', TRUE),
  //$form['file_upload'] = array('#title' => t('Upload file'), '#type'  => 'file');
  //$form['submit_upload'] = array('#type'  =>  'submit', '#value'  =>  'Submit');
• Comments should be on a separate line before the code line, see http://drupal.org/node/1354#inline
  
  ./includes/taxonomy_terms.inc:264:  } // foreach vocabulary - this is the test loop to check that the
./includes/taxonomy_terms.inc:471:      } // Found a term in the dataset which matches on term id and
./includes/taxonomy_terms.inc:474:    } // Looping through existing terms.
./includes/taxonomy_terms.inc:480:  } // Looping through the existing vocabularies
./includes/taxonomy_terms.inc:523:                                 'vid' => $term_from_dataset->vid, // Voacabulary ID
./includes/taxonomy_terms.inc:549:      } // Block to deal with term needing to be created.
./includes/taxonomy_terms.inc:551:    } // Looping through the dataset terms.
./includes/taxonomy_terms.inc:557:  } // Looping through the dataset vocabularies.
./includes/taxonomy_terms.inc:570:} // import_vocabularies function.
./data_export_import.module:28:  $output = '';  //declare your output variable
./data_export_import.module:36:  } // function data_export_import_help
• ./includes/taxonomy_terms.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  
  function taxonomy_save_term_with_defined_tid(&$form_values) {
• ./includes/taxonomy_terms.inc: The description on the line after the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
  
  652- *  Internal use only.
• ./data_export_import.module: The description on the line after the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
  
  25-   */
• There should be no space after the opening "(" of an array, see http://drupal.org/node/318#array
  
  includes/taxonomy_terms.inc:67:      if (in_array( $term->tid, $list_of_terms)) {
• Remove all old CVS $Id tags, they are not needed anymore.
  
  data_export_import.info:1:; $Id$

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

I've set this to 'needs review'. Can I take it that this is what I'm supposed to do after fixing up the points raised by the reviewer.

Hi,

I've set this to 'needs review'. Can I take it that this is what I'm supposed to do after fixing up the points raised by the reviewer.

Yes, this is exactly it. ;)

Automated review (Please keep in mind that this is primarily a high level check that does not replace but, after all, eases the review process. There is no guarantee that no other issues could show up in a more in-depth manual follow-up review.)

Review of the 6.x-1.x branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  
  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards

sites/all/modules/pareview_temp/test_candidate/data_export_import.module:
+35: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
+42: [minor] Comment should be read "Implements hook_foo()."

Status Messages:
Coder found 1 projects, 1 files, 1 normal warnings, 1 minor warnings, 0 warnings were flagged to be ignored
• ./includes/profiles/taxonomy_terms.inc: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  
          // Currently changed from chop() to rtrim() as recommended by coder_tough_love.
• Comments should be on a separate line before the code line, see http://drupal.org/node/1354#inline
  
  ./includes/profiles/taxonomy_terms.inc:517:                                 'vid' => $term_from_dataset->vid, // Voacabulary ID
./includes/profiles/pages.inc:40:  $orig = node_load($node_id); // load original
./includes/profiles/pages.inc:47:  // $node = $orig;       // copy for mods
./includes/profiles/pages.inc:49:  // //  $orig->body = 'xxxxqwertyasdasdasd12345678'; // Do some changes
./includes/profiles/articles.inc:40:  $orig = node_load($node_id); // load original
./includes/profiles/articles.inc:47:  // $node = $orig;       // copy for mods
./includes/profiles/articles.inc:49:  // //  $orig->body = 'xxxxqwertyasdasdasd12345678'; // Do some changes
• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  
  ./test.php ./data_export_import.info ./README.txt ./includes/profiles/taxonomy_terms.inc ./includes/profiles/pages.inc ./includes/profiles/articles.inc

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Manual review

• There are still files in your master branch. You should replace them as described in http://drupal.org/node/1127732 at step 5.

Code has been tidied and run past pareview_sh and coder (with coder_tough_love).

Master branch code has been pruned to a single README.txt file as advised.

Thanks for the previous review.

Code now passes code, pareview_sh and code sniffer!

Export and import of taxonomy terms and pages is now complete.

Extra blank pages have been commented out to keep the interface cleaner.

Docs have been added to to explain how to download and upload the dataset files.

Some stubs of code have been left in to start the export/import of another content type - these have been commented out in a compliant way. Next I will look to move the development of new content types to a development branch.

Hi,

This was set to 'needs review' over two weeks ago so I've taken the liberty to raise the priority level as per the guidelines.

Currently the code is polished and users can export then import taxonomy terms and pages. Should all be working fine and the interface is clean and easy to use.

I'm currently working on another branch to enable the export and import of all content types - but I'd like to get this project reviewed/tested/approved etc before merging in this new work as a development branch.

Thanks in advance for any reviews carried out.

All was fine - the code review tools were picking up things from the new branch which is not in this release. Will repost the reason I raised the priority to major.

This was set to 'needs review' over two weeks ago so I've taken the liberty to raise the priority level as per the guidelines.

Currently the code is polished and users can export then import taxonomy terms and pages. Should all be working fine and the interface is clean and easy to use.

I'm currently working on another branch to enable the export and import of all content types - but I'd like to get this project reviewed/tested/approved etc before merging in this new work as a development branch.

Thanks in advance for any reviews carried out.

The review at:

http://ventral.org/pareview

is now being passed with now errors. This online test includes pareview and code sniffer - and I think pareview runs coder as well.

Thanks for the review.

All points have been worked through - I've attached a document which outlines all the details of the work carried out and responses to the points raised.

Module fully passes the coding standards review at: http://ventral.org/pareview/httpgitdrupalorgsandboxbailey861278830git

Module is fully functional - users can export and import all taxonomy terms - and can export/import all standard page nodes.

Once the module is accepted I will be able to work on the next phase which is to export/import all content types and users.

Thanks for the review. All points have been fixed or addressed so the module is ready for review again.

• data_export_import_help(): This is a hook implementation, so the first line in the comment must be "Implements hook_help().". Please also check your other hooks. See http://drupal.org/node/1354#hookimpl

>>> REPLY
Comments have been changed.

• data_export_import_callback_export(): if you just want to return a form on your page callback you can use "drupal_get_form" directly in hook_menu(). Same for data_export_import_callback_import().

>>> REPLY
I'd prefer to leave them in separate functions as later I may add some checks which would be carried out before the current forms are loaded.

• "'#value' => 'Create dataset files',": Make sure that all user facing text runs through t() for translation.

>>> REPLY
The t() function has been added to 'Create dataset files' and elsewhere where needed. Menu items have been left as they are as pareview says 'Menu item titles and descriptions should NOT be enclosed within t()'.

• "$directory = "sites/default/files/data_export_import/taxonomy_terms";": still a hard-coded path.

>>> REPLY
Fixed.

• "drupal_set_message(t("The data export functionality is not currently compatible with localization."), 'error');": that message is not correct, it should be "internationalization (i18n)"

>>> REPLY
Fixed.

• "$form_state['clicked_button']['#post']['pages']": do not access raw POST data, use $form_state['clicked_button']['values'] instead.

>>> REPLY
Fixed.

• test.php: I don't think this file has any value in the repository. Also you use drush integration if you want to do something from the command line with Drupal.

>>> REPLY
Fixed - the file has been removed.

• data_export_import_export_pages(): This will only work for a rather small amount of nodes, for larger sites you will run into a timeout or a memory issue. Batch API helps with that http://api.drupal.org/api/drupal/includes--form.inc/group/batch/6

>>> REPLY
I've currently tested this module with 5,000 nodes and 5,000 taxnomy terms and the export and import ran fine. I would like to change the code to run functions under Batch API and I've looked closely at Batch API to see how it can be used.

I'm currently being sponsored to create this module for a company I am working for. If the module is accepted as a full Drupal module then they may be more likely to sponsor it further and myself or one of their full-time coders can add in the Batch API functionality.

• I think node_import is very similar to your approach, why does it not work for you? http://drupal.org/project/node_import

>>> REPLY
I looked very closely at node_import.

The key/main/important difference is that this module will import the nodes and assign them the same ID number as they had when they were exported.

Also, it will only update or create the nodes (or taxonomy terms) necessary to make them match the nodes/terms which were exported. So if you have a dataset of 1,200 nodes - and the receiving instance already holds the first 1,000 nodes - then only the new 200 nodes will be imported. Node Import will re-import all the nodes and will create them with new ID numbers. So before any import it would be necessary to delete all the existing nodes.

Think of this module is terms of 'rsync --delete' for terms and nodes.

If the terms and nodes have the same ID number as the original data then any cross references and hard-coded links will still work OK. The sites I'm working on are huge multimedia sites with dozens of custom modules. And dozens of editors/reporters etc - and many of the content types (approx 17) use the node ID in their path.

This module is to be used as part of a whole dev/staging/live deployment process. I've written about the process in the description but basically the dev's all push site functionality/design changes up to a common beta site - and using this module live data can be exported from the current live site and imported into the beta site. The beta site becomes the new live site when all has been tested.

I.e. - using this module you pull data down from a live site to a beta rather than pushing code/module changes from a beta site to a live site.

• Re-implementing node_save() and drupal_write_record() really scares me. The migrate module for example just sets the "is_new" property on a new node with an id that should be preserved, but I think that only works in Drupal 7. http://drupal.org/node/1382932#comment-5400362

>>> REPLY
Agreed - as you say that is D7 only - so I've had to override the current function to get the functionality needed in D6.

• "echo "Term not saved correctly. Term id:" . $existing_term->tid . "\n";": Remove all echo statements from your code and replace them with proper messages.

>>> REPLY
Fixed.

FINAL NOTES.

If you check out the all_content_types branch you'll see that I now have the module able to cleanly export/import any of the content types on a Drupal instance - they get listed automatically and the dataset files get named automatically. Standard CCK fields work fine. I am currently working on exporting then re-importing any files which have been attached to a node or are from a CCK field (FileField or ImageField).

If this module is shown to be accepted as part of the Drupal module list my current employer may be happy to sponsor further development either by myself or by one of their own full-time coders. Currently the 6.x-1.x branch export/imports taxonomy terms and pages - I feel that the terms import/export on it's own would be very useful to many. Further down the line we're looking to add a profile to export/import users as well.

Please note - the way the module is designed on the new all_content_types branch allows for site devs to create their own completely custom profiles by adding a single file containing the relevant functions. This will allow the export/import of ANY data for any site no matter how specialised any added modules are.

FURTHER WORK

As this module addresses a big issue for Drupal - the data content migration issue - I'm hoping it will be of use to the wider Drupal community who may wish to work with it and enhance it further.

CODE STANDARDS

I've run the code through the automated tests at http://ventral.org/pareview and it's looking fine. I'm sure you remember from the discussion I generated at http://groups.drupal.org/node/195303 about the coding standards tools!

Thanks for your review - I await any further feedback if you find the time.

And thanks for the heroic effort on the modules queue - it was getting quite large and seemed to be becoming a log jam.

There is now a branch which exports/imports using the batch API. The branch is called handle_large_files.

I will now clean up the code and make the batch API messages nicer. Once the code is compliant I will merge in the handle_large_files branch to the 6.x-1.x branch.

This module now exports and imports users.

A Drush interface has been added to enable the module to be used from the command line.

Think in terms of the module acting like rsync with the --delete option.

If the node id or user id (or term id) already exists in the destination DB then those nodes/users are updated with the information in the incoming data file. The plan is that the nodes and users are first exported to a data file - then this data file is imported into a destination DB - and after the importation the users/nodes should be an *exact* match of the data which was exported from the first Drupal site.

This is useful for the following reason.

Say you have a live site which you export from - and it has a new node with id of 123.

You also have a beta copy of the site which is being tested - and a developer creates a test node with dummy data which has an id of 123.

If you import from the data file exported from the live site the data for node 123 will overwrite the data in the beta site. I.e. it will replace test/dummy data with data from the live site.

Similarly, if on the beta site a developer creates a test node of id 123456 which does not exist on the live site (which exported the data) then this node will be deleted from the destination DB.

Preserving the node ID's of everything - terms/users/nodes - will (as you say) keep noderefs and userrefs working.

As mentioned - the idea is that terms/users/nodes will be exactly replicated between Drupal sites.

This is all part of a larger development/deployment plan which has been documented in the module. I'm currently working on enhancing the plan and should be posting a white paper when it is ready.

Thanks for the review.

I've not been able to review other projects recently as I've been working hard on testing this module. It's been used to pull in 10GB dataset files and has been used in a large deployment scenario.

Also, I've been writing up a document called 'Drupal websites development and deployment strategies' which shows how this module can be used as part of large deployments. Once the document is finished it will be published as a white paper (or possibly a book!) and
made available to the Drupal community.

Please note that I've aided the review process by helping to clear up the issue of coding standards testing. I raised the issue at http://groups.drupal.org/node/195303 and the conclusion RE using ventral.org has become part of the instructions on the 'Tips for ensuring a smooth review' page - http://drupal.org/node/1187664 - hopefully that shows I'm keen to help!

I've worked on your module review points. Here's my replies (not in the same order):

2. data_export_import_callback_overview(): all user facing text must run through t() for translation.
DONE.
I've changed the output in that function.

4. dei.drush.inc: make sure to use your full module name as function prefix to avoid name collisions with other modules.
DONE.
I've changed the drush.inc file so that the functions use the full module name as the prefix. The function names are related to the drush.inc file name so I've changed the file name as well.

1.data_export_import.module: don't execute PHP code gloablly here, as this will be called on every single page request. Include your files only in the appropriate hooks/functions.
and
3. data_export_import_menu(): why do you include files in hook_menu()? You don't need them there?

I've designed the module so it can be extended relatively easily by adding new profiles. If someone adds a new profile file into includes/profiles then it will create a new tab on the data_export_import interface and allow the user to export and import data.

Currently, the module enables user, taxonomy terms and nodes to be exported and imported. I wanted to allow for expansion by adding new profiles. This means that users can relatively easily add new profiles to handle any weird and wonderful custom data they may need to export/import.

As part of this profile plugin setup the code in data_export_import.module loops through all the files it finds in the includes/profiles directory and adds in the callback functions and the extra menu items (tabs). That is what the code is doing.

I don't really want to remove the ability to easily add new profiles - and can't see how it can be implemented differently. When the admin page is requested it needs to read the profiles directory to see what tabs to load and to have the callback functions available.

Thanks again for the review - my current employer has invested heavily in this module and it would be great if we can get it approved as a full Drupal module.