Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix php for macOS Monterey #158

Closed
wants to merge 2 commits into from

Conversation

elalemanyo
Copy link

@elalemanyo elalemanyo commented Nov 29, 2021

Checks for PHP location to handle differences between Intel/M1 macs running Monterey.

Instead of running things like this:

php ./bin/index.php bower {query}

we now do:

if [ -f "/opt/homebrew/bin/php" ]; then
	/opt/homebrew/bin/php ./bin/index.php bower {query}
elif [ -f "/usr/local/bin/php" ]; then
    /usr/local/bin/php ./bin/index.php bower {query}
elif [ -f "/usr/bin/php" ]; then
    /usr/bin/php ./bin/index.php bower {query}
fi

Not sure if this is the best way but fixes the problem.

/issues/157

UPDATE
After @jeffbyrnes comments I decide to do it simpler, now all php calls use env $PHP_BIN, each user can set it as needed. I test it locally and works great.

$PHP_BIN ./bin/index.php bower {query}

image

@jeffbyrnes
Copy link
Collaborator

jeffbyrnes commented Nov 29, 2021

This can be condensed to

if [ -x "/opt/homebrew/bin/php" ]; then
    PHP_BIN=/opt/homebrew/bin/php
elif [ -x "/usr/local/bin/php" ]; then
    PHP_BIN=/usr/local/bin/php
elif [ -x "/usr/bin/php" ]; then
    PHP_BIN=/usr/bin/php
fi

$PHP_BIN ./bin/index.php bower {query}

Also, switch from -f to -x to ensure that the path in question not only exists & is a file, but is executable.

@elalemanyo
Copy link
Author

This can be condensed to

if [ -x "/opt/homebrew/bin/php" ]; then
    $PHP_BIN = /opt/homebrew/bin/php
elif [ -x "/usr/local/bin/php" ]; then
    $PHP_BIN = /usr/local/bin/php
elif [ -x "/usr/bin/php" ]; then
    $PHP_BIN = /usr/bin/php
fi

$PHP_BIN ./bin/index.php bower {query}

Also, switch from -f to -x to ensure that the path in question not only exists & is a file, but is executable.

@jeffbyrnes I get this error with your code suggestion:
image

@jeffbyrnes
Copy link
Collaborator

Trust me to typo this 🤦🏻‍♂️

Edited the original to be correct.

@jeffbyrnes
Copy link
Collaborator

Or… not… might have (re)learned a bash nuance…

@jeffbyrnes
Copy link
Collaborator

Right… spaces are meaningful when assigning variables. Derp! All fixed & really works now, sorry about that.

@jeffbyrnes
Copy link
Collaborator

That said, I think Alfred is now, as of the most recent version, set to do this internally if you use a relative php command in your workflows.

@elalemanyo
Copy link
Author

@jeffbyrnes would be a bad idea to do it maybe simpler:
image

I am using now Alfred 4.6.1 and was not working for me 🤷‍♂️

@jeffbyrnes
Copy link
Collaborator

Well now that’s an idea! Though it does leave it up to the end-user to know where their copy of PHP, if any, is installed.

@elalemanyo
Copy link
Author

Well now that’s an idea! Though it does leave it up to the end-user to know where their copy of PHP, if any, is installed.

yes, that is true. But this is a workflow for developers, so this should not be a big issue. Just open a terminal and write which php

Copy link

@dioxide dioxide left a comment

Choose a reason for hiding this comment

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

It works fine on Monterey.

@learn2reid
Copy link

learn2reid commented Jan 18, 2022

Can also update the PATH variable in environment variable & you won't have to change any of the other values.
ex)

PATH: /usr/local/bin:/usr/local/sbin:~/bin

@varp
Copy link
Contributor

varp commented Jan 24, 2022

@jeffbyrnes When this will come up to the master?

@varp
Copy link
Contributor

varp commented Jan 24, 2022

Can also update the PATH variable in environment variable & you won't have to change any of the other values. ex)

PATH: /usr/local/bin:/usr/local/sbin:~/bin

@learn2reid Could give us more explanation about where do we set PATH env vars? In each script invocation in the workflow?

@jeffbyrnes
Copy link
Collaborator

@varp I haven’t had much time to devote to this lately, but I’ll see what I can do this week.

As for @learn2reid’s suggestion, if the workflow is using the unqualified php, so long as your $PATH is set to include wherever you have php installed, it should work (but that needs to be verified).

I’ll try that, first, before we go adding complications, and thus fragility, to the workflow.

@jeffbyrnes
Copy link
Collaborator

Ok, a quick test: it seems that Alfred runs scripts & don’t inherit $PATH from the current user. However, adding a $PATH env var to the workflow & setting it does work. That’s less fragile & less of a change than what’s been proposed here with the $PHP_BIN_PATH, so I think that’s the best way forward right now?

image

@varp
Copy link
Contributor

varp commented Jan 25, 2022

Ok, a quick test: it seems that Alfred runs scripts & don’t inherit $PATH from the current user. However, adding a $PATH env var to the workflow & setting it does work. That’s less fragile & less of a change than what’s been proposed here with the $PHP_BIN_PATH, so I think that’s the best way forward right now?

image

@jeffbyrnes Firstly , thank you!
@elalemanyo I totally agree with @jeffbyrnes. I think use of $PATH less fragile and more common and standard approach to execute binaries from custom paths

@elalemanyo
Copy link
Author

Ok, a quick test: it seems that Alfred runs scripts & don’t inherit $PATH from the current user. However, adding a $PATH env var to the workflow & setting it does work. That’s less fragile & less of a change than what’s been proposed here with the $PHP_BIN_PATH, so I think that’s the best way forward right now?
image

@jeffbyrnes Firstly , thank you! @elalemanyo I totally agree with @jeffbyrnes. I think use of $PATH less fragile and more common and standard approach to execute binaries from custom paths

@varp yes, I agree. The solution from @jeffbyrnes is better. Feel free to close this PR.
Thanks @jeffbyrnes.

@jeffbyrnes
Copy link
Collaborator

Alright! Glad that works well folks.

@jeffbyrnes jeffbyrnes closed this Jan 25, 2022
@elalemanyo elalemanyo deleted the fix/php_monterey branch January 25, 2022 21:23
@varp
Copy link
Contributor

varp commented Jan 26, 2022

@jeffbyrnes I've just tested your suggested solution with path /usr/local/bin:$PATH.
One thing I should notice, is that Alfred doesn't support $VAR substitution, so you need to specify also standard locations (you can get it from /etc/paths) /usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin for do not break invocation of core utils like: mkdir for bash scripts

@learn2reid
Copy link

@jeffbyrnes glad it worked for others too.

@varp Yes, if you refer back to my original comment with my solution, I did not add $PATH to the variable. I just added the PATH from login invocation.

Can also update the PATH variable in environment variable & you won't have to change any of the other values. ex)

PATH: /usr/local/bin:/usr/local/sbin:~/bin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants