Skip to content

Fix proc_open_compat util on Windows#5689

Merged
danielbachhuber merged 1 commit intowp-cli:masterfrom
lipemat:fix-windows-commands
Oct 28, 2022
Merged

Fix proc_open_compat util on Windows#5689
danielbachhuber merged 1 commit intowp-cli:masterfrom
lipemat:fix-windows-commands

Conversation

@lipemat
Copy link
Contributor

@lipemat lipemat commented Oct 14, 2022

Wrapping the commands in double quotes conflicts with various flags also using double quotes.

For example when running db export you get.

Warning: Failed to get current character set of the posts table. Reason: '"mysql --no-defaults --no-auto-rehash --batch --skip-column-names --host="localhost" --user="mat" --default-ch aracter-set="utf8" --execute="SELECT' is not recognized as an internal or external command, operable program or batch file.

Removing the quotes resolves the issue.

@lipemat
Fix proc_open commands on Windows
1e244ae 
Wrapping the commands in double quotes conflicts with various flags also using double quotes. 

For example when running `db export` you get.

`Warning: Failed to get current character set of the posts table. Reason: '"mysql --no-defaults --no-auto-rehash --batch --skip-column-names --host="localhost" --user="mat" --default-ch
aracter-set="utf8" --execute="SELECT' is not recognized as an internal or external command,
operable program or batch file.`

Removing the quotes resolves the issue.
@lipemat lipemat requested a review from a team as a code owner October 14, 2022 17:55
This was referenced Oct 14, 2022
Closed
Closed
@cjhaas
Copy link

cjhaas commented Oct 17, 2022

Since proc_open supports environment variables as the fifth parameter for all platforms, would removing the is_windows() check and using (and renaming) _proc_open_compat_win_env be an option for everyone?

@lipemat
Copy link
Contributor Author

lipemat commented Oct 17, 2022

Since proc_open supports environment variables as the fifth parameter for all platforms, would removing the is_windows() check and using (and renaming) _proc_open_compat_win_env be an option for everyone?

Yes. I am open to this pattern.

@cjhaas
Copy link

cjhaas commented Oct 17, 2022

proc_open_compat was added in #4572, which also included some unit tests . As far as I can tell, those tests run regardless of platform, so presumably they've been passing for years now. There would be a tiny bit of overhead by doing this, technically, but I think it would be negligible in grander scheme of things.

If I get a chance I might try to make that change and get the tests to run, although it would be great if someone more familiar with running all the tests could do this instead.

@danielbachhuber danielbachhuber added this to the 2.8.0 milestone Oct 28, 2022
danielbachhuber
danielbachhuber approved these changes Oct 28, 2022
View reviewed changes
Copy link
Member

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @lipemat !

Landing so we can let it soak and see if any issues come up: #5521 (comment)

@danielbachhuber danielbachhuber changed the title Fix proc_open commands on Windows Fix proc_open_compat util on Windows Oct 28, 2022
@danielbachhuber danielbachhuber merged commit 2c7bd19 into wp-cli:master Oct 28, 2022
@lipemat lipemat deleted the fix-windows-commands branch February 24, 2023 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@danielbachhuber danielbachhuber danielbachhuber approved these changes

Assignees

No one assigned

Labels

bug scope:framework

Projects

None yet

Milestone

2.8.0

Development

Successfully merging this pull request may close these issues.

3 participants

@lipemat @cjhaas @danielbachhuber