AppVeyor failure looks unrelated.

As for#21718 doing this now is a BC break, people might rely on the current behavior and/or workaround it on their side.
The BC break is more impactful here since the changed value is also used in prod env.
I'll try to think about an upgrade path today if nobody suggests one, but 👎 as is to me.

A BC layer could be to add the underscore version of cookie names in the list whenever a dashed one is here, to keep clearing the underscore version too (it would then clear 2 cookies).

@thewilkybarkid Would you mind looking at@stof's suggestion? I can do it if you don't.

This is for 3.4 as it is a behavior change and should include deprecations, branch needs to be rebased and PR retargeted.

Don't see how this is a BC break? I don't think it's possible to rely on the current behaviour (since it's mangling the name), and if you have an existing workaround then that would continue to work? If you want underscores if your cookie name why wouldn't you have used that already?

If you want underscores if your cookie name why wouldn't you have used that already?

Dunno, probably no viable reason. But fact is that if one set a name with dashes and ends up with underscores, one could just forgot about the name set in the config and implicitly rely on this behavior. For the same input the final value will be different than before, that is a behavior change which can break.

But why prioritise someone who's incorrectly configured the cookie name here (using dashes when they meant underscores) over someone who's being bitten by this bug (probably without realising)?

@thewilkybarkid That is where I disagree. Incorrectly or not, the one who did configure it could be a different person that the one who rely on the unexpected result, thinking it's expected, letting untouched and just dealing with underscores... Breaking (edge) cases are easy to imagine here.
This behavior exists for years and this changes it, but we don't want to bother our users for no strong reason (given you can avoid normalization by explicitly setting thename attribute instead of using the cookie name as key, right?).

I agree with Stof and@chalasr. This is obviously a worthwhile fix - definitely was a bug/mistake originally. But we should add the BC layer.

@thewilkybarkid Feature freeze is this weekend? Would you have time to update your PR before then?

Thanks!

Moving to 4.1. Rebase on master might be needed, where PHP 7.1 features can be used btw.

This is tricky ... but Symfony's policy in similar situations has always been:

1. If this is considered a bug, then we can fix it, even if that changes the behavior and introduces a BC break...
2. ..."BC breaks" are not considered for bug fixes, because otherwise, every single bug fix would be a "BC break" (you are changing the previous code behavior!)

So I'd say, this is a bug fix and not a BC break.

Still -1 for me without BC layer, not worth the breaking change as there is a valid way to avoid this behavior (by specifyingname: ... instead of using the name as key).
All keys are normalized by default (that could be reconsidered on master) and that's consistent with the decision made on the very same issue regarding other config nodes in the past.@thewilkybarkid should we close/take over?

Still don't agree that it needs a BC layer since it's a bug that doesn't have behaviour that can be relied on.

But I'd much rather the bug is fixed than having this left open, so do whatever's needed to have it resolved.

Closing in favor of#30111, thank you for rising the issue.