-
-
Notifications
You must be signed in to change notification settings - Fork 336
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug fix #213 and #208 #215
Conversation
nyamsprod
commented
Feb 22, 2017
- Remove the @deprecate message from the Reader class Deprecation of League\Csv\Reader::fetch #208
- Bug fix internal Reader::getRow when used with a StreamIterator fetchAssoc() fails with freshly created Reader #213
d01d0b5
to
8992405
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this does actually fix the issue, but there are a fair number of changes here which are definitely not part of the fix.
@@ -156,18 +156,17 @@ public function setFlags($flags) | |||
* @param array $fields | |||
* @param string $delimiter | |||
* @param string $enclosure | |||
* @param string $escape |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You removed the $escape
type hint, but the param is still available in the method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep typo error from my part.. I'll correct that
* | ||
* @return int | ||
*/ | ||
public function fputcsv(array $fields, $delimiter = null, $enclosure = null, $escape = null) | ||
public function fputcsv(array $fields, $delimiter = ',', $enclosure = '"', $escape = '\\') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the public interface and should not be part of a bugfix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be part of bugfix because it aligns the StreamIterator to how SplFileObject and fputcsv works. StreamIterator must be align to SplFileObject and it did no align.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StreamIterator is marked as @internal
so anyone using it in public API exposes himself to harm.
} | ||
|
||
return str_getcsv($line, $this->delimiter, $this->enclosure, $this->escape); | ||
return $row; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The expected return value for this method is an array, and now appears to be a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no $row is always an array otherwise an exception is triggered