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.

Sign up for GitHub

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

Revert back to requiring php 5.6 as the minimum php version #2665

Merged
merged 18 commits into from
Mar 28, 2018
Merged

Revert back to requiring php 5.6 as the minimum php version #2665

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
2c8d8c0
Change minimum version back to php 5.6
japerry Mar 21, 2018
01307a0
Revert "Change minimum version back to php 5.6"
Mar 21, 2018
4791b66
Revert "Require PHP 7.1 in all the right places. (#2611)"
Mar 21, 2018
0e4ceff
Change Minimum version back to 5.6 from php 7.
Mar 21, 2018
5300d43
Revert "Require PHP 7 minimum. (#2601)"
Mar 21, 2018
cf4c984
Update DrupalVM default config.
japerry Mar 21, 2018
76d4d85
Update core travis.yml to use php 5.6
japerry Mar 21, 2018
7620008
Update pipelines to use php 5.6
japerry Mar 21, 2018
06435a5
travis should also deploy 5.6 not 7.1
japerry Mar 21, 2018
42ffa86
Try wrapping config around a different var name.
Mar 22, 2018
146c737
Revert roboconfig trial and remove trail from webserverhook.
Mar 22, 2018
290e2e1
Remove associated use statement.
Mar 22, 2018
aa4ece4
Remove all traits from webserver hook as they’re inherited from BltTa…
Mar 22, 2018
b689763
Add php7.1 as an additional test environment.
Mar 22, 2018
3de1144
Merge branch '9.x' into 9.x
japerry Mar 25, 2018
eb64dbb
Update the travis php version, and provide doc comment on how to use …
Mar 25, 2018
be8c60d
Merge branch '9.x' into 9.x
japerry Mar 27, 2018
522ed4e
Update .travis.yml
grasmash Mar 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ language: php
dist: trusty

php:
- 5.6
- 7.1

env:
Expand Down Expand Up @@ -83,5 +84,5 @@ deploy:
skip_cleanup: true
on:
branch: $DEPLOY_SOURCE_BRANCH
php: 7.1
php: 5.6
condition: $DRUPAL_CORE_VERSION = default
2 changes: 1 addition & 1 deletion composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
"type": "composer-plugin",
"license": "GPL-2.0-only",
"require": {
"php": ">=7.1",
"php": ">=5.6",
"composer-plugin-api": "^1.0.0",
"composer/installers": "^1.2.0",
"composer/semver": "^1.4",
Expand Down
2 changes: 1 addition & 1 deletion scripts/drupal-vm/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ npm_config_prefix: "/home/{{ drupalvm_user }}/.npm-global"
installed_extras: ${installed_extras}

# PHP 7
php_version: "7.1"
php_version: "5.6"
Copy link
Contributor

Choose a reason for hiding this comment

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

@geerlingguy Are more changes than this required?

Copy link
Contributor

Choose a reason for hiding this comment

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

No not for the VM... but do we really want to default to 5.6? Shouldn’t we default to whatever Acquia Cloud currently defaults to (7.1 IIRC)?

Copy link
Contributor

Choose a reason for hiding this comment

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

The trouble is that if your host is 5.6 and your VM is 7, you get some issues with Composer. Maybe we should look into defaulting the "config" in composer.json to PHP 7. I know that adding it to template/composer.json and composer.required.json do not work. We'd need to actually modify the generated composer.json after project creation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually run 5.6 on host and 7 in VM. TBH, I haven't really had a problem with it... I usually run $ composer commands on the host, and BLT will run composer in the VM... knock on wood but I haven't seen much yet. Just wanted to share... ha.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you use the platform config setting in composer.json?

Copy link
Contributor

@danepowell danepowell Mar 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Most people I've seen (including myself) run a higher PHP version on the host than the VM. i.e. 7.2 on the host, and 5.6 or 7.1 in the VM. The reason being pretty obvious, that the VM replicates an Acquia Cloud or other hosting environment that generally lags behind general desktop software in terms of PHP releases.

I'd really like to see the VM default to the highest version possible in order to match the host machine, or at least never be "behind" the host, which leads to some nasty problems. For instance, running composer update on the host breaks everything by pulling in 7.1 dependencies that won't run in the VM.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you use the platform config setting in composer.json?

I don't. I mean, below is my output, haha. I haven't upgraded my host b/c I've just been lazy, TBH, and I haven't seen a need to. I do a lot of composer updates for our project too, and just haven't run into any issues...

If you need me to try anything out, LMK - I'm happy to report back.

$ php -v
PHP 5.6.31 (cli) (built: Aug 17 2017 16:41:40) 
Copyright (c) 1997-2016 The PHP Group
Zend Engine v2.6.0, Copyright (c) 1998-2016 Zend Technologies
    with Zend OPcache v7.0.6-dev, Copyright (c) 1999-2016, by Zend Technologies
    with Xdebug v2.5.3, Copyright (c) 2002-2017, by Derick Rethans

$ vagrant ssh -c 'php -v'
PHP 7.0.20-2~ubuntu16.04.1+deb.sury.org+1 (cli) (built: Jun 14 2017 05:33:01) ( NTS )
Copyright (c) 1997-2017 The PHP Group
Zend Engine v3.0.0, Copyright (c) 1998-2017 Zend Technologies
    with Zend OPcache v7.0.20-2~ubuntu16.04.1+deb.sury.org+1, Copyright (c) 1999-2017, by Zend Technologies
    with Xdebug v2.5.0, Copyright (c) 2002-2016, by Derick Rethans

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe b/c my VM is 7.0 instead of 7.1? I really need to update both my host and my guest, haha.

Copy link
Contributor Author

@japerry japerry Mar 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm IMHO, except for grabbing the initial code you really should never run composer on the host environment. The part that matters here is that your VM matches your Production environment. If I could have my host not have ANY dev tools (php, drush, composer, etc) I would.

While Acquia Cloud -defaults- to 7.1 for new instances, it certainly isn't what most of our clients are running. And until ACE+ACSF are fully off 5.6, we shouldn't be shipping a VM that can break prod.. which is the whole point of Vagrant in the first place. Since 7.0 much more backwards compatible with 5.6 than visa-versa, we need to use the least common denominator.

php_packages_extra:
- "php{{ php_version }}-bz2"
- "php{{ php_version }}-imagick"
Expand Down
2 changes: 1 addition & 1 deletion scripts/pipelines/acquia-pipelines.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ version: 1.1.0
services:
- mysql
- php:
version: 7.1
version: 5.6

variables:
global:
Expand Down
12 changes: 8 additions & 4 deletions scripts/travis/.travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@ sudo: true
language: php
dist: trusty

# Adjust the version of PHP to match your production environment.
# Making this version number greater than the production environment can unintended consequences including a
# non-functional prod environment.
php:
- 7.1
- 5.6

matrix:
fast_finish: true
Expand Down Expand Up @@ -56,22 +59,23 @@ install:
script:
- source ${BLT_DIR}/scripts/travis/run_tests

# The version below MUST match the php version indicated at the top of this file, otherwise the artifact won't build.
deploy:
- provider: script
script: "${BLT_DIR}/scripts/travis/deploy_branch"
skip_cleanup: true
on:
branch: develop
php: 7.1
php: 5.6
- provider: script
script: "${BLT_DIR}/scripts/travis/deploy_branch"
skip_cleanup: true
on:
branch: master
php: 7.1
php: 5.6
- provider: script
script: "${BLT_DIR}/scripts/travis/deploy_tag"
skip_cleanup: true
on:
tags: true
php: 7.1
php: 5.6
11 changes: 0 additions & 11 deletions src/Robo/Hooks/WebServerHook.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,13 @@
namespace Acquia\Blt\Robo\Hooks;

use Acquia\Blt\Robo\BltTasks;
use Acquia\Blt\Robo\Common\IO;
use Acquia\Blt\Robo\Config\ConfigAwareTrait;
use Acquia\Blt\Robo\Inspector\InspectorAwareTrait;
use Consolidation\AnnotatedCommand\CommandData;
use League\Container\ContainerAwareTrait;
use Psr\Log\LoggerAwareTrait;

/**
* Starts and kills Drush webserver for @launchWebServer annotation.
*/
class WebServerHook extends BltTasks {

use ConfigAwareTrait;
use ContainerAwareTrait;
use LoggerAwareTrait;
use InspectorAwareTrait;
use IO;

protected $serverUrl;
protected $serverPort;

Expand Down
2 changes: 1 addition & 1 deletion src/Robo/Inspector/Inspector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -788,7 +788,7 @@ public function issueEnvironmentWarnings($command_name) {
* @throws \Acquia\Blt\Robo\Exceptions\BltException
*/
public function warnIfPhpOutdated() {
$minimum_php_version = 7;
$minimum_php_version = 5.6;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not keep a warning in here, but don't throw an exception? Maybe suggest users to move off 5.6 at their convenience...

Copy link
Contributor

Choose a reason for hiding this comment

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

Or, have a warning and suggest users move to 5.6 if they really need it, but not encourage it as a default. Otherwise, this entire issue feels like a step backwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is not currently a warning but rather an exception, causing BLT to fail. I agree -- giving some warning that you're running 5.6 would be good, although Drupal already does that.

$current_php_version = phpversion();
if ($current_php_version < $minimum_php_version) {
throw new BltException("BLT requires PHP $minimum_php_version or greater. You are using $current_php_version.");
Expand Down