search

valeryan / vscode-phpsab

Php Sniffer and Beautifier for VS Code
https://marketplace.visualstudio.com/items?itemName=ValeryanM.vscode-phpsab
MIT License
58 stars 12 forks source link

Escape filePath and args passed to the phpcs command #145

Closed aslamdoctor closed 1 week ago

aslamdoctor commented 1 month ago

Fixes issue #144

Tested on Mac. Need to be tested on Windows.

jonathanbossenger commented 1 month ago

@aslamdoctor thanks for the PR. Of course, both the filePath and the standard would need to be escaped if they're both in the same location :facepalm: I'm annoyed I missed that :grin:

One question: in my original PR, to fix for just the filePath, instead of running the string through a string replace, I wrap the string in double quotes. This comes from my experiences in bash scripting, where one should wrap any variables in double quotes, to prevent scripts from breaking when input contains spaces, line feeds, glob characters and such.

I will freely admit I don't know a lot about the node child_process spawn method used to execute the phpcs command, but it seems to me that following the same process for escaping strings in the terminal made sense here. However, as it's a JavaScript code calling a method to execute the command, perhaps your solution is the better option.

Do you have any opinions on this?

@valeryan, as the original developer, I'd also appreciate any thoughts you might have on which solution is "better"; thanks.

valeryan commented 1 month ago

Seems like a good solution. Makes me wish I had time to write unit test though...

aslamdoctor commented 1 month ago

@jonathanbossenger I was also confused why adding the double quote didn't work. It was supposed to work. It's quiet tedious to debug this. But I believe when you pass anything as argument to a command on terminal, it shouldn't have space and needs to be escaped with \.

jonathanbossenger commented 1 month ago

Ok, thanks for the confirming folks, let's go ahead and get this merged.

jonathanbossenger commented 1 month ago

Makes me wish I had time to write unit test though...

@valeryan I hear you, but in my experience open source tools often don't get tests until someone comes along with enough time to add them. I will see if I can put some time aside for this as well.

jonathanbossenger commented 1 month ago

Need to be tested on Windows.

I can test this on Windows, hopefully tomorrow.

jonathanbossenger commented 4 weeks ago

Apologies for the delay in testing here folks, I discovered my Windows boot was borked, and I had to find some time to reinstall. I'm in the process of setting up my local WAMP environment, and then I can test on Windows.

aslamdoctor commented 4 weeks ago

Hey @jonathanbossenger no probs. Thanks for the heads up. Broken windows boot is always a pain 😔

danieliser commented 2 weeks ago

This is pretty annoying bug, any chance its getting merged/released soon?

roosalles commented 1 week ago

+1

jonathanbossenger commented 1 week ago

At this stage, my ability to test this on Windows is limited, so I'm going to merge it. If we get bug reports on Windows, we can work on those later.