Implement export / import Salsa2Salsa

1. +++ b/salsa_entity.drush.inc
   @@ -13,15 +13,22 @@ function salsa_entity_drush_command() {
   +      'paging' => dt('Number of records per file.'),
   

   this should probably mention that the default and max is 500 (and also enforce that. salsa limits to that and your code might get confused)

2. +++ b/salsa_entity.drush.inc
   @@ -13,15 +13,22 @@ function salsa_entity_drush_command() {
   +      'directory' => dt('Destination folder where to export the files.'),
   

   what's the default? isn't this required? then it possibly should be an argument?

3. +++ b/salsa_entity.drush.inc
   @@ -29,89 +36,145 @@ function salsa_entity_drush_command() {
     * Export salsa entities.
   + *
   + * @param array $types The salsa type, e.g. donate_page.
   + *
   + * @return mixed
   

   coding standard ;)

4. +++ b/salsa_entity.drush.inc
   @@ -29,89 +36,145 @@ function salsa_entity_drush_command() {
   +  // Export all salsa tables or just specified if types attribute is submitted.
   +  $salsa_types = salsa_entity_object_types();
   +  $types = empty($types) ? array_keys($salsa_types) : explode(',', $types);
   

   not sure about this default. Having one might make sense but it should possibly be a different, hardcoded list.

5. +++ b/salsa_entity.drush.inc
   @@ -29,89 +36,145 @@ function salsa_entity_drush_command() {
   +  // Prepare destination directory.
   +  $directory = drush_get_option('directory', 'public://salsa');
   

   yeah, I don't think a default here makes sense. Those files should *not* be in the public folder as that would make accessible over the web if you can guess the names.

6. +++ b/salsa_entity.drush.inc
   @@ -29,89 +36,145 @@ function salsa_entity_drush_command() {
   +  $paging = drush_get_option('paging', 500);
   +  if ($paging > 500) {
   +    return drush_set_error(dt('Number of records per file can not be bigger than 500.'));
   

   ah, fine, so just document it.

7. +++ b/salsa_entity.drush.inc
   @@ -29,89 +36,145 @@ function salsa_entity_drush_command() {
   +  // Prepare sub-directories and export objects into .csv files.
   ...
   +      while (TRUE) {
   +        if ($entities = salsa_api()->getObjects($type, array(), $offset . ',' . $paging)) {
   

   considering using a do/while loop.

8. +++ b/salsa_entity.drush.inc
   @@ -29,89 +36,145 @@ function salsa_entity_drush_command() {
   +          // Decided to store fields in KEY|VALUE format because after array_filter() call
   +          // do not have all records the same number of columns (depends on values).
   +          foreach ($entities as $entity) {
   +            $entity = array_filter(get_object_vars($entity));
   +            foreach ($entity as $key => &$value) {
   +              $value = $key . '|' . $value;
   +            }
   

   I wasn't aware that we decided for csv, I thought we use json? this seems very fragile and it would be useless for manual extraction anyway with that custom format.

   Why use array_filter()?

   Have you tried something like donation pages with special characters, multiline and so on? is that working correctly?

9. +++ b/salsa_entity.drush.inc
   @@ -29,89 +36,145 @@ function salsa_entity_drush_command() {
   +    return drush_set_error(dt('Sorry, import is not possible because directory @directory does not exists.', array('@directory' => $dir)));
   

   Sorry but we don't use sorry in error messages :)

10. +++ b/salsa_entity.drush.inc
    @@ -29,89 +36,145 @@ function salsa_entity_drush_command() {
    +  $sub_dirs = scandir($dir);
    +  foreach ($sub_dirs as $sub_dir) {
    +    $sub_dir_path = $dir . '/' . $sub_dir;
    +    if (is_dir($sub_dir_path) && array_key_exists($sub_dir, $salsa_types)) {
    +      $files[$sub_dir] = scandir($sub_dir_path);
    +    }
    

    Consider using DirectoryIterator for this: http://stackoverflow.com/questions/4202175/php-script-to-loop-through-al....

    You get http://php.net/manual/en/class.splfileinfo.php objects, with methods like isDot()/isReadable() and so on.

11. +++ b/salsa_entity.drush.inc
    @@ -29,89 +36,145 @@ function salsa_entity_drush_command() {
    +            // @todo Is it important the order in which objects are imported? If objects getting new KEYs on import, then for example donation import will depends on supporter, we should import supporters first, to store their mapping somewhere in order to connect donation to proper supporter later.
    

    Yes, the order is important. Which is why we need a hardcoded list that is in the correct order or need to find a dynamic solution, which is not trivial.

    A dynamic solution could look like this:

    if you find a $type_KEY property, extract type. If $type has not yet been imported, switch to $type first (the loop needs to be a function then), track what you already mapped/imported somehow. But that's step two, first get it working with a manual order.

12. +++ b/salsa_entity.drush.inc
    @@ -29,89 +36,145 @@ function salsa_entity_drush_command() {
    +            // @todo Where to store mapping definitions. Is local.settings.php appropriate solution? Maybe to generate file that will be located in the root of the source folder?
    

    That's a good question.

    As a start, you need to keep it in memory so that importing at the same time works. In a second step, we could write it into a while that we read again, but security wise, that's not trivial. Probably with an option --store-mappings=filename, e.g. as JSON.

    The current logic of printing PHP code should become an option (--print-php-mappings). There are still use case for that.

13. +++ b/salsa_entity.drush.inc
    @@ -29,89 +36,145 @@ function salsa_entity_drush_command() {
    +  // If types attribute has been submitted, remove all other files and import only preferred tables.
    +  $types = explode(',', $types);
    +  if (!empty($types)) {
    +    foreach ($files as $key => $value) {
    +      if (!in_array($key, $types)) {
    +        unset($files[$key]);
    +      }
    +    }
    +  }
    

    $files = array_intersect($files, $types)

8. About exceptions that make everything break: Should be pretty simple as long as we are talking about regular exceptions. Just a global try / catch. The only problem is if you create fatal errors. These need to be avoided. Persisting after every record is too much overhead. You might want to dump the mapping every 50 / N records?

4. Unsure about all this unlinking... I would expect that we can just overwrite and deleting could be up to the user (or as option only?)...
Otherwise if an export is just a one-off thing, we should possibly always create a new folder per run in the export folder? Dunno..

1. +++ b/salsa_entity.drush.inc
   @@ -13,105 +13,574 @@ function salsa_entity_drush_command() {
   +  // Make sure export directory exists.
   +  if (!isset($GLOBALS['salsa_export_dir'])) {
   +    try {
   

   Hm, $GLOBALS isn't really nice code..

   what about about a helper function like salsa_entity_export_iterator($directory = NULL). you initialize it just like here by calling with the argument, and then call it without argument to get the directory iterator where you need it?

2. +++ b/salsa_entity.drush.inc
   @@ -13,105 +13,574 @@ function salsa_entity_drush_command() {
   +      foreach ($dependencies as $dependent_type) {
   +        if (!in_array($dependent_type, $called)) {
   +          drush_salsa_entity_export($dependent_type, NULL);
   +        }
   

   Ah, I see, you call yourself again here.

   It might be a bit easier to follow if have the actual processing logic into a separate function that you call recursively. And just keep the option parsing and initializing into the current function.

   Also, shouldn't you track $called globally? Assume the following dependency tree:

   A
   - B
   - C
   - D
   - C

   So C is twice in there. But I think you'd end up importing it twice?

3. +++ b/salsa_entity.drush.inc
   @@ -13,105 +13,574 @@ function salsa_entity_drush_command() {
   +        // Get out of loop if $entities empty or error occurs, otherwise put data into JSON file.
   +        if (empty($entities) || $error->result == 'error') {
   +          $exported[] = $type;
   +          break;
   +        }
   

   If you have less then e.g. 500 records, you now always do two requests. First you fetch e.g. 100, then you try again for more.

   But you know that if you have less than $paging then you're done. So instead of while (TRUE), write (count($entities) == $paging. Then it will continue until there are less.

   You still need this check here, but only for the case where you have exactly 500 records or an error. An error should handled separatey and displayed IMHO.

4. +++ b/salsa_entity.drush.inc
   @@ -13,105 +13,574 @@ function salsa_entity_drush_command() {
   +        $sequence++;
   +        $offset = $offset + $paging;
   

   You should be able to save a variable if you instead use $sequence * $paging as $offset?

5. +++ b/salsa_entity.drush.inc
   @@ -13,105 +13,574 @@ function salsa_entity_drush_command() {
   +      // Check whether there are dependencies, if so, import that tables first.
   +      // @todo What if some of the dependent table / files is missing?
   +      $dependencies = salsa_mapping_dependencies($type);
   +      foreach ($dependencies as $dependent_type) {
   +        drush_salsa_entity_import($dependent_type, NULL);
          }
   

   If missing, display a warning and move on.

   Aren't we missing the logic here to avoid importing the same dependencies multiple times?

6. +++ b/salsa_entity.drush.inc
   @@ -13,105 +13,574 @@ function salsa_entity_drush_command() {
   +function salsa_mapping_dependencies($type = NULL) {
   +  $dependencies = array(
   +    'action' => array(
   +      'email_trigger',
   +      // 'add_to_groups_KEYS',
   +      // 'optionally_add_to_groups_KEYS',
   +      'template',
   

   Hm, so we hardcode this now? I was hoping it wouldn't be necessary but now that the work is done, fine ;)

7. +++ b/salsa_entity.drush.inc
   @@ -145,23 +614,23 @@ function drush_salsa_entity_generate_translatables() {
        }
        foreach ($salsa_info[$name]['item'] as $item) {
   -     // Skip some internal/technical things.
   -     foreach (array('READONLY', 'PRIVATE', 'KEY') as $ignore_pattern) {
   -       if (strpos($item['name'], $ignore_pattern) !== FALSE) {
   -         continue 2;
   -       }
   

   Unrelated change, possibly due to auto-format?

Ah didn't see that $called was defined as static, ignore that then.

Notes from testing:

* You can set 'required-arguments' => TRUE, apparently, then it should not display some strange notices if you forget the arguments.

* Getting notices like "Only variables should be passed by reference salsa_entity.drush.inc:73" when running mit -v, you need to store that in a variable first.

* Also getting "Undefined index: key salsa_api.inc:324 ", that might be a bug in salsa_api itself, maybe when there are no records or so.

* I think the all list is too much. There are many tables in there that we don't need. The following can certainly be removed as we don't need them for note. Also note that many of those won't be defined as entity types, then your entity_create() would fail on them:

+ distributed_event (not used by us atm)
+ database_table (that is internal metadata, not something you can import)
+ package (same)
+ custom_column and custom_column_option are special. That's about defining new custom fields. I don't know if that just works. We should try with a single one (creating them has lots of overhead, they also have unique names, breaking that might break salsa, we managed that before)
+ publish
+ tag/tag_data is special, I'm not sure if we can just save that, needs to be tested. Maybe we need to import tags differently (usually done as a special element on saving an entity: $form_state['supporter']->additional['tag'] = implode(',', $form_state['values']['tags']);. We might want to load them first and then add them to saved objects like that. We however need to database_table names to do that, so we might actually need that table, but not for importing again, just for lookup up the ID's in that table
+ merchant_account ( we don't need them and I don't think they can just be set up like this)
+ feed_archetype
+ supporter_picture and album, currently not used by us, we'd also need the files for that
+ version (internal)
+ chapter* ( I don't think we can just export/import them, not sure

So comment out all those things.

Also import should skip anything that does not exist as an entity type and display a warning, even if we define it.

* Another notice: Undefined property: stdClass::$result salsa_entity.drush.inc:112, you need to check if you actually have a result property there.

* Some more information would be great during the import. When you start a type, you could do a getCount() query and then display an info message like "Starting to export supporter (100'000 records)" (with getCounts()). If you get a count of 0 then you can also skip that type.

* For displaying messages that you want to be seen by the user, use drush_log($message, 'ok') use the default type (notice) for debug info that shouldn't be visible by default.

1. +++ b/salsa_entity.drush.inc
   @@ -294,6 +331,30 @@ function salsa_sync_map_key($table, $source_key, $target_key = NULL, $save = FAL
   + * @param array $entities
   + *   Salsa entities (custom fields).
   ...
   +function drush_salsa_entity_custom_fields_listing($entities, $iterator) {
   

   You seem to make it more complicated for yourself than it has to be. Why not use $custom_fields and document it as such?

2. +++ b/salsa_entity.drush.inc
   @@ -387,14 +448,14 @@ function salsa_mapping_dependencies($type = NULL) {
   -    'custom_column' => array(
   +    /*'custom_column' => array(
          // 'database_table',
          // 'old_custom_field_KEY',
   

   If we have things like this commented out, we should have an explanation for it. In this case, say that they are added explicitly at the beginning.

   without explanation, it might be better to remove them. Or keep them all together and have a single comment, explaining that those objects are not supported.

1. +++ b/salsa_entity.drush.inc
   @@ -13,105 +13,673 @@ function salsa_entity_drush_command() {
   +/**
   + * Helper function: Returns DirectoryIterator class or FALSE if directory does not exist.
   

   I find "Helper function: " and similar prefixes to be extremely useless. It's a waste of characters and width, even making your description go over 80 characters because of that. Just use the sentence that you have, that's perfect.

2. +++ b/salsa_entity.drush.inc
   @@ -13,105 +13,673 @@ function salsa_entity_drush_command() {
   + * @return object
   + *   DirectoryIterator.
   

   only use object when the type is unknown. Otherwise just use the type directly: @return \DirectoryIterator.

   +++ b/salsa_entity.drush.inc
   @@ -13,105 +13,673 @@ function salsa_entity_drush_command() {
   +  static $iterator = FALSE;
   +
   +  if (!is_null($directory)) {
   +    try {
   +      $iterator = new DirectoryIterator($directory);
        }
   +    catch (RuntimeException $e) {}
   

   This code is backwards, you always create a new iterator?

   You set it to FALSE but then use !is_null(). neither false nor an iterator object is null, so it will *always* go in there. Just use !$iterator.

   I'm also not sure about catching the exception here. As you've seen yourself, error handling then gets more complicated. You basically have to catch and ignore but then check again. That's what exception handling is for, that you can bubble up an unexpected error and deal with it in the caller.

Some feedback from testing out the export:

* I got some exceptions on certain tables:

Received exception during execution getCounts() method for type person, with message: SalsaScript Error                                                                                                  [warning]
Successfully created ../salsa/person/person_0.json file.                                                                                                                                                 [ok]
Received exception during execution getCounts() method for type admin, with message: SalsaScript Error                                                                                                   [warning]
Successfully created ../salsa/admin/admin_0.json file.                                                                                                                                                   [ok]
Received exception during execution getCounts() method for type webform, with message: SalsaScript Error                                                                                                 [warning]
Successfully created ../salsa/webform/webform_0.json file.                                                                                                                                               [ok]
Received exception during execution getCounts() method for type person_legislator, with message: SalsaScript Error                                                                                       [warning]
Successfully created ../salsa/person_legislator/person_legislator_0.json file.                                                                                                                           [ok]
Starting to export recipient (3 records)                                        

If those tables definitely don't work, we should not have them in our list. And if there's an exception I would expect to not see the succesfully created file message?

And last, looking at the tables we're exporting now, I noticed you export tag but not tag_data. Just the tags table alone is useless, it just contains a list of all existing tags. The tags_data table is the one that connects it to a certain thing. The problem with that is that it does go through the database table ID.

If you look at https://hq-kampa.salsalabs.com/api/getObjects.sjs?object=tag_data&limit=10, you can see that it has database_table_KEY and table_key. The first is the row in the database table. So if you have 45 there, you need to look it up in database_table, which tells you that's donation. So it's a tag for donation with KEY 12788348.

For saving, it's a lot easier. You can just put a comma separated list of tag *strings* (not ID's) on the $object->tags property. And salsa will take care of the rest.

Given that, I would recommend to deal with this complexity on export. We have a method for that on SalsaEntity, see getTags. You no longer use that, which is OK. If we do this, we need to improve that logic so we can fetch it for many objects at a time anyway.

That's the theory. Let me check how important that actually is.

Lets open a follow-up for those tags.

Added email things, fixed a few bugs, sorting the files for import and skip already exported files, for faster restart when it fails on export.

Committed, we'll do necessary improvements in follow-ups.