httptoolkit / httptoolkit-server

The backend of HTTP Toolkit
https://httptoolkit.com
GNU Affero General Public License v3.0
433 stars 96 forks source link

Update php #87

Closed kimrow closed 1 year ago

kimrow commented 1 year ago

fix of the Applications/HTTP Toolkit.app/Contents/Resources/httptoolkit-server/overrides/path/php: line 11: bad substitution: no closing "" inecho $HTTP_TOOLKIT_OVERRIDE_PATH

pimterry commented 1 year ago

Hi @kimrow, thanks for looking at this!

I'm a bit confused on what's going on here though, as the current code doesn't actually error on my machine - it seems to work just fine! And that's what I'd expect - the /php is inside the backticks, and so should be treat as part of the argument to echo, and returned as a suffix to the $HTTP_TOOLKIT_OVERRIDE_PATH string.

It would be good to work out why this is failing for you but not for me before we change it, both to document the issue and so that we can test it definitely works correctly everywhere afterwards.

Can you share the version info from /usr/bin/env bash --version? That might have some clues. In my case I'm using GNU bash v5.2.2 on Linux.

kimrow commented 1 year ago

Hi @pimterry ,

I'm actually using zsh on arm64 mac

(base) ➜  ~ /usr/bin/env bash --version
GNU bash, version 3.2.57(1)-release (arm64-apple-darwin22)
Copyright (C) 2007 Free Software Foundation, Inc.
(base) ➜  ~ zsh --version
zsh 5.8.1 (x86_64-apple-darwin22.0)
pimterry commented 1 year ago

Which shell you're using shouldn't matter - the shebang on the first line means the script will always be run by your default bash shell (whatever /usr/bin/env bash runs).

I've done some testing (turns out you can access every released version of Bash via Docker) and I can reproduce this issue there - it works in Bash v5, but not in v3. Thanks for reporting this! I'm sure lots of others are using similar Bash versions and I was totally unaware of the issue.

Unfortunately the fix here still doesn't work for all cases though, as it seems that in some setups the second command (without the preceeding colon) ends up replacing PHP_INI_SCAN_DIR with php//php, presumably because it interprets the final /php as the string to replace? Hard to say. The definition for how this should work is here (the ${parameter//pattern/string} case) but parsing it is all a bit tricky since the slashes are both separators and part of the value.

Fortunately though, I have managed to get it working by just extracting the var+/php logic into a separate variable before the search & replace, which seems to work nicely in my testing. I've pushed the commit here. Can you try that out locally and confirm it works for you?

pimterry commented 1 year ago

This passes the tests and fixes the issue locally for me, so I'm going to assume it's good to go and merge it now. If you do get time to test this in your environment, do let me know how it goes!