I am using views_data_export to create a CSV download with the Exposed Filters form in a block. One of the exposed filters is a Select widget (for content type). I get an error message at the top of my file when I download it.

I think the problem is that the value of the Select widget is an array, and the code expects a string (as for a text box/area). It passes that value through check_plain(), which hands it off to htmlspecialchars(), generating the error.

I will supply a patch as soon as I have an issue number.

Update (2014-06-20):

The original patch (#1) was committed to the 7.x-3.x branch (#2) and backported to the 6.x-2.x branch (#4).

The original patch assumes that the option is a string or an array of strings. Later comments point out that it can get worse than that. The patch in #12 deals with more cases and was marked RTBC (#13, #14, #16). The patch in #15 aims to deal with any level of arrays, but (according to #16) has trouble with simple strings.

CommentFileSizeAuthor
#15 views_data_export-exposed_filter_token_bug-1888614-15.patch1.16 KBmichfuer
#12 vde-exposed-filter-1888614-12.patch1.49 KBJvE
#7 vde-exposed-filter-1888614-7.patch1.48 KBbenjifisher
#1 vde-exposed-filter-1888614-1.patch1.1 KBbenjifisher

Comments

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Status: Active » Needs review
StatusFileSize
new vde-exposed-filter-1888614-1.patch1.1 KB

The attached patch fixes the problem for me. I am not sure what effect it will have on other file formats, or if the Exposed Filter form is not in a block. My guess is that the file format will not matter, since add_http_headers() adds the filter information as tokens in the 'Content-Disposition' header, which (I guess) is independent of the format.

I am also not sure whether the option can be anything other than a string or an array. At least, I think this patch is a step in the right direction.

steven jones’s picture

steven jones commented
Status: Needs review » Fixed

Thanks very much for the patch, fixed in 7.x-3.x.

steven jones’s picture

steven jones commented
Version: 7.x-3.x-dev » 6.x-2.x-dev
Status: Fixed » Patch (to be ported)

Needs backport

steven jones’s picture

steven jones commented
Status: Patch (to be ported) » Fixed

Pushed in 6.x-2.x

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

sinn’s picture

sinn commented
Status: Closed (fixed) » Needs work

#1 doesn't work with the between-date-exposed-filter because option in that case is like

array('value' => array('date' => '2013-06-28'),
    'min' => array('date' => '2013-06-28'),
    'max' => array('date' => '2013-06-28'),
);

Code:

  $option = implode('--', $option);

causes error in that case: "Array to string conversion in views_data_export_plugin_style_export->add_http_headers() (line 330 of D:\xampp54\htdocs\specsavers.localhost\brand2\code\sites\all\modules\contrib\views_data_export\plugins\views_data_export_plugin_style_export.inc)."

https://drupal.org/node/1298814#comment-6870686 works in that case.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Status: Needs work » Needs review
StatusFileSize
new vde-exposed-filter-1888614-7.patch1.48 KB

@sinn, thanks for pointing this out. I guess if I had looked harder in the issues queue, I would have found #1298814: PHP warning when exposed filters contain non-scalar data. (now closed as a duplicate of this issue) instead of opening this one.

I am not too surprised that the patch in #1 fails to cover some cases: see my comment there.

The latest patch in the other issue has some funkiness:

            foreach($filter_data as $key=>$data) {
              $data = is_array($data) ? implode('-', $data) : $data;
              $filter_data = $key . '-' . $data;
            }

Even if PHP allows it, I do not like replacing an array while iterating over it, but surely this snippet does not do what we want: the effect is to set $filter_data to $key . '-' . $data for the last entry of the original array, discarding the results of earlier entries. Also (and the first thing I noticed) it violates Drupal coding standards: => should have a single space on either side.

Please test the attached patch. It could be more concise, but I think it is pretty clear. I tested it on my site, but not with a date-range filter.

Note that my patch ignores $key unless $data is an array. I do not feel strongly about this: if you think the key should be included, then I can roll a new patch.

Are we confident that "array of array of strings" is as bad as it can get? Maybe we should do something with array_walk_recursive() in order to future-proof this bit of code.

JvE’s picture

JvE commented
Version: 6.x-2.x-dev » 7.x-3.x-dev
Status: Needs review » Needs work

I am using patch #7 on one of my sites now and it does fix the notice.

I do not know if the resulting $tokens string is useable though.

Example:
One date field "field_date" and one date-range field "field_date_range".

$tokens['%exposed'] with only range filled:
field_date_value_value----field_date_range_value_min-2011-7-9--max-2012-5-16
$tokens['%exposed'] with only date filled:
field_date_value_value-2012-6-10-field_date_range_value_min-----max---
$tokens['%exposed'] with neither filled:
field_date_value_value----field_date_range_value_min-----max---

Perhaps an array_filter() is needed on $data before implosion?

koppie’s picture

koppie commented
Status: Needs work » Reviewed & tested by the community

Patch in #7 works for me. (Patch in #1 did not.) Note that the new patch needs to be run against the dev version of the module. I'm marking this RTBC and switching the version back to 7.x-3.x.

Any word on when we can expect to see a new stable version?

Thanks!

koppie’s picture

koppie commented

Whoops sorry - our ticket updates crossed in the blogosphere. Can I leave this RTBC or does it need more work?

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented

If I remember correctly, the token is used to create a default file name for the download. It is configurable to some extent. It is not useful for me.

If that is right, then the problem in #8 is an annoyance, not a serious problem. Unless you need to be able to parse the file name ...

JvE’s picture

JvE commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new vde-exposed-filter-1888614-12.patch1.49 KB

The same patch from #7 with a filter for empty values.

$tokens['%exposed'] with only range filled:
field_date_value_value--field_date_range_value_min-2011-7-9--max-2012-5-16
$tokens['%exposed'] with only date filled:
field_date_value_value-2012-6-10-field_date_range_value_min---max-
$tokens['%exposed'] with neither filled:
field_date_value_value--field_date_range_value_min---max-

jelle_s’s picture

jelle_s
Primary language Dutch
Location Antwerp, Belgium
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Patch #12 works for me

nancydru’s picture

nancydru
she/her/hers
Primary language English
Location Boston
commented

Seems to take care of me.

michfuer’s picture

michfuer commented
StatusFileSize
new views_data_export-exposed_filter_token_bug-1888614-15.patch1.16 KB

I feel like recursion is appropriate here since I never can anticipate the depth of a particular field/filter used by Views. This patch doesn't do anything the others don't, just takes a recursive approach.

drenton’s picture

drenton commented

Patch #12 works for me.
#15 does not work for string only filters.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Issue summary: View changes

I am updating the issue summary to summarize the recent activity.

@drenton, can you give steps to reproduce the problem using the patch in #15?

drenton’s picture

drenton commented

Any string only exposed filter should reproduce the problem.

I created a filter for :

Content: Author uid (exposed)

Here is the view export :


$view = new view();
$view->name = 'test';
$view->description = '';
$view->tag = 'default';
$view->base_table = 'node';
$view->human_name = 'test';
$view->core = 7;
$view->api_version = '3.0';
$view->disabled = FALSE; /* Edit this to true to make a default view disabled initially */

/* Display: Master */
$handler = $view->new_display('default', 'Master', 'default');
$handler->display->display_options['title'] = 'test';
$handler->display->display_options['use_more_always'] = FALSE;
$handler->display->display_options['access']['type'] = 'perm';
$handler->display->display_options['cache']['type'] = 'none';
$handler->display->display_options['query']['type'] = 'views_query';
$handler->display->display_options['exposed_form']['type'] = 'basic';
$handler->display->display_options['pager']['type'] = 'full';
$handler->display->display_options['pager']['options']['items_per_page'] = '10';
$handler->display->display_options['style_plugin'] = 'table';
$handler->display->display_options['style_options']['columns'] = array(
  'title' => 'title',
);
$handler->display->display_options['style_options']['default'] = '-1';
$handler->display->display_options['style_options']['info'] = array(
  'title' => array(
    'sortable' => 0,
    'default_sort_order' => 'asc',
    'align' => '',
    'separator' => '',
    'empty_column' => 0,
  ),
);
/* Field: Content: Title */
$handler->display->display_options['fields']['title']['id'] = 'title';
$handler->display->display_options['fields']['title']['table'] = 'node';
$handler->display->display_options['fields']['title']['field'] = 'title';
$handler->display->display_options['fields']['title']['label'] = '';
$handler->display->display_options['fields']['title']['alter']['word_boundary'] = FALSE;
$handler->display->display_options['fields']['title']['alter']['ellipsis'] = FALSE;
/* Filter criterion: Content: Author uid */
$handler->display->display_options['filters']['uid']['id'] = 'uid';
$handler->display->display_options['filters']['uid']['table'] = 'node';
$handler->display->display_options['filters']['uid']['field'] = 'uid';
$handler->display->display_options['filters']['uid']['value'] = '';
$handler->display->display_options['filters']['uid']['exposed'] = TRUE;
$handler->display->display_options['filters']['uid']['expose']['operator_id'] = 'uid_op';
$handler->display->display_options['filters']['uid']['expose']['label'] = 'Author uid';
$handler->display->display_options['filters']['uid']['expose']['operator'] = 'uid_op';
$handler->display->display_options['filters']['uid']['expose']['identifier'] = 'uid';

/* Display: Page */
$handler = $view->new_display('page', 'Page', 'page');
$handler->display->display_options['path'] = 'test';

/* Display: Data export */
$handler = $view->new_display('views_data_export', 'Data export', 'views_data_export_1');
$handler->display->display_options['pager']['type'] = 'some';
$handler->display->display_options['style_plugin'] = 'views_data_export_csv';
$handler->display->display_options['style_options']['provide_file'] = 1;
$handler->display->display_options['style_options']['parent_sort'] = 0;
$handler->display->display_options['style_options']['quote'] = 1;
$handler->display->display_options['style_options']['trim'] = 0;
$handler->display->display_options['style_options']['replace_newlines'] = 0;
$handler->display->display_options['style_options']['header'] = 1;
$handler->display->display_options['style_options']['keep_html'] = 0;
$handler->display->display_options['path'] = 'testcsv';
$handler->display->display_options['displays'] = array(
  'page' => 'page',
  'default' => 0,
);
steven jones’s picture

steven jones commented
Status: Reviewed & tested by the community » Needs work

Setting back to needs work, as there seems to be some disagreement about whether it works or not.

JvE’s picture

JvE commented

#12 works in all cases so far reported.

#15 adds support for multidimensional select options but removes the check to see if $options is an array or not before treating it as one.

@michfuer: when is $option a multidimensional array? Could you provide an example view?

steven jones’s picture

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

Sorry for the lack of attention to your issue, please accept my apologies.

Drupal 7 is going to be end-of-life'd by the community in approximately 1 month.

As such, I am closing all non-critical looking, non-PHP compatibility issues for Views Data Export to tidy up the issue queues and reduce the noise. You can read about this on #3492246: Close down Drupal 7 issues.

If you feel like this issue has been closed by mistake, please do comment about re-opening it.
If you feel like the ticket is still relevant for the 8.x-1.x version of the module, then please search for a duplicate issue first, and if there really isn't one (and you've looked properly) then change the version on the ticket and re-open.

Thanks to everyone involved in this issue: for reporting it, and moving it along, it is truly appreciated.
The Drupal community wouldn't be what it is today without your involvement and effort, so I'm sorry that we couldn't get this issue resolved. Hopefully we'll work together in a future issue though, and get that one resolved :)