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

valet php can't be defined to run on a site linked to a name different than its directory #1367

Closed
scrummitch opened this issue Feb 14, 2023 · 11 comments · Fixed by #1370
Closed
Assignees

Comments

@scrummitch
Copy link

Description:

If I want to run a php command such as: valet php -S 0.0.0.0:3030 -t ./public it will use the default installed /usr/bin/php rather than the isolated php version if my site is isolated.

It would be preferred if I could pick my php version or name the site when running php commands like this, for example:

valet php --site=mycustomsite -S 0.0.0.0:3030 -t ./public OR valet php [email protected] -S 0.0.0.0:3030 -t ./public

Steps To Reproduce:

In a folder OUTSIDE of your normally parked folder, or on a subdomain, isolate a site with a custom name and then run valet php -v, you will see that it does not use the isolated php version.

Diagnosis

This is not related to an internal issue that would be solvable using diagnose

@scrummitch
Copy link
Author

For reference, I can get this to work by running:

 /usr/local/Cellar/[email protected]/7.4.33_1/bin/php -S 0.0.0.0:3030 -t ./public

@driesvints
Copy link
Member

cc @mattstauffer

@mattstauffer
Copy link
Collaborator

mattstauffer commented Feb 17, 2023

I can't reproduce this issue.

Here are the steps I took to try to reproduce this. Notice I'm seeing Valet 3.3.3 because I'm working on dev-master, but I am pretty certain 3.3.2 would have the same outcome.

$ valet --version
Laravel Valet 3.3.3

$ cd /tmp

$ mkdir issue-1367

$ cd issue-1367

$ valet link
A [issue-1367] symbolic link has been created in [/Users/mattstauffer/.config/valet/Sites/issue-1367].

$ php -v
PHP 8.2.1 (cli) (built: Jan 12 2023 19:14:53) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.2.1, Copyright (c) Zend Technologies
    with Zend OPcache v8.2.1, Copyright (c), by Zend Technologies

$ valet isolate [email protected]
Updating PHP configuration for [email protected]...
Restarting [email protected]...
Restarting nginx...
The site [issue-1367.test] is now using [email protected].

$ valet php -v
PHP 8.1.14 (cli) (built: Jan 12 2023 19:52:54) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.1.14, Copyright (c) Zend Technologies
    with Zend OPcache v8.1.14, Copyright (c), by Zend Technologies

@mattstauffer
Copy link
Collaborator

Whoops, I just realized you said "isolate your site with a custom name." Let me check that out.

@mattstauffer mattstauffer changed the title Isolated sites valet php uses incorrect version if outside main directory Isolated sites valet php uses incorrect version if outside main directory and linked with a different name than their folder Feb 17, 2023
@mattstauffer
Copy link
Collaborator

I'm actually surprised to get this error:

image

I think in my mind i didn't expect you to be able to isolate the same folder different ways depending on the link. Can you even do that? Testing now...

mkdir /tmp/issue-1367
cd /tmp/issue-1367
valet link isolate-php80
valet link isolate-php81
valet isolate --site=isolate-php80 [email protected]
valet isolate --site=isolate-php81 [email protected]

I then set public/index.php to just show the output of phpversion(); and visited both sites in my browser.

Lo and behold... we can have two different sites using the same local folder isolated to two different versions of PHP. 🤯

Thinking "out loud".. I think I expected this behavior:

  • Change to the directory
  • Link the directory
  • Run valet isolate [email protected] and expect it to isolate this site

Now I'm trying to think if that's viable if we're also supporting this idea that different links to the same site can be isolated differently.

@mattstauffer mattstauffer changed the title Isolated sites valet php uses incorrect version if outside main directory and linked with a different name than their folder valet php can't be defined to run on a site linked to a name different than its directory Feb 17, 2023
@mattstauffer
Copy link
Collaborator

mattstauffer commented Feb 17, 2023

OK, so this has nothing to do with the main parked directory or not.

Here's what it is: valet php runs valet which-php to figure out which version of PHP to use. It's proxied in the file valet , which is a bash script, so it doesn't have all the conveniences of the rest of the codebase--for example, the easy ability to add --site=whatever type specifications.

valet/valet

Lines 82 to 87 in 044bdc1

# Proxy PHP commands to the "php" executable on the isolated site
elif [[ "$1" = "php" ]]
then
$(php "$DIR/cli/valet.php" which-php) "${@:2}"
exit

which-php can be given a site to work with, when you're running it on its own. E.g. valet which-php custom-linked-site. But when it's being called by valet php, it doesn't specify the site, which means it's just looking for which version of PHP the current directory is assigned. That'll be either the root Valet version, this directory's .valetphprc definition, or this directory's isolation version.

Solving this would require, I think, writing code in Bash to parse --site=whatever in the Bash script so it can then be parsed and passed along to which-php.

I understand why you want this feature... it seems in sync with the ability to isolate each link to a folder uniquely......... I just don't think it's worth it. 😬

But I'll keep thinking about it and see if I can come up with a simple solution.

@mattstauffer
Copy link
Collaborator

@scrummitch Would you test this PR out please? #1370

@scrummitch
Copy link
Author

@mattstauffer Its a bit of a weird solution because the php command takes so many more arguments, but it does look like it works 😄

  1. Installed this commit locally in a new laravel project (not sure if this is appropriate but it works) "laravel/valet": "dev-master#5b75cfe"
  2. Ran ./vendor/bin/valet php and got output: PHP 8.2.2 (cli) (built: Feb 5 2023 12:51:27) (NTS)
  3. Ran ./vendor/bin/valet php --site=transformd -v (transformd is my custom site name and got output : PHP 7.4.33 (cli) (built: Jan 21 2023 07:11:51) ( NTS )

LGTM

@scrummitch
Copy link
Author

Tested Composer and it is working too :)

@mattstauffer
Copy link
Collaborator

Thanks @scrummitch!

As a user of this, do you think it would be a pain if it only works if --site is the first parameter (which it is right now), or would you think folks/yourself would expect it to work anywhere in the parameter list? or would folks at least expect to be able to make it the first or the last?

@scrummitch
Copy link
Author

I have would expected it to be the first parameter, otherwise it will get confused with the php and composer argument list. Your decision seems the correct one.

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

Successfully merging a pull request may close this issue.

3 participants