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

Alternate to 20131 - Avoid crash during import for blank lines in a one-column csv file #21216

Merged
merged 1 commit into from
Aug 23, 2021

Conversation

demeritcowboy
Copy link
Contributor

Overview

Alternate to #20131

Importing a one-column csv file that has blank lines in it will crash

Before

  1. Create a file with just, e.g. email, in it. It should only have one column.
  2. Add one or more blank lines at the end.
  3. Go to contact import and import it.
  4. Crash with TypeError: Argument 1 passed to CRM_Import_DataSource_CSV::trimNonBreakingSpaces() must be of the type string, null given in CRM_Import_DataSource_CSV::trimNonBreakingSpaces() (line 259 of .../CRM/Import/DataSource/CSV.php)
  5. Unit test fails with same error.

After

Blank line is skipped. Test passes.

Technical Details

A blank line is read by fgetcsv as array(0 => NULL). When there's more than one expected column this gets skipped by some column-count checking already in the import code. When there's only one column, this ends up calling a function that's not expecting null. If the line is blank, don't even need to bother.

Comments

Has test.

@civibot
Copy link

civibot bot commented Aug 22, 2021

(Standard links)

@eileenmcnaughton
Copy link
Contributor

Ohh nice - that will also help if we want to do the 'better' fix & switch to reading the csv using the Reader package because the test is there now

@eileenmcnaughton eileenmcnaughton added the merge ready PR will be merged after a few days if there are no objections label Aug 22, 2021
@eileenmcnaughton
Copy link
Contributor

@mlutfy looks good to me - work for you?

@mlutfy
Copy link
Member

mlutfy commented Aug 23, 2021

Looks good, thank you!

I tested a few variations, including:

  • One column, with some empty lines at the middle of the file, or at the end of the file
  • Two columns, with the above variations
  • Lines that had only spaces

@mlutfy mlutfy merged commit 653ceed into civicrm:master Aug 23, 2021
@demeritcowboy
Copy link
Contributor Author

Thanks!

@agileware-justin
Copy link
Contributor

IMHO this should be backported and released sooner rather than later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-test master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants