Skip to content
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

CSVReader::current() avoid recursion #101

Merged
merged 2 commits into from
Jul 25, 2014
Merged

CSVReader::current() avoid recursion #101

merged 2 commits into from
Jul 25, 2014

Conversation

boekkooi
Copy link
Contributor

The current CSVReader is using recursion when a row is invalid this may cause a Fatal error: Maximum function nesting level when using a file with a lot of invalid rows.

To avoid this a do while is introduced and the $this->current() call is removed.

@ddeboer
Copy link
Owner

ddeboer commented Jul 24, 2014

Interesting. Can you add a test case that makes the original implementation fail?

@boekkooi
Copy link
Contributor Author

@ddeboer Hope https://github.com/boekkooi/data-import/commit/813cebded8ab7ca4716e57e96c7cd1017c417d6a is good enough. It's not a great test but it will fail with the old implementation and work with the new one.

@Baachi
Copy link
Collaborator

Baachi commented Jul 25, 2014

@boekkooi Cany you push the testcase to this PR?

@ddeboer ddeboer added the bug label Jul 25, 2014
@boekkooi
Copy link
Contributor Author

@Baachi You mean combine the 2 commits? since they are both already in the PR.

@Baachi
Copy link
Collaborator

Baachi commented Jul 25, 2014

Ah my fault, sorry. I didn't see that you already pushed it 😅

Baachi added a commit that referenced this pull request Jul 25, 2014
CSVReader::current() avoid recursion
@Baachi Baachi merged commit 87283d9 into ddeboer:master Jul 25, 2014
@boekkooi
Copy link
Contributor Author

@Baachi No problem thanks for the merge!

@boekkooi boekkooi deleted the patch-1 branch July 26, 2014 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants