Problem/Motivation

PHP 7 complete's test runs 1/3rd faster than PHP 5.6. This saves overall resources for the testing infrastructure, as well as giving quicker feedback to patch authors.

https://dispatcher.drupalci.org/job/php7_mysql5.5/133/ 21 minutes.

https://dispatcher.drupalci.org/job/php5.6_mysql5.5/173/ 30 minutes.

Also in general I think we should emphasise support for the most recent stable PHP version

Proposed resolution

Default patch testing to PHP 7, switch PHP 5.5 to per-commit testing.

Do this as soon as PHP 7.0 is out or when the two testing hacks in #2454439: [META] Support PHP 7 have been reverted.

Remaining tasks

Decide that we're going to do it, then postpone on #2454439: [META] Support PHP 7.

User interface changes

API changes

Data model changes

Comments

catch created an issue. See original summary.

alexpott’s picture

This seems like a really good idea.

Wim Leers’s picture

This sounds like a great idea. It'll ensure we don't regress against PHP7.

OTOH, it means that developers may encounter PHP 7-specific errors/fails. Which means they'll need to have both PHP 5.5/5.6 installed *and* PHP 7. But for core development, that is likely going to be a must-have anyway.

Crell’s picture

The potential risk here is making it easier for developers (core or contrib) to accidentally introduce PHP 7-specific syntax. In practice I think that's unlikely to be inadvertent, but it's a possible concern. (Moreso for contrib than core.)

On the flipside, since we do want to ensure code works with both 5.5 and 7, at least at the moment nearly all devs have a 5.5 local install, not a 7, I imagine. So testbot on 7 while people's local is 5.5 or 5.6 means we essentially get double-coverage, sort of. :-)

Given the big performance difference, though (and generally agreeing with catch on "emphasize support for the most recent stable PHP version"), I'm +1.

Fabianx’s picture

+1 to that, it also means we will be getting speed improvements more directly.

effulgentsia’s picture

at least at the moment nearly all devs have a 5.5 local install, not a 7

I don't think that'll be the case for long. For example, I suspect MAMP will offer PHP 7 pretty soon. You can already get it via bitnami. However...

The potential risk here is making it easier for developers (core or contrib) to accidentally introduce PHP 7-specific syntax

I'm not too concerned about that. We already get daily runs against all environments, and if we want, we can get on-commit testing for a PHP 5 environment, so we can detect and revert such accidental commits quickly.

So another +1 to doing this once #2454439: [META] Support PHP 7 is done.

catch’s picture

Yes on-commit testing of PHP 5* feels like the maximum change we might want to do here. Daily test runs should catch things well enough unless it's random fails.

It may even be the case that PHP7 patch test runs + PHP 5* on commit testing works out the same as PHP5* patch test runs given the performance improvement - although we'd need to check with infra folks on that.

dawehner’s picture

+1 for it in general. Sites will switch to php 7 at some point as well, and well, we also recommend to use it at some point.

On the flipside, since we do want to ensure code works with both 5.5 and 7, at least at the moment nearly all devs have a 5.5 local install, not a 7, I imagine. So testbot on 7 while people's local is 5.5 or 5.6 means we essentially get double-coverage, sort of. :-)

If we care about that, we could warn users if they run tests on PHP < 7

catch’s picture

Priority: Normal » Major
Status: Active » Needs review

Moving to needs review, and major given the 20 vs. 30 minute patch test times.

Fabianx’s picture

Title: Default to PHP 7 for patch testing » [policy, no patch] Default to PHP 7 for patch testing

Setting policy header to get visibility.

Mixologic’s picture

We probably wanna have a looksee at PHP7 + the alternative databases.

Pgsql + PHP7 doesnt pass right now:
https://dispatcher.drupalci.org/job/staging_simpletest_job/283/ (last night)
https://dispatcher.drupalci.org/job/php7_postgres9.1/1/ (today)

sqlite + PHP7 seems to work:
https://dispatcher.drupalci.org/job/staging_simpletest_job/287/ (last night)
https://dispatcher.drupalci.org/job/php7_sqlite3.8/2/ (today)

So, currently, 'we support php7, and we support Pgsql, just not at the same time'.

We've got this almost ready to deploy new environment combinations so that this is testable (https://bitbucket.org/drupalorg-infrastructure/drupal.org/pull-requests/...), but wanted to flag this early enough that people were aware of it.

catch’s picture

Opened #2608558: PostgreSQL + PHP 7 test failures.

I don't think that affects defaulting to PHP 7 + MySQL, but yes definitely want to look at those.

Fabianx’s picture

I agree with #12, PHP 7 + MySQL is fine to default to.

I opened https://bugs.php.net/bug.php?id=70861 with reproduce instructions on my EC2 box as usual.

As we support Postgres natively might need to add a note to release notes that PG + PHP 7 is not yet supported ... (unless they magically fix it in time for PHP 7.0.0, which is not impossible.)

Edit: https://bugs.php.net/bug.php?id=70861 is fixed already, now need to look into other failures ...

xjm’s picture

Hmm, I'm not sure I agree with this proposal. Patch-level testing should support the version of PHP that is most likely to be used on real sites and the most likely to break due to new syntax, and that is 5.5. I think if we switch to PHP7 we will end up with a lot of reverts. Even with 5.4 and 5.5 I had many occasions where I'd accidentally used PHP 5.4-invalid syntax and I only found out when I uploaded a patch. Not knowing until commit would be frustrating, and I see PHP7 as the default much more likely to introduce branch-specific problems.

A mitigation might be to do merely PHP syntax checking on all patches for all versions once that's possible on CI. But I think PHP 7 is frankly way too young and raw to treat as the default version for Drupal yet. The performance gain is compelling for large sites, but the vast majority will still be on 5.5 or 5.6 for awhile yet.

catch’s picture

I think if we switch to PHP7 we will end up with a lot of reverts.

Reverts are fine. Especially if we put PHP 5.5 on per-commit testing, then we'll be informed of breakage within 30 minutes of a commit.

Fabianx’s picture

There are way less syntax changes in PHP 5.5, 5.6 & 7 then from 5.3 to 5.4, so accidental bugs will be rare and language constructs like generators or the new comparison operator should easily be found in patches.

catch’s picture

7.0.0 is out. Agreed with #16 that we should not see a high number of incompatibilities compared to 5.2/5.3/5.4 - also we could revisit this if we did, will be easy to spot since they'll mean HEAD breaks on commit test runs, if it doesn't we don't have a problem.

Crell’s picture

Question: Would we default to PHP 7 for Drupal 7 as well? I haven't been tracking its status as closely but if that is also now PHP 7-friendly then it may make sense to apply the same testing to both, for the same reason. (And if it's not, well, let's fix that.)

chx’s picture

Most of the syntax changes in PHP 7 are in places where no decent developer should ever go to. The only one we used is $foo->$bar['baz'] but for that one we always used $foo->{$bar}['baz'] anyways because that's what we meant and with the {} it's not changing. Things like Foo::$bar['baz']() changing meaning, well, if you used code like that , you deserve it breaking!

While I think is_numeric($hex) changing to FALSE will catch people unaware, most of the time that's what we want anyways, when we do an is_numeric check what we really want is a "is this an integer casted to a string" so that's not a biggie.

The spaceship is easy to catch on a review.

So I have doubts about lots of reverts.

catch’s picture

@Crell Drupal 7 currently has 4 fails on PHP 5.4, tests do not run successfully on 5.5, 5.6, and PHP 7 isn't available as an environment for it. #2620104: PHP 5.4 test failures

So that's impossible at the moment.

Even if it was, would need to be a separate issue and agreed with David once it was possible.

Fabianx’s picture

So do we have consensus and can go into RTBC stage for the policy or this there more discussion need?

chx’s picture

Regarding #14 again -- yes, the 5.3/5.4 switch is way more rough because of the short array syntax. It's easy to write, much harder to catch because it's ubiquitous and "small" so to speak. The spaceship will be rare and much more jarring.

effulgentsia’s picture

Title: [policy, no patch] Default to PHP 7 for patch testing » [policy, no patch] Default to PHP 7 for Drupal 8.x patch testing

