Found a big problem with bin/export.php and civicrm_cli_csv_exporter

[sonicthoughts]

I noticed a few bugs (some fixed) related to the bin/import/export functions( e.g. CRM-14729).  I noticed when doing an export of Tags, that parent_id was not in the column header.  Turns out that the logic of the civicrm_cli_csv_exporter in bin/cli.class.php looks at the return of the first row using a Get action and bases the column heading on the object.  However for some objects, if the field is NULL it does not include the field in the object (for some fields it does.)  It seems that this happens with Int type fields (like parent_id in the case of Tag) but not with strings.

This has been fixed in some cases by returning a null value in the object but the real issue is the code logic. I think the header and fields should be based on the getfield API call, not grabbing the fields from the first line.

Here is the bit of code causing the issue:

Code: [Select]

$this->row = 1;
    $result = civicrm_api($this->_entity, 'Get', $this->_params);
    $first = true;
//var_dump($result); - When I dumped the objects it is often missing null fields.
    foreach ($result['values'] as $row) {
      if($first) {
        $columns = array_keys($row);
        fputcsv($out, $columns, $this->separator, '"');
        $first = false;

I would fix this if I were a developer, but pretty sure this is related to several issues.

[Eileen]

That makes sense - do you want to try this - it's the most conservative approach because it should include all fields from both (if one has a field the other doesn't).

(note the lines that don't look changed are because of trailing white-space being removed on save - which is why the coding standard is not to have any )

diff --git a/bin/cli.class.php b/bin/cli.class.php
index de72c47..d939074 100644
--- a/bin/cli.class.php
+++ b/bin/cli.class.php
@@ -207,12 +207,12 @@
     CRM_Core_ClassLoader::singleton()->register();
 
     $this->_config = CRM_Core_Config::singleton();
-   
+
     // HTTP_HOST will be 'localhost' unless overwritten with the -s argument.
     // Now we have a Config object, we can set it from the Base URL.
     if ($_SERVER['HTTP_HOST'] == 'localhost') {
         $_SERVER['HTTP_HOST'] = preg_replace(
-                '!^https?://([^/]+)/$!i',
+                '!^https?://([^/]+)/$!i',
                 '$1',
                 $this->_config->userFrameworkBaseURL);
     }
@@ -304,9 +304,10 @@
     $this->row = 1;
     $result = civicrm_api($this->_entity, 'Get', $this->_params);
     $first = true;
+    $columns = civicrm_api3($this->_entity, 'getfields', array('action' => 'get'));
     foreach ($result['values'] as $row) {
       if($first) {
-        $columns = array_keys($row);
+        $columns = array_keys(array_merge($row, $columns['values']));
         fputcsv($out, $columns, $this->separator, '"');
         $first = false;
       }

[sonicthoughts]

I created a patchfile from what you sent and the headers are correct, but the data is misaligned.  This should give you an idea:http://screencast.com/t/gYWSvriX0
If I applied the patch correctly (no errors) it seems the issue is that the null values are not getting output in foreach ($row as &$field)  or the  fputcsv

[Eileen]

This might fix that

diff --git a/bin/cli.class.php b/bin/cli.class.php
index de72c47..06a35d2 100644
--- a/bin/cli.class.php
+++ b/bin/cli.class.php
@@ -207,12 +207,12 @@
     CRM_Core_ClassLoader::singleton()->register();
 
     $this->_config = CRM_Core_Config::singleton();
-   
+
     // HTTP_HOST will be 'localhost' unless overwritten with the -s argument.
     // Now we have a Config object, we can set it from the Base URL.
     if ($_SERVER['HTTP_HOST'] == 'localhost') {
         $_SERVER['HTTP_HOST'] = preg_replace(
-                '!^https?://([^/]+)/$!i',
+                '!^https?://([^/]+)/$!i',
                 '$1',
                 $this->_config->userFrameworkBaseURL);
     }
@@ -304,18 +304,20 @@
     $this->row = 1;
     $result = civicrm_api($this->_entity, 'Get', $this->_params);
     $first = true;
+    $columns = civicrm_api3($this->_entity, 'getfields', array('action' => 'get'));
     foreach ($result['values'] as $row) {
       if($first) {
-        $columns = array_keys($row);
+        $columns = array_keys(array_merge($columns['values'],$row));
         fputcsv($out, $columns, $this->separator, '"');
         $first = false;
       }
       //handle values returned as arrays (i.e. custom fields that allow multiple selections) by inserting a control character
-      foreach ($row as &$field) {
+      foreach ($row as $key => &$field) {
         if(is_array($field)) {
           //convert to string
           $field = implode($field,CRM_Core_DAO::VALUE_SEPARATOR) . CRM_Core_DAO::VALUE_SEPARATOR;
         }
+        $row = array_merge(array_fill_keys($columns, ''), $row);
       }
       fputcsv($out, $row, $this->separator, '"');
     }

[sonicthoughts]

Again I'm not a coder, but from my snooping, the problem may be that the fields need to be explicitly set to null for fputcsv to work. I found this:
http://www.experts-exchange.com/Programming/Languages/Scripting/PHP/Q_26548863.html
and discussion here: http://php.net/manual/en/language.types.null.php
I could be off base ....

BTW - I have to wonder if people are using these routines.  We have an old database that has been upgraded many times and for many tables, the first row has many null values since the feature didn't exist.  Take create_date in Tags for example. I think that was added in a later version so the first entry has a null value and wouldn't be output.  Just wondering....

[sonicthoughts]

I'm afraid that this code hangs the process ...

This might fix that

diff --git a/bin/cli.class.php b/bin/cli.class.php
index de72c47..06a35d2 100644
--- a/bin/cli.class.php
+++ b/bin/cli.class.php
@@ -207,12 +207,12 @@
     CRM_Core_ClassLoader::singleton()->register();
 
     $this->_config = CRM_Core_Config::singleton();
-   
+
     // HTTP_HOST will be 'localhost' unless overwritten with the -s argument.
     // Now we have a Config object, we can set it from the Base URL.
     if ($_SERVER['HTTP_HOST'] == 'localhost') {
         $_SERVER['HTTP_HOST'] = preg_replace(
-                '!^https?://([^/]+)/$!i',
+                '!^https?://([^/]+)/$!i',
                 '$1',
                 $this->_config->userFrameworkBaseURL);
     }
@@ -304,18 +304,20 @@
     $this->row = 1;
     $result = civicrm_api($this->_entity, 'Get', $this->_params);
     $first = true;
+    $columns = civicrm_api3($this->_entity, 'getfields', array('action' => 'get'));
     foreach ($result['values'] as $row) {
       if($first) {
-        $columns = array_keys($row);
+        $columns = array_keys(array_merge($columns['values'],$row));
         fputcsv($out, $columns, $this->separator, '"');
         $first = false;
       }
       //handle values returned as arrays (i.e. custom fields that allow multiple selections) by inserting a control character
-      foreach ($row as &$field) {
+      foreach ($row as $key => &$field) {
         if(is_array($field)) {
           //convert to string
           $field = implode($field,CRM_Core_DAO::VALUE_SEPARATOR) . CRM_Core_DAO::VALUE_SEPARATOR;
         }
+        $row = array_merge(array_fill_keys($columns, ''), $row);
       }
       fputcsv($out, $row, $this->separator, '"');
     }

[sonicthoughts]

perhaps "$row = array_merge(array_fill_keys($columns, ''), $row);" is messing with the for loop " foreach ($row as $key => &$field) "

[sonicthoughts]

YES that was it.  changed to:
$newrow = array_merge(array_fill_keys($columns, ''), $row);
      }
      fputcsv($out, $newrow, $this->separator, '"');
and it seems to work!

[sonicthoughts]

Did some more testing and changing to newrow works fine!
I tried to generate a patchfile but don't have the latest branch cloned and got into trouble with git diff.  Can you generate a patch and submit?

thanks!
S.

[Eileen]

Do you want to log a ticket & point to this - we might ask Xavier to check it since it's his baby

[xavier]

Until you provide the DNS test, I feel free to deny any paternity over that code. And as it's been moved over from csv, Tim might be identified.

It make sense, not sure why it wasn't a problem before (might be that the null objects were handled differently).

It might be good to add a option.format=csv in the api v4 (by default still json), that would replace this export/import.

@sonicthoughts, can you attach the final version modified (not the patch) and tell me which civicrm version you started from, I'll try to git foo the PR

[sonicthoughts]

Thanks guys.  The code below seems to work fine.  Did testing with a few Entity types.  Hope we get this in as cli.php and csv commands are very handy for the non-developer mortals

Based on 4.4.5
simple diff:

Code: [Select]

210c210
<
---
>
215c215
<                 '!^https?://([^/]+)/$!i',
---
>                 '!^https?://([^/]+)/$!i',
307d306
<     $columns = civicrm_api3($this->_entity, 'getfields', array('action' => 'get'));
310c309
<         $columns = array_keys(array_merge($columns['values'],$row));
---
>         $columns = array_keys($row);
315c314
<       foreach ($row as $key => &$field) {
---
>       foreach ($row as &$field) {
320d318
<         $newrow = array_merge(array_fill_keys($columns, ''), $row);
322c320
<       fputcsv($out, $newrow, $this->separator, '"');
---
>       fputcsv($out, $row, $this->separator, '"');
full function snippet

Code: [Select]

/**
 * class used by csv/export.php to export records from
 * the database in a csv file format.
 **/

class civicrm_cli_csv_exporter extends civicrm_cli {
  var $separator = ',';

  function __construct() {
    $this->_required_arguments = array('entity');
    parent::initialize();
  }

  function run() {
    $out = fopen("php://output", 'w');
    fputcsv($out, $this->columns, $this->separator, '"');

    $this->row = 1;
    $result = civicrm_api($this->_entity, 'Get', $this->_params);
    $first = true;
    $columns = civicrm_api3($this->_entity, 'getfields', array('action' => 'get'));
    foreach ($result['values'] as $row) {
      if($first) {
        $columns = array_keys(array_merge($columns['values'],$row));
        fputcsv($out, $columns, $this->separator, '"');
        $first = false;
      }
      //handle values returned as arrays (i.e. custom fields that allow multiple selections) by inserting a control character
      foreach ($row as $key => &$field) {
        if(is_array($field)) {
          //convert to string
          $field = implode($field,CRM_Core_DAO::VALUE_SEPARATOR) . CRM_Core_DAO::VALUE_SEPARATOR;
        }
        $newrow = array_merge(array_fill_keys($columns, ''), $row);
      }
      fputcsv($out, $newrow, $this->separator, '"');
    }
    fclose($out);
    echo "\n";
  }
}



[sonicthoughts]

Until you provide the DNS test, I feel free to deny any paternity over that code. And as it's been moved over from csv, Tim might be identified.

It make sense, not sure why it wasn't a problem before (might be that the null objects were handled differently).

It might be good to add a option.format=csv in the api v4 (by default still json), that would replace this export/import.

@sonicthoughts, can you attach the final version modified (not the patch) and tell me which civicrm version you started from, I'll try to git foo the PR

Just added attachment too. Thanks
+1 for option.format=csv and less code to maintain.  I guess a cli tool like this http://www.ofb.net/~egnor/xml2/ would work for the desperate ...