Great initiative 👍 I'd target 3.4.

Thanks@chalasr@kaznovac@xabbuh for your comments and review.
I just push fixes. I'm agree to set$escapeFormulas atfalse by default, after all negative numbers doesn't have to be escaped. And it will avoid BC Break.

Should I replace allassertEquals byassertSame inCsvEncoderTest class?

@welcoMattic Nope, that would create merge conflicts when merging lower branches up to master. Thanks.

Should we also allow to pass this option through the context so that you do not necessary have to configure this option globally?

@xabbuh why not. We should also make the context$escapeFormulas value overrides the global$escapeFormulas value. It could be set totrue globally, but for one particular encoding it could be set tofalse.

👍 to allow overriding this option in the context.

@chalasr@xabbuh Overriding is now possible

Great!

But as stated in the article, it's definitely not a security issue on our side. The current implementation respects the RFC (Excel and Google Sheets don't), and most software consuming CSV expect the current behavior.
It should made be clear in the docs PR that this option should only be used when the generated CSV will be imported in spreadsheets or it can cause interoperability issues.

Thanks@dunglas!

For documentation, should I create a new page underSerializer section? Named CsvEncoder, and where I explain why this options exists?

Alternative approach might be to just add a normalizer. That would help with deserialization of such sanitized csvs too and doesn't pollute __construct arguments.

I don't like having this in core anyway though, as it's security issue in Excel and Google sheets only and this is destructive kind of escaping

Not sure if we need to support that in core. /cc @symfony/mergers

👍 to include that as it's security feature that people should be aware about.

👍 to support this feature too. But as it's definitely not a security fix (on our side), it's a new feature that should be merged in 4.1.

there is only a problem if this component is used to generate sheets with formula.

There is also a problem when the value start with'=', '-', '+', '@' and the parser supports correctly the RFC: a\t will be added, but it should not (actually, when using this flag, it's not really a true CSV file anymore).

Thanks@dbu@dunglas for your comments. I think that we can support this "fix" and explain it explicitly in PHPDoc.

Should I update the PR now, or we need to wait other opinions?

i'd update the changelog and phpdoc as that will be relevant either way. i hope one of the maintainers can decide which version this should go. i can see the point of kevin, and its a good reason why its not on by default. but i feel it still is security relevant, even if its not a bug of symfony, but symfony helping around lax security of other tools - and as such should go into 3.3 or 3.4.

I think that you can update and rebase the PR@welcoMattic

@nicolas-grekas Done, I also fix@ostrolucky's comment.

@welcoMattic thanks. Can you rebase to get rid of the merge commit? I invite you to squash while doing so.

Thanks for review@Tobion 👍

Thank you@welcoMattic.