Adding "Drupal 8.x" to the issue title to clarify the scope of this issue per #20.

The proposed resolution still says "switch PHP 5.5 to daily branch testing only.". Do we want to switch this to "per commit testing" per #15?

do we have consensus

@xjm was -1 in #14. Would be good to get an updated opinion from her or someone else who works a lot with new and/or infrequent contributors, since I think they're the most likely to get discouraged by reverts that occur due to lack of PHP 5 testing occurring at the time they're actively working on their patch.

catch’s picture

Issue summary: View changes

For me I think the trade-off of potentially more reverts vs. 1/3 quicker test runs should be a large net-positive for patch authors.

@alexpott noted we should postpone this on APCu being available on DrupalCI, that's a bigger disrepancy than the PHP versions so makes sense, can't find the ticket for it at the moment.

I updated the issue summary to say per-commit testing - we did this for all 8.0.x DrupalCI environments shortly before the 8.0.0 release already.

Mile23’s picture

If we promised people that D8 will run on PHP 5.5.9, then there should be a PHP 5.5.9 CI run in the process.

It could be the branch run on commit, for instance, if CI can do that.

Otherwise, if CI can't do that, then no, PHP 7 is not our target version.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

catch’s picture

#2656452: add ACPu is available for PHP7 is the DrupalCI ticket for adding apcu.

I think we could still consider switching the patch tests to PHP 7, keep one of 5.5 and 5.6 for commits, and review if we have to roll back due to the change - would get faster test runs in the meantime.

catch’s picture

Fabianx’s picture

David_Rothstein’s picture

Title: [policy, no patch] Default to PHP 7 for Drupal 8.x patch testing » [policy, no patch] Default to PHP 7 for Drupal core patch testing

There are no longer any specific blockers to doing this on Drupal 7 (Drupal 7's tests are now passing on PHP 7, and already running per-commit thanks to #2738921: Drupal 7 tests should not be blocked for PHP 7 on the Automated Testing tab of project pages)... Also this is a policy issue rather than an implementation issue, so it can be about both versions at once.

It seems to me like the current practice of running patch tests on the oldest possible PHP version is more of a historical accident than an actual policy? Switching it to the newest version (i.e. PHP 7) instead sounds worthwhile due to the improved speed benefits, general promotion/awareness of newer PHP versions, etc.

As long as we still do per-commit testing on the oldest possible PHP version (to avoid regressions), I'm in favor.

Fabianx’s picture

I am also in favor to enable this for D7 as well as the default.

alexpott’s picture

What'd be nice is to lint using the lowest support PHP version and test using the fastest. That way we can make just the PHP is valid for the lowest version... or actually linting should be difficult from test running. We should lint all the changed files against all the supported minor versions.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

cilefen’s picture

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joseph.olstad’s picture

I agree with @Fabianx , make PHP 7 the default for D7 as well, for obvious reasons. it's a slam dunk win. History was meant to be rewritten.

Alexpott mentioned the lint test, also great idea. Let's do it php 7 and lint the rest.

catch’s picture

Version: 8.3.x-dev » 8.4.x-dev
Status: Needs review » Reviewed & tested by the community
Issue tags: -blocked by drupalci

APCu has been available on the containers since January, see #2656452: add ACPu is available for PHP7.

So I think this can be RTBC? Would be nice to make the switch during the 8.4.x alpha.

xjm’s picture

Status: Reviewed & tested by the community » Fixed

Yeah, I think we can and should do this now that PHP 7 is the only actively supported version. I went ahead and changed the patch testing default for all three Drupal core branches.

joseph.olstad’s picture

Version: 8.4.x-dev » 7.x-dev
Status: Fixed » Needs review

can we hijack this issue for D7 , or do you prefer a new issue for D7?

andypost’s picture

Version: 7.x-dev » 8.4.x-dev
Status: Needs review » Fixed

D7 needs separate issue

joseph.olstad’s picture

Wim Leers’s picture

OMG YAY! 🎉

catch’s picture

Adding some credit for direct work on getting this decided/fixed, if I missed someone let me know.