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

Set php version #2488

Closed
wants to merge 3 commits into from
Closed

Set php version #2488

wants to merge 3 commits into from

Conversation

Mte90
Copy link
Member

@Mte90 Mte90 commented Jun 9, 2021

#2309

Checks

  • I've updated the changelog.
  • I've tested this PR
  • This PR is for the develop branch not the stable branch.
  • This PR is complete and ready for review.

@update-docs
Copy link

update-docs bot commented Jun 9, 2021

Thanks for opening this pull request! Make sure CHANGELOG.md gets updated with this change, additionally any docs that need updated can be found at https://github.com/Varying-Vagrant-Vagrants/varyingvagrantvagrants.org

GitHub
The VVV docs and website. Contribute to Varying-Vagrant-Vagrants/varyingvagrantvagrants.org development by creating an account on GitHub.


source /srv/provision/provision-helpers.sh

phpversion=$(get_config_value "general.default_php")
Copy link
Member Author

Choose a reason for hiding this comment

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

It is missing the default from VVV.

@tomjn
Copy link
Member

tomjn commented Jun 23, 2021

I don't like the idea of exposing this to users. It might be nice to have a bash function for provisioners to make things like the PHP provisioners easier to wrangle, but this is attempting to solve a problem with a sledge hammer, it'll introduce new problems which are harder to solve without ever really solving the original problem. This is an X Y solution that needs a fundamental rethink of what it's trying to do.

For example, if I have 3 sites, and the second site requires PHP 7.3 to provision, then this would let me fix that sites provisioning. But now the 3rd site which requires 7.4 fails to provision. Setting 7.4 as the default would fix that but now site 2 fails. Meanwhile the dashboard is broken because we require 7.4 but the user has chosen 7.3.

Instead, a feature that lets you specify an actual PHP version on a site, that then configures the nginx upstream and runs the utility to make sure it's installed, and changes the PHP default before running the provisioner then changes it back when it's done/fails, that would be a better option.

@Mte90
Copy link
Member Author

Mte90 commented Jun 23, 2021

Right now we already set the php version on provisioner. This was to help us to change the php version on VVV and also for the people that asks often how to change the default php version.

@tomjn
Copy link
Member

tomjn commented Jun 23, 2021

Right now we already set the php version on provisioner.

We just allow the nginx upstream to be set, which isn't quite the same thing, e.g. the site provisioner still runs on PHP 7.4 even if nginx_upstream is set to php72

This was to help us to change the php version on VVV and also for the people that asks often how to change the default php version.

People who ask this are just doing the problem X Y thing trying to solve other issues. E.g. xhgui not working on PHP 7.4, or a specific site requiring 7.2 or 7.3

Imagine it this way, we serve cheese and you're lactose intolerant, so we switch everybody to peanut butter and you're happy, but the person next to you has anaphylactic shock and goes to hospital.

  • ✅ a bash function is good as it simplifies provisioning
  • ❌ a config.yml option and a command are bad as it introduces new sources for bugs and will be misused by people trying to solve unrelated bugs

There's also the issue that if I set 7.4 as the default in the config, then VVV upstream switches to 8.0 as the default, when I update none of my sites or the dashboard will work because they use the wrong nginx upstream.

What we actually need is:

  wordpress-two:
    repo: https://github.com/Varying-Vagrant-Vagrants/custom-site-template.git
    php: 7.2
    hosts:
      - two.wordpress.test

With this we can auto-configure the upstream, change the default PHP but just for that specific site, we could even modify the terminal prompt to modify PATH so that php is mapped to the correct version when they cd into that directory.

A configurable default PHP is a bad thing, not a good thing. It's not something we want. If a user wants to use a non-default version of PHP for their site, they can already do that, and we shouldn't encourage workarounds for bugs, we should fix the bugs instead.

@Mte90
Copy link
Member Author

Mte90 commented Jun 24, 2021

So that change is not for VVV but for the provisioner, so the php commands on that will use that version for everything.

Instead my idea was to create a tool to help us to change the default php version, so maybe we can have a constant internally and remove that stuff from nginx.

@tomjn
Copy link
Member

tomjn commented Jun 24, 2021

So that change is not for VVV but for the provisioner, so the php commands on that will use that version for everything.

Extending the custom site template would mean conflicting instructions on setting up PHP, or, parsing the nginx upstream value to try and extract the PHP version, both of which are confusing and problematic for users. This is not the solution.

Instead my idea was to create a tool to help us to change the default php version, so maybe we can have a constant internally and remove that stuff from nginx.

We cannot allow the default PHP to be configured via config.yml.

@tomjn
Copy link
Member

tomjn commented Jun 24, 2021

It seems like this PR conflates several things:

  • letting users set the default PHP version
  • making the PHP configs generic
  • adding CLI commands for changing the current default version of PHP

letting users set the default PHP version

A hard no, this must not be possible.

making the PHP configs generic

Are we sure that newer version of PHP won't require changes? What benefit do we have from this

adding CLI commands for changing the current default version of PHP

I can see value in this but I think that changing the default PHP isn't the solution, but rather changing the PATH. This way, we can do it on a per session basis that's safe for multiple sessions and parallel commands.

@Mte90
Copy link
Member Author

Mte90 commented Jun 25, 2021

So we can close this PR and update the ticket with a recap of what we need with al those details.

@Mte90 Mte90 closed this Jun 25, 2021
@tomjn tomjn deleted the php-switcher branch June 15, 2022 20:31
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.

None yet

2 participants