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

Fix For #1509 #1518

Merged
merged 6 commits into from
Jun 19, 2020
Merged

Fix For #1509 #1518

merged 6 commits into from
Jun 19, 2020

Conversation

oleibman
Copy link
Collaborator

@oleibman oleibman commented Jun 10, 2020

This is:

- [x] a bugfix
- [ ] a new feature

Checklist:

Why this change is needed?

User expected no CSV enclosures after $writer->setEnclosure(''),
which had been changed to be consistent with $reader->setEnclosure('').
Writer will now omit enclosures after code above; no change to Reader.
Tests have been added for this condition.

Resync with base project
User expected no CSV enclosures after $writer->setEnclosure(''),
which had been changed to be consistent with $reader->setEnclosure('').
Writer will now omit enclosures after code above; no change to Reader.
Tests have been added for this condition.
Allowing the user to specify no enclosure when writing a CSV can lead to
a situation where PhpSpreadsheet (likewise Excel) will not read the
resulting file as intended, e.g. if any cell contains a delimiter character.
This is demonstrated in new test TestBadReread.
No existing setting will rectify this situation.

A better choice would be to add an option to write the enclosure
only when it is needed, which is what Excel does. The RFC4180 spec at
https://tools.ietf.org/html/rfc4180
states when it is needed - when the cell contains the delimiter,
or the enclosure, or a newline.
New test TestGoodReread demonstrates that the file is read as intended.

The documentation has been updated to describe the new function,
and to change the write example where the enclosure is set to null.
3 minor changes, all in tests.
@nickfls
Copy link

nickfls commented Jun 15, 2020

Are there any plans to merge it any time soon?

@MarkBaker
Copy link
Member

Thank you for the quick resolution to the issue with setting empty CSV enclosures

@MarkBaker MarkBaker merged commit ce6ac1f into PHPOffice:master Jun 19, 2020
@andriesreitsma
Copy link

@MarkBaker could you be so kind and make this a new release so we don't need workarounds to make CSV Enclosure working as expected again in Maatwebsite/Excel ? since it uses the latest release... thanks in advance!

@MarkBaker
Copy link
Member

I really want to get the changes that I'm doing rewriting named ranges/formulae completed before doing a new release (perhaps another week or two of work), because the next steps that I'm planning after that could lead to some bc breaks, which would require a new 2.0.0 release.
Is it possible for you to work with dev-master for that time? Or does the Laravel wrapper restrict you to tagged releases?

@SudoGetBeer
Copy link

I really want to get the changes that I'm doing rewriting named ranges/formulae completed before doing a new release (perhaps another week or two of work), because the next steps that I'm planning after that could lead to some bc breaks, which would require a new 2.0.0 release.

Is it possible for you to work with dev-master for that time? Or does the Laravel wrapper restrict you to tagged releases?

You can just use "phpoffice/phpspreadsheet": "1.12.0", in composer and everything works 😉

@andriesreitsma
Copy link

Even though @MarkBaker just tagged a new release, the requirements changed for Guzzle 6 -> 7 and that is a no go (it's like domino, now with Google API) ... so no solution to update to latest tagged releases and that indeed leaves the option as stated by @SudoGetBeer open as final solution for now. I do understand the choice @MarkBaker to aim for a bigger release... and will keep an eye out on package updates so this will integrate nicely again.

@oleibman oleibman deleted the csvenclosures branch November 2, 2020 01:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants