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

japerry · 2018-03-21T20:23:02Z

This reverts back all the php7 changes that was performed after beta3 was released.

Note: We should be documenting that if you're using php7 locally, you'll need to manually update configuration files to support php7. While its unfortunate that we support both versions of PHP, we must continue to do so until our product roadmap makes it EOL, which is slated for the end of 2018.

This reverts commit 2c8d8c0.

This reverts commit ce3d78d.

This reverts commit ceb6e8e. # Conflicts: # src/Robo/Commands/Sync/SyncCommand.php # src/Update/Updates.php

This reverts commit 07504a4.

grasmash · 2018-03-21T20:30:40Z

I think we'd need to change Drupal VM's default version of PHP, or else somehow enforce a composer platform requirement of PHP 7 when Drupal VM is used.

https://github.com/acquia/blt/blob/9.x/scripts/drupal-vm/config.yml

grasmash · 2018-03-21T20:34:07Z

Also need to change PHP version in:

japerry · 2018-03-21T20:41:57Z

Ahh yup yup, I've gone and made those updates. I didn't see anything else in a search/replace for 7.1 or 7 although that had a lot of noise. We'll see what travis thinks ;)

dpagini · 2018-03-21T21:45:03Z

src/Robo/Inspector/Inspector.php

@@ -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;


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

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.

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.

dpagini · 2018-03-21T21:46:13Z

.travis.yml

@@ -3,7 +3,7 @@ language: php
 dist: trusty

 php:
-  - 7.1
+  - 5.6


Isn't it possible to test with both versions? via the matrix? I don't know enough about it, but just seems like it would be worth considering.

…sks.

grasmash · 2018-03-22T13:27:17Z

Weird, the last build exited prematurely. It's a false positive.

fatal: Invalid symmetric difference expression fa8dc6766ae72fe72c58a0d6eb0f7629188e4b52...b6897635c12000f994709d957d2a87351e4f9c9f

Somehow this has exposed and triggered a bug in the exit_early script.

grasmash · 2018-03-22T14:20:37Z

Cleared caches and restarted builds.

grasmash · 2018-03-22T15:45:51Z

Passing.

However, I'm still not convinced that this should be merged. Can we continue the conversation here? #2661

I would like to understand what the blockers are for updating to PHP 7, and also why extending LTS for 8.9.x is not sufficient.

grasmash · 2018-03-23T13:46:09Z

scripts/travis/.travis.yml

@@ -1,4 +1,4 @@
-sudo: true
+sudo: false


Need to update line 6. Also, is this sudo change necessary?

Line 6 of this file also needs to be 5.6.

Fixed! I also added a doc comment to advise the user this should match whatever version their prod environment is in.

grasmash · 2018-03-23T13:57:41Z

scripts/drupal-vm/config.yml

@@ -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"


@geerlingguy Are more changes than this required?

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)?

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.

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.

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

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.

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

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

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.

grasmash · 2018-03-23T13:58:45Z

Pinging @danepowell for visibility.

grasmash · 2018-03-23T19:43:18Z

scripts/travis/.travis.yml

@@ -25,6 +25,7 @@ cache:
  - "$HOME/.drush/cache"
  - "$HOME/.npm"
  - "$HOME/.nvm"
+  - "vendor"


Why add this back?

I was just reverting the changes associated with php7, if this wasn't meant to be part of that commit (or was two commits in one), then this should be taken out of the PR.

Let's not add it back.

…php versions.

…cquia#2665)" This reverts commit f43c708.

* Revert "Revert back to requiring php 5.6 as the minimum php version (#2665)" This reverts commit f43c708. * Undo namespace changes.

japerry and others added 5 commits March 21, 2018 08:46

Change minimum version back to php 5.6

2c8d8c0

Revert "Change minimum version back to php 5.6"

01307a0

This reverts commit 2c8d8c0.

Revert "Require PHP 7.1 in all the right places. (acquia#2611)"

4791b66

This reverts commit ce3d78d.

Change Minimum version back to 5.6 from php 7.

0e4ceff

This reverts commit ceb6e8e. # Conflicts: # src/Robo/Commands/Sync/SyncCommand.php # src/Update/Updates.php

Revert "Require PHP 7 minimum. (acquia#2601)"

5300d43

This reverts commit 07504a4.

Update DrupalVM default config.

cf4c984

japerry added 3 commits March 21, 2018 13:37

Update core travis.yml to use php 5.6

76d4d85

Update pipelines to use php 5.6

7620008

travis should also deploy 5.6 not 7.1

06435a5

dpagini reviewed Mar 21, 2018

View reviewed changes

Jakob Perry added 5 commits March 21, 2018 22:43

Try wrapping config around a different var name.

42ffa86

Revert roboconfig trial and remove trail from webserverhook.

146c737

Remove associated use statement.

290e2e1

Remove all traits from webserver hook as they’re inherited from BltTa…

aa4ece4

…sks.

Add php7.1 as an additional test environment.

b689763

grasmash reviewed Mar 23, 2018

View reviewed changes

japerry and others added 4 commits March 24, 2018 22:39

Merge branch '9.x' into 9.x

3de1144

Update the travis php version, and provide doc comment on how to use …

eb64dbb

…php versions.

Merge branch '9.x' into 9.x

be8c60d

Update .travis.yml

522ed4e

grasmash merged commit f43c708 into acquia:9.x Mar 28, 2018

grasmash added a commit to grasmash/bolt that referenced this pull request Oct 2, 2018

Revert "Revert back to requiring php 5.6 as the minimum php version (a…

96f9756

…cquia#2665)" This reverts commit f43c708.

danepowell pushed a commit that referenced this pull request Oct 5, 2018

Remove PHP 5.6 support (#3140)

f2b9551

* Revert "Revert back to requiring php 5.6 as the minimum php version (#2665)" This reverts commit f43c708. * Undo namespace changes.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

japerry commented Mar 21, 2018

grasmash commented Mar 21, 2018

grasmash commented Mar 21, 2018 •

edited

Loading

japerry commented Mar 21, 2018

dpagini Mar 21, 2018

greylabel Mar 21, 2018

japerry Mar 25, 2018

dpagini Mar 21, 2018

grasmash commented Mar 22, 2018 •

edited

Loading

grasmash commented Mar 22, 2018

grasmash commented Mar 22, 2018

grasmash Mar 23, 2018

grasmash Mar 23, 2018

japerry Mar 25, 2018

grasmash Mar 23, 2018

geerlingguy Mar 23, 2018

grasmash Mar 23, 2018

dpagini Mar 23, 2018

grasmash Mar 23, 2018

danepowell Mar 23, 2018 •

edited

Loading

dpagini Mar 23, 2018

dpagini Mar 23, 2018

japerry Mar 25, 2018 •

edited

Loading

grasmash commented Mar 23, 2018

grasmash Mar 23, 2018

japerry Mar 25, 2018

grasmash Mar 27, 2018

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

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

Conversation

japerry commented Mar 21, 2018

grasmash commented Mar 21, 2018

grasmash commented Mar 21, 2018 • edited Loading

japerry commented Mar 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grasmash commented Mar 22, 2018 • edited Loading

grasmash commented Mar 22, 2018

grasmash commented Mar 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danepowell Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

japerry Mar 25, 2018 • edited Loading

Choose a reason for hiding this comment

grasmash commented Mar 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grasmash commented Mar 21, 2018 •

edited

Loading

grasmash commented Mar 22, 2018 •

edited

Loading

danepowell Mar 23, 2018 •

edited

Loading

japerry Mar 25, 2018 •

edited

Loading