Problem/Motivation

As a result of 2854083 and 2854080 we have a use case where we would like to avoid the header row being output.

Proposed resolution

Provide an option in the constructor in the same way that the delimiter is set.

Remaining tasks

Proposed patch to follow shortly.

User interface changes

None.

API changes

None, fully backwards compatible; simply provides a new option that another module could choose to use.

Data model changes

None.

CommentFileSizeAuthor
#7 csv_serliazation-2854087-skip-headers.patch1.65 KBSteven Jones
#4 interdiff-2854087-2-4.txt1.12 KBscott_euser
#4 option_for_csv_encoder-2854087-4.patch2.78 KBscott_euser
#2 option_for_csv_encoder-2854087-2.patch1.73 KBscott_euser
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

scott_euser created an issue. See original summary.

scott_euser’s picture

scott_euser CreditAttribution: scott_euser as a volunteer and at Fat Beehive commented
Status: Active » Needs review
FileSize
option_for_csv_encoder-2854087-2.patch1.73 KB
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented 
+++ b/src/Encoder/CsvEncoder.php
@@ -55,11 +62,15 @@ class CsvEncoder implements EncoderInterface, DecoderInterface {
    *   Indicates the character used for escaping. Defaults to "\"
+   *
+   * @param bool $output_header

no empty line between multiple @params.

Also, this is a service, so how would you set this through the constructor? Should we add a setter method? This is something you want to control per call, as you might call it multiple times per request, so maybe actually an argument to encode()? I see it supports $context and also has setSettings(), so it should probably be integrated there.

This will also help #2789531: Support for batch operations

scott_euser’s picture

scott_euser CreditAttribution: scott_euser as a volunteer and at Fat Beehive commented
FileSize
option_for_csv_encoder-2854087-4.patch2.78 KB
interdiff-2854087-2-4.txt1.12 KB

no empty line between multiple @params.

Was wondering what to do with that; my editor is throwing all sorts of coding standards warnings. Thought I might keep it consistent with their current style then submit a second issue to fix all the coding standards at once? For instance, pre patch it looks like this already:

  /**
   * Constructs the class.
   *
   * @param string $delimiter
   *   Indicates the character used to delimit fields. Defaults to ",".
   *
   * @param string $enclosure
   *   Indicates the character used for field enclosure. Defaults to '"'.
   *
   * @param string $escape_char
   *   Indicates the character used for escaping. Defaults to "\"
Also, this is a service, so how would you set this through the constructor? Should we add a setter method? This is something you want to control per call, as you might call it multiple times per request, so maybe actually an argument to encode()? I see it supports $context and also has setSettings(), so it should probably be integrated there.

Makes sense, I looked at the setSettings, but it seems to be very tied to views and it feels dirty to do something like

$context = [
  'views_style_plugin' => new stdClass(),
];
$context['views_style_plugin']->options['csv_settings'] = [
  ...
];
$encoder->encode($data, $format, $context);

Proposed solution attached.

scott_euser’s picture

scott_euser CreditAttribution: scott_euser as a volunteer and at Fat Beehive commented

Essentially I think the method needs to be public if we want to use it, or potentially break it into separate getters and setters as you suggest and leave that one for the views plugin only as it is now.

grasmash’s picture

grasmash CreditAttribution: grasmash at Acquia commented
Status: Needs review » Needs work

Would you be willing to re-roll this for 8.x-2.x?

Steven Jones’s picture

Steven Jones CreditAttribution: Steven Jones at ComputerMinds commented
FileSize
csv_serliazation-2854087-skip-headers.patch1.65 KB

Here's an updated patch against 8.x-1.4.

I've changed it to use a public setter and called that from the protected setSettings. I've also remove the constructor argument, though it does feel like this should be an argument to the encode method, but I guess the interface is fairly inflexible there.

grasmash’s picture

grasmash CreditAttribution: grasmash at Acquia commented
Status: Needs work » Needs review
Kingdutch’s picture

Kingdutch
he/him
Primary language English
CreditAttribution: Kingdutch at Open Social commented

Is there a reason that the patch chooses for a new function? I think this makes it hard to use through the Serializer API right? If context were to be used then that may be easier as that variable is passed on verbatim.

Could an example be added of how the new setting would be used through batcher Serializer API calls?

There was also some work and discussion done in #2999806: Option for CSV Encoder to add header or not to. which may be of interest.

scott_euser’s picture

scott_euser CreditAttribution: scott_euser as a volunteer and commented

Berdir's comment https://www.drupal.org/project/csv_serialization/issues/2854087#comment-... explains why it shouldn't be in the constructor.

Use case example: https://www.drupal.org/node/2854083 - would not want the headers to be output in in batch.

Hope that helps clarify!

Kingdutch’s picture

Kingdutch
he/him
Primary language English
CreditAttribution: Kingdutch at Open Social commented

Re #10

I understand why you wouldn't want it in the constructor and I agree with the sentiment.

My question was why the patch under review introduces a new function rather than using the $context parameter of the encode function as described by Berdir in that same comment.

It also makes your use case easier because you could just add a ternary expression in the context instead of calling a separate function.

  • grasmash committed 03695e1 on 8.x-1.x 
    Issue #2854087 by scott_euser, Steven Jones, grasmash, Kingdutch: Option...
grasmash’s picture

grasmash CreditAttribution: grasmash at Acquia commented
Status: Needs review » Fixed

Please take a look at https://www.drupal.org/node/2567011/commits and let we know if it addresses this issue. I followed Kingdutch's line of thinking and removed the separate setter method.

Status: Fixed » Closed (fixed)

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

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented

The code in the module doesn't provide any means to control whether the header is output, the only solution is to extend the class and add an option in the constructor.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented

The idea of using the $context argument to encode() wouldn't currently work as the function() doesn't modify the current object attributes before $this->outputHeader is checked. So as it works, there isn't any way to control the header unless a) you extend the class, b) you pass in $context['views_style_plugin']->options['csv_settings']. Can this please go back to "needs work"?