Corrects previous fixes that dealt with the mojibake problem on Windows where an OEM code page was applied to an input string and then messed with PHP.internal_encoding setting used by the script. This caused strings with different encodings to be displayed on the console output.

orkan requested a review fromchalasr as acode owner

May 5, 2021 16:12

carsonbot added Status: Needs Review Bug Console labels

May 5, 2021

Copy link

carsonbot commentedMay 5, 2021

Hey!

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

Symfony has acontribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (seehttps://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://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

Copy link

Member

chalasr commentedMay 6, 2021

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?

chalasr added this to the4.4 milestone

May 6, 2021

Copy link

ContributorAuthor

orkan commentedMay 6, 2021

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

Copy link

Member

chalasr commentedMay 6, 2021

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

symfony/src/Symfony/Component/Console/Helper/QuestionHelper.php

Lines 111 to 114 inbeca689

	if (\function_exists('sapi_windows_cp_set')) {
	// Codepage used by cmd.exe on Windows to allow special characters (éàüñ).
	@sapi_windows_cp_set(1252);
	}

.
So I guess we need an equivalent PR for 4.4. It'd be great if you could submit one. Otherwise, no worries, someone else will do (maybe me).

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

Copy link

ContributorAuthor

orkan commentedMay 6, 2021

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     {

Copy link

Member

Nyholm commentedMay 6, 2021

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

Copy link

ContributorAuthor

orkan commentedMay 6, 2021

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

Nyholm reviewed

May 6, 2021

View reviewed changes

src/Symfony/Component/Console/Helper/QuestionHelper.php OutdatedShow resolvedHide resolved

Copy link

Member

chalasr commentedMay 6, 2021•
edited
Loading

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 thestring return type, so the patch can be applied :)

chalasr modified the milestones:4.4,5.2

May 6, 2021

Copy link

ContributorAuthor

orkan commentedMay 6, 2021

OK. Got it! Thanks

[Console] Fix Windows code page support

be68682

nicolas-grekas changed the title~~[Console] Add Windows code page support~~[Console] Fix Windows code page support

May 7, 2021

nicolas-grekas changed the base branch from5.x to5.2

May 7, 2021 14:24

nicolas-grekas requested review fromdunglas,lyrixx,wouterj,xabbuh andyceruto ascode owners

May 7, 2021 14:24

nicolas-grekas force-pushed the5.x branch fromb4359ec tobe68682Compare

May 7, 2021 14:25

Copy link

Member

nicolas-grekas commentedMay 7, 2021

Thank you@orkan.

nicolas-grekas merged commit53e47b3 intosymfony:5.2

May 7, 2021

fabpot mentioned this pull request

May 9, 2021

Release v5.3.0-BETA3#41144

Merged

orkan mentioned this pull request

May 11, 2021

[Console] Fix Windows code page support#41174

Merged

nicolas-grekas added a commit that referenced this pull request

May 11, 2021

bug#41174[Console] Fix Windows code page support (orkan)

2289c32

This PR was submitted for the 5.x branch but it was merged into the 5.2 branch instead.Discussion----------[Console] Fix Windows code page supportMy previous PR#41113 was corrected by `@nicolas`-grekas on3bac7fe. He introduced logical changes in the code which resulted in incorrect behaviour.The basic idea was to restore the I/O codepage as soon as you get console input. And you have to do this even if `fgets()` returns **false**, because otherwise you'll leave the changed codepage for the rest of the script execution - and that's bad!Commits-------044b585 [Console] Fix Windows code page support

Copy link

Member

nicolas-grekas commentedMay 11, 2021

PR for 4.4 welcome btw!

Copy link

Member

Nyholm commentedMay 11, 2021

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? Robinsaid the bug exists there to but the logic has been moved around.

Copy link

ContributorAuthor

orkan commentedMay 11, 2021

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!