Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

• Always add tests
• Keep backward compatibility (see https://symfony.com/bc).
• Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
• Features and deprecations must be submitted against the 5.x branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

Thanks for the PR.
This should target the 4.4 branch as the logic is the same there. Can you rebase and change the PR base branch accordingly?

Well, at first I was thinking the same as the PR guide says all bug fixes should adress 4.4 branch, but here I'm fixing a method that was introduced later in 352aed3

Ok got it, yet this patch changes somethig that exists on 4.4:

Also, can you fix the CS issues reported by fabbot (see failing CI builds)?

I think this is beyond my knowledge of git, but applying this fix against 4.4 will result in conflicts with 352aed3 changes... An equivalent would be to not change anything ;)

Btw, I fixed CS issues but why fabbot wants me to remove these lines?

      *
      * @param int    $cp Code page in IBM/EBCDIC format
      * @param string $in User input
-     *
-     * @return string
      */
     private function resetIOCodepage(int $cp, string $in): string
     {

If Fabbot suggest to do CS fix on a unrelated line, you should just ignore it.

It's actually related but where's the problem? Should I add description to the returned string?

Symfony omits some annotations (@return, @param) when they don't provide more value that type declarations already provide.
Here @return string states something already known thanks to the string return type, so the patch can be applied :)

OK. Got it! Thanks

Thank you @orkan.

PR for 4.4 welcome btw!

Great work @orkan. Thank you and congratulations to your first contribution.

As Nicolas says, could you do a similar PR to the 4.4 branch? Robin said the bug exists there to but the logic has been moved around.

Hi @Nyholm.
Can you describe me please, what's the workflow here? I can see from the git history, that 4.4 is regulary merged into 5.2, right? So, if I remove something in 5.2 that existed in 4.4 and later introduce it again as a PR for 4.4, doesn't it mean it'll show up in 5.2 too?
That's the thing here. I don't want my fix for 4.4 to show up in 5.2... They'll be incompatible!

Of course.

Normally, bug fixes are merged in 4.4 (or the lowest branch they exists in). Every few days we merge 4.4 -> 5.2 -> 5.x.

later introduce it again as a PR for 4.4

I dont understand. We should create a PR to 4.4 to add setIOCodepage and resetIOCodepage, right?

They'll be incompatible!

Could you elaborate how they are incompatible? I dont really understand.

Ok. Nevermind. I pushed fix #41210 for 4.4. "I am going to sit back now and..." see what will be the result code after 4.4 > 5.2

Resolving merge conflicts is our maintainer's duty. This one was easy ;)