Trimming on instantiation #132

Closed
rwitchell opened this Issue Oct 29, 2015 · 2 comments

Comments

Projects
None yet
2 participants
@rwitchell

Hi,
I have a concern with the trim on instantiation.

 public static function createFromString($str, $newline = "\n")
    {
        $file = new SplTempFileObject();
        $file->fwrite(rtrim($str).$newline);

We allow persons to copy/paste data from excel into a text input, which we then create a CSV object.

For example, if they copy in

image

the rtrim() on createFromString() would remove the final tab character, effectively removing the empty column in the last row.

I don't believe the trim here is a good idea.

The work arounds are:

  1. applying the second parameter, $newline, to be "\t\n" - however if data is provided with the last cell filled, then i've needlessly added an extra column. Solution: i have to read the last character of $str to check if it is a tab, and if so, update the $newline to "\t\n" otherwise leave it as "\n"
$newline = substr($_POST['textinput'], -1) === "\t" ? "\t\n" : "\n";
$reader = Reader::createFromString($_POST['textinput'], $newline);
$reader->setDelimiter("\t")
            ->setEnclosure("\"")
            ->setEscape("\\");

I don't believe the rtrim is necessary, as the application should match the user's representation of data as closely as possible. I think the better option would be to add trim() as an option on the object, $reader->rtrimIncomingString(true)

thoughts?

@nyamsprod nyamsprod added the bug label Oct 29, 2015

@nyamsprod

This comment has been minimized.

Show comment
Hide comment
@nyamsprod

nyamsprod Oct 29, 2015

Member

Hi @rwitchell,

Thanks for reporting this bug. As a matter of fact, this one is being fixed as part of the 7.2.0 release that will be out hopefully this week if I still have time or next week. The change will be in the master branch anyway. This is fixed as part of resolving SplFileObject flags bugs see #131

The $newline parameter is being deprecated from the createFromString method and will be remove completely in the next major release.

Member

nyamsprod commented Oct 29, 2015

Hi @rwitchell,

Thanks for reporting this bug. As a matter of fact, this one is being fixed as part of the 7.2.0 release that will be out hopefully this week if I still have time or next week. The change will be in the master branch anyway. This is fixed as part of resolving SplFileObject flags bugs see #131

The $newline parameter is being deprecated from the createFromString method and will be remove completely in the next major release.

@nyamsprod nyamsprod added the duplicate label Oct 29, 2015

nyamsprod added a commit that referenced this issue Oct 29, 2015

Improving code quality
- remove the need for the $newline argument in createFromString #132 #130
- improve integer filtering
@nyamsprod

This comment has been minimized.

Show comment
Hide comment
@nyamsprod

nyamsprod Oct 29, 2015

Member

This issue is fixed in the master branch

Member

nyamsprod commented Oct 29, 2015

This issue is fixed in the master branch

@nyamsprod nyamsprod closed this Oct 29, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment