Problem/Motivation

See last few posts at #1498574: [policy, no patch] Require PHP 5.4.

This would ensure core support for PHP 5.4, but has two side-effects:

  • PHP 5.4-only code might not get caught when committed. I think this is better than not catching bugs on PHP 5.4 given it's either/or for testing on PHP version.
  • Existing core and contrib module tests that fail on PHP 5.4 will start failing on the bots, due to this it'd need to be scheduled with an announcement in advance I think.

In terms of 8.x, I'd be happy to make this change soonish, if there's a couple of days of disruption due to 5.4 bugs now, that's better than shipping with them.

Examples

Example issue that could have been caught:

Proposed resolution

Testbots need to run on php 5.3.10, 5.4, 5.5, and 5.6

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#27 D7_PHP54.jpg163.2 KBjthorson
#26 D8_PHP54.jpg215.08 KBjthorson
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jthorson’s picture

Related discussion: #1683942: File syntax check will choke for modules which require PHP 5.4
A couple of comments in #1565094: Remove "vendor" directory from PHP syntax check to avoid PHP 5.4 errors are also relevant.

To be honest, this could require a fair bit of work at the moment, as moving to PHP 5.4 on the testbots may break our ability to syntax check D6 and D7 code.

I did play around with two simultaneous PHP versions on a single server as a proof of concept (as reflected in the first issue linked above) ... but can't remember whether we had a valid reason to not chase that further, or if it just died due to other higher priorities.

RobLoach’s picture

Yes please.

Crell’s picture

I'm trying to add another 3rd party library to core (PSR\Log) that offers traits. Even if we don't use them, we shouldn't have to play games with vendor gitignore files. Let's just use the 5.4 linter (by upgrading testbot to it) and be done with it.

(It should make testbot a bit faster, too.)

msonnabaum’s picture

If the goal is just to ensure 5.4 compatibility, why not build a separate php 5.4 binary to do just that check with? If that's too complex, we could even trigger a travisci job to do it for us.

Not testing on the version of php we require seems totally insane to me.

jthorson’s picture

As per http://drupalcode.org/project/project_issue_file_review.git/blob/refs/he... ... the testbots do not syntax check anything in the '/core/vendor' directory.

catch’s picture

There's no support for testing on different environments with the current test infrastructure.

I don't think saying we require PHP 5.3 is exactly right. We require /at least/ PHP 5.3.x. Contrib modules (and arguably specific core modules if there was a real need) can optionally require higher versions.

Drupal 6, 7, and 8 all support PHP 5.4.

So across core and contrib, it's OK to write code that only works on PHP 5.4 (as long as you specify a dependency), but it's not OK to write code that doesn't work on PHP 5.4.

Not testing on PHP 5.3 might mean we accidentally break support for that on occasion, has has happened with Drupal 6 and PHP 4, but that's the least-worst case IMO (and someone could still set up a 5.3 branch test somewhere and post critical issues if that breaks).

xjm’s picture

I think it's a terribly bad idea to have the bots on anything greater than our minimum required version, which is currently set to 5.3.5. There are numerous things that 5.3.5 does not support that folks consistently try to use, and if we upgrade the bots to 5.4, we lose that minimum compatibility checking.

That's not to say testing HEAD periodically on 5.4 would be unwelcome--that's also a good idea, on its own, separately, and is something I think has been in the "big picture" plan for awhile--but our main testbots should match our minimum requirement. When we upgrade the minimum supported version of PHP for D8, we can upgrade the bots to that version. This isn't the issue to discuss what the minimum version for D8 should be. :)

tstoeckler’s picture

I disagree completely. Given two versions X and Y and the two conditions
A. We can only choose one, i.e. either test on X or on Y
B. You can write both code that is only broken on X and code that is only broken on Y
I think we should always be as forward-leaning as possible and choose max(X,Y). That would be 5.4 in this case.
By Murphy's law we will break either 5.3 or 5.4 (to then go and fix it, obviously) and I'd much rather break 5.3 at this point.

jthorson’s picture

The testing infrastructure serves much more than D8 core development ... it must also consider all of contrib. And right now, contrib is primarily D6 and D7.

Upgrading all testbots to PHP 5.4 could potentially result in these projects throwing all sorts of warnings and errors which they did not do previously ... then again, it might not. The point being, we need to at least enumerate the expected impact on projects other than D8 core before we can consider making the change. And if that research comes back with the answer 'break contrib testing to help core', I'm going to need some heavy convincing.

I also think that max('real world') should take precedence over max(X,Y), and this comment seems to suggest that the real world is lagging.

Crell’s picture

jthorson: If contrib modules have bugs on PHP 5.4, we SHOULD be throwing errors all over the place on that. There was discussion on the PHP internals team yesterday and today about the uptake problem, and Drupal came up as one of the projects that "still doesn't work on modern PHP". Contrib modules that are buggy on PHP 5.4 *are buggy*, and our CI system should be telling maintainers that.

We need to be spewing errors on people if their code doesn't work in PHP 5.3 OR PHP 5.4. And we need to be testing HEAD against PHP 5.5 HEAD so that we can let the internals team know if they broke anything on us while they still can fix it. Sadly, I think that comes down to "moar hardware!".

jthorson’s picture

It comes down to 'moar hardware', as well as 'moar peoplez'. But at the end of the day, even that's not enough.

In the current infrastructure, deploying additional environments would result in a doubling of the processing time required for every patch, and a failure on any one environment results in a failure of the entire patch being sent back to drupal.org. Last weekend's qa.d.o deployment contains the first step in decoupling environments from each other, so we can get rid of this architectural limitation ... but it's got a long ways to go.

On top this, during the push to Drupal 7 launch, it was stressed that stability of the testing system needed to take precedence over introduction of new features. To the best of my knowledge, this policy has not changed ... which suggests that if we DO implement this level of testing, it's probably best to develop the capability in parallel rather than on top of the current qa.d.o infrastructure.

So, as an alternative, we have a new architecture which can solve the multiple environment problem without injecting more instability into the testing infrastructure, and it's about 70% complete. But unless we get a bit more support from key individuals, I can't see that making it's way to operation anytime soon. (Of course, that also got blocked on the D7 port of drupal.org, but that's a whole other story ...)

I recognize the desire, and we do have multiple needs ... but until one of these above initiatives is complete, or more folks start chipping in, or I see evidence of a strong community consensus that we should NOT test on the minimum requirement by default (which I am not seeing), then we're simply going to have to accept that we can't meet them all with today's qa.d.o infrastructure. Given the environment we've got, and the situation we're in, my opinion is that we go with the option that caters to the majority of users.

This isn't to say we don't *need* to test other versions ... it's just to say that we currently have to pick *one*, and at a bare minimum, we need to validate the 'bare minimum' requirements (which also happens to reflect the version being used by the majority of existing Drupal sites). ... And given a choice between our users and their devs, I have to think that validating PHP's internal development activities is simply a 'nice-to-have'. */me dives for cover and dons his flame-retardant suit* ;)

Crell’s picture

I reluctantly agree with you up until the last point. "Make sure Drupal isn't going to get screwed by PHP 5.5", "Make sure we don't break on PHP 5.4 (current stable version)", and "make sure Drupal contrib projects aren't holding back deployment of PHP 5.4 which is notably faster than PHP 5.3" are all somewhat more important than "nice to have". I'd file that as "major" at least, if not "critical". But it's definitely not a "minor".

jthorson’s picture

Fair enough. I should have added "relatively speaking" on the last point. Though it was stated partially in jest, that doesn't really come across in text. It certainly wasn't my intent to trivialize the other requirements you brought up ... I only meant to reflect that they aren't the one located at the top of the hierarchy.

renat’s picture

Actually there is one more point, not mentioned by @Crell - there definitely will be D8 contrib modules requiring you to use at least PHP 5.4, while other modules would not play well with that version without 5.4 automated testing. So you'll not be able to choose PHP version that 'just works', and it can significantly slow down D8 adoption.

msonnabaum’s picture

There were very few d7 contrib modules that required 5.3, it seems pretty unlikely that'll happen in d8 to the point where it'll cause anyone problems. If you do that in contrib, there are other testing options besides our testbot.

Crell’s picture

Title: Upgrade testbots to PHP 5.4? » Testbots need to run on 5.3.10, 5.4, and 5.5
Priority: Normal » Major
Issue tags: +D8 stable release blocker

It looks like there's a number of errors with Drupal core on 5.4 right now. Drupal not running correctly on PHP 5.4 is a critical release-blocking problem.

That said, we still need to be testing 5.3 if we claim to support that, too.

I believe it's therefore a Drupal 8 release blocker to get testbot running *all* supported PHP versions. That is 5.3.10 (our current minimum), 5.4.latest, and 5.5.latest. I'm bumping this to major, but I really think it should be critical. However we do it, we must be able to verify that Drupal works on a PHP release that isn't already on life support before we can ship.

I know jthorson noted above in #11 that there are non-trivial challenges to doing so in our current infrastructure. That may be true, but those challenges are now blocking a D8 release. Not addressing them is no longer an option. IMO that falls squarely on the DA and its new governance committees to figure out how to resolve. :-)

alexpott’s picture

It might be worth considering a minimal implementation first up... lets just get two testbots up with php 5.4 and 5.5 respectively. And just have them test 8.x HEAD in a loop. There at least we would have a way to find out what issues we have... and then we can work on more complex issues like how to determine the issue that broke it.

klonos’s picture

Title: Testbots need to run on 5.3.10, 5.4, and 5.5 » Testbots need to run on php 5.3.10, 5.4, and 5.5

...

Crell’s picture

Something else worth exploring, perhaps, that just hit beta: http://www.phptesting.org/

jthorson’s picture

#19: PHPCI currently doesn't support multiple bots ... but it does support non-github repository urls, so definitely worth keeping an eye on.

catch’s picture

Just branch testing like Alex suggests in #17 would be better than what we have now.

catch’s picture

Also @Crell where are the issues for 5.5 breakage?

amateescu’s picture

@catch, he said 5.4, not 5.5 :) Here's one that I had to deal with #2025451: Drupal\entity_reference\Tests\EntityReferenceAdminTest fails on PHP 5.4.9 and libxml 2.9.0.

alexpott’s picture

@amateescu the test that is being fixed in #2025451: Drupal\entity_reference\Tests\EntityReferenceAdminTest fails on PHP 5.4.9 and libxml 2.9.0 breaks just as well in 5.5 :)

amateescu’s picture

@alexpott, no doubt about that, just trying to be accurate.

And, unfortunately, we're not just fixing "a test" there, the Entity reference UI is actually broken with 5.4, and that test catches it..

jthorson’s picture

FileSize
215.08 KB

Our staging environment is updating from prod nightly, so I can't save these results there ... in any case, here's the current D8 HEAD run on PHP 5.4. (57,180 pass(es), 3 fail(s), and 5 exception(s))

jthorson’s picture

FileSize
163.2 KB

And here's D7 on PHP 5.4

Berdir’s picture

Running on PHP 5.5. is going to give a lot of these:

curl_setopt_array(): The usage of the @filename API for file uploading is deprecated. Please use the CURLFile class instead
curl_setopt_array(Resource, Array)
Drupal\simpletest\WebTestBase->curlExec(Array)
Drupal\simpletest\WebTestBase->drupalPost('admin/config/services/aggregator/add/opml', Array, 'Import')
Drupal\aggregator\Tests\ImportOpmlTest->submitImportForm()
Drupal\aggregator\Tests\ImportOpmlTest->testOpmlImport()
Drupal\simpletest\TestBase->run()
_simpletest_batch_operation(Array, '188', Array)
call_user_func_array('_simpletest_batch_operation', Array)
_batch_process()
_batch_do()
_batch_page()
system_batch_page()
call_user_func_array('system_batch_page', Array)
Drupal\Core\EventSubscriber\LegacyControllerSubscriber->Drupal\Core\EventSubscriber\{closure}()
call_user_func_array(Object, Array)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\HttpKernel->handle(Object, 1, 1)
Drupal\Core\DrupalKernel->handle(Object)
drupal_handle_request()

And it's going to blow up when trying to log this because this is a E_DEPRECATED and that's not defined in Simpletest's error map, so you need this:

diff --git a/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
index 0971f00..640f2cb 100644
--- a/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -1115,6 +1115,8 @@ public function errorHandler($severity, $message, $file = NULL, $line = NULL) {
         E_USER_WARNING => 'User warning',
         E_USER_NOTICE => 'User notice',
         E_RECOVERABLE_ERROR => 'Recoverable error',
+        E_DEPRECATED => 'Deprecated',
+        E_USER_DEPRECATED => 'User deprecated',
       );
 
       $backtrace = debug_backtrace();

Will open an Issue for this. Probably doesn't make much sense to run it before this is fixed.

Berdir’s picture

Opened #2054205: Broken Tests on PHP 5.5 for that. Other than that, it's looking very good so far on 5.5 (I'm in the Node group now), except the tests that also fail on 5.4, of course.

webchick’s picture

It seems to me that what we need is the /ability/ to run our test suite on PHP 5.4+ prior to rolling a stable release (and on an ongoing basis), not necessarily to have this capability in testbot itself prior to a D8 stable release. So we could run an external equivalent of DamZ's fancy thing that would tell us of postgres/sqlite/etc. fails in D7. It would definitely be useful to have this capability in testbot itself, though, so we could find out much sooner when stuff breaks.

Crell’s picture

webchick: Fair, the release-blocker is "we know and can verify that 8.0.0 runs on 5.4/5.5 without errors". Making testbot do that for us automatically is just the most obvious way to get there and make sure it stays that way, and long-term is a requirement.

Berdir’s picture

jthorson’s picture

Just for awareness, after DC Portland, Randy Fay did a fair bit of puppet work to simplify the building of a testbot; so turning up additional bots is no longer a challenge ... but when it comes to our available OSUOSL testbot resources, I've been trying to walk a tight balancing act between having enough resources to keep the patch queue down, having bots available for testing the drupal.org D7 port of PIFT, and handling special requests (freezing out bots for troubleshooting, PHP 5.4 testing, etc.).

Our current puppet build is based on Debian squeeze, which does not have available packages for PHP 5.5 (even on dotdeb.org); so in order to get a 5.5 test, we need to build our own PHP packages or update the entire puppetmaster build. Upgrading the puppetmaster build to Wheezy updates PHP to 5.4, at which point we are no longer testing against our minimum supported version ... which drives the need to find a new 5.3 solution.

We also need to consider that run-tests.sh is run via php-cli, but also leverages apache for some of the work. Running multiple versions of PHP under the same apache server is doable with fast-cgi, but this may potentially have a performance impact on testbot apache throughput.

Simply turning up more environments is feasible (almost trivial), but comes with a tradeoff that each D8 HEAD test would likely take twice as long to return results ... and would take away from existing testbot resources that are barely managing to keep the existing patch queue down to a reasonable level.

Each of the above requires a bit of development on various aspects of the legacy infrastructure ... so while making the testbots do it automatically *is* the most obvious response as Crell points out, the obvious solution is not always the simplest.

The last item for consideration is the balancing act between patching for today versus developing for tomorrow ... most core devs I know would say they prefer to work on D8 than D7. As a parallel, every new testbot change request we action holds us back in D6 land for that much longer, time that could otherwise be spent figuring out the next evolution of our testing architecture. Yes, it may be slowing down D8 velocity ... but we need to start shifting more attention towards figuring out the evolution plan, instead of simply applying additional layers of duct-tape to the current system.

If we don't, then on D8 launch day, our entire continuous integration workflow will be based on servers running on an unsupported, 'End-of-Life' D6 software release. :(

<sarcasm>/me considers opening a 'Get testbots off D6' issue, tagging it as a D8 launch blocker ... and then taking a community poll on where he should spend his time.</sarcasm> ;)

Berdir’s picture

Found an existing issue for the locale thing, crazy stuff: #2043953: Singular/plural strings don't work in JavaScript anymore

rfay’s picture

As an interesting start, I created Debian Squeeze packages for php 5.3/5.4/5.5 that we could work with.

I won't have any time to experiment with them (and they're completely untested) but here they are. They install into /srv/php5.x

php-5.3.27.deb
php-5.4.17.deb
php-5.5.1-deb

The build failures (seem minor) are at https://gist.github.com/rfay/6143588

jthorson’s picture

Created #2057263: Test deploy of multiple custom PHP packages on a testbot to track progress of testing packages in #35, so as to not hijack this thread.

ParisLiakos’s picture

a phpunit one for php 5.5 #2068593: JsonTest fails with pecl-json

Crell’s picture

Issue summary: View changes
Issue tags: +PHP 5.4, +PHP 5.5
catch’s picture

Title: Testbots need to run on php 5.3.10, 5.4, and 5.5 » Testbots need to run on php 5.3.10, 5.4, 5.5, and 5.6

PHP 5.6 is in beta, fourth and final beta release is supposed to come out tomorrow, so we'll likely see a stable before 8.0.0.

YesCT’s picture

catch’s picture

alexpott’s picture

What #2364647-26: [sechole] Remove blacklist mode from Filter:XSS points to is that we need to run on all supported minor versions too - 5.5.8 and 5.5.18 were getting different PHPUnit test results in a class that is vital for security.

jthorson’s picture

I'll add a note for this on the DrupalCI project ... we're transitioning to using phpenv, so we should be able to build a container image that contains all supported minor versions and be able to iterate through testing each of them.

Of course, repeating this for every minor release of php would be extremely resource-intensive, and I don't think this is something we would do on each and every patch/commit ... so the question becomes how frequently does this cycle of tests need to be repeated? Once a week? Month? Every 3 months (i.e. after an initial check is complete)?

greggles’s picture

From my perspective, it would be nice if an individual test could be marked so that it will get tested on all platforms.

webchick’s picture

Agreed. That's probably all that's needed for things like that. The vast majority of patches are a wash PHP version-wise, and the "big" version targets (5.3*, 5.4*, 5.5*, etc.) should catch almost all other regressions.

alexpott’s picture

Re #44 and #45 - how would you find this out?

catch’s picture

Yeah I don't see how we could possibly have predicted that need for #2364647-26: [sechole] Remove blacklist mode from Filter:XSS - it was pure luck it was found now and not in two years. At least we should run phpunit on every possible version without an option, and look at bailing out early if that fails before running simpletests.

chx’s picture

fabianx raised the possibility of using travis-ci to run phpunit on every php. Better than nothing. Not as nice... and definitely a different issue.

YesCT’s picture

Issue tags: +Security

so, would running head once a day be worth it (responding soon enough, but not sooo resource intensive)?

Mile23’s picture

Testbot for patch review/daily RTBC: Lowest required PHP version for all tests.

Testbot branch test for commits: All major versions of PHP for unit tests, lowest required PHP for integration (SimpleTest, whatever else).

If it's discovered that one minor version of PHP presents a special challenge, that minor version becomes a requirement, and we test it as well as the newest minor version in that major version.

In an ideal world, there would be a local vagrant/docker type package with a script to do the version switching and test running for you so you could verify before you submit your patch.

greggles’s picture

@alexpott the scenario I'm talking about is that a test is manually created on qa.drupal.org for security issues, so it can be a checkbox on the form there. Beyond that it seems fine to me if we just run the tip of the current branches once a day.

YesCT’s picture

adding "infrastructure blocker for Drupal 8.0.0" tag, since this blocks the drupal 8 core issue: #2267715: [meta] Drupal.org (websites/infra) blockers to a Drupal 8 RC1, so that the infrastructure blockers to d8 release (in issue queues outside of the core queue) is accurate. Remove the blocker tag when this issue is fixed, and update 2267715.

catch’s picture

Priority: Major » Critical

I've just marked #2135203: Fix all known PHP 5.4 incompatibilities on D6 and D7 that are critical or would prevent a migration to D8 fixed - this was Drupal 6/7 critical 5.4/5 compatibility issues that would prevent a site migrating from 6-8.

YesCT’s picture

YesCT’s picture

nlisgo’s picture

The following issue is an example of something that got through the testbot but caused some disruption with a fatal error and had to be quickly reverted: https://www.drupal.org/node/2407361#comment-9686033

YesCT’s picture

I will add #2407361-35: Move usages of drupal_html_id() to Html::getUniqueId() to the summary.
@nlisgo we have a format that makes links to issues, [#NNNNN-CC] where NNNN is the issue number and CC is the comment number

YesCT’s picture

YesCT’s picture

Issue tags: +PHP 7.0 (duplicate)

.

YesCT’s picture

Title: Testbots need to run on php 5.3.10, 5.4, 5.5, and 5.6 » Testbots need to run on php 5.3.10, 5.4, 5.5, 5.6 and 7
Related issues: +#2454439: [META] Support PHP 7
isntall’s picture

Project: Drupal.org Testbots » DrupalCI: Dispatcher (Modernizing Testbot Initiative)
Mixologic’s picture

Issue tags: +blocked by drupalci
Wim Leers’s picture

dawehner’s picture

I'm curious whether there is a meta issue|plan which tracks the progress of drupal_ci, so you could see how far away from using it, we are?

andypost’s picture

heddn’s picture

jthorson’s picture

That hitlist is specific to my drupalci_testbot work, to get that to an alpha release.

In the meantime, we've been live for a while for environment testing on Drupal core with an interim solution; see the bottom of the page at https://www.drupal.org/node/3060/qa.

salvis’s picture

we've been live for a while for environment testing on Drupal core with an interim solution

(When) will that be available for contribs? I've enabled it for ACL, but it's stuck "Queueing".

drumm’s picture

Contrib will take a few days or weeks, depending on how much we find that needs stabilizing with core tests.

David_Rothstein’s picture

I had a similar question, but about Drupal 7 core: #2467925-49: [plan] Remove blockers to the "minimum viable" state of DrupalCI to ship Drupal 8 RC1

Really cool stuff!

hass’s picture

May I get the testbots running for Google Analytics prior to enabling it for all projects, please? Contrib preproduction testing...?

jibran’s picture

I created #2530640: PHP 7 testing issue to test Drupal 8 on PHP 7.

Mixologic’s picture

webchick’s picture

Title: Testbots need to run on php 5.3.10, 5.4, 5.5, 5.6 and 7 » Testbots need to run on 5.4, 5.5, 5.6 and 7
Status: Active » Fixed

We had a pow-wow today with most of the core committers (@xjm, @alexbronstein, @alexpott, @catch) as well as most of the DA tech team (@drumm, @basic, @isntall, @joshuami, @Mixologic, @hestenet, @japerry... hope I didn't forget anyone!) to go through the list at #2267715: [meta] Drupal.org (websites/infra) blockers to a Drupal 8 RC1.

Apart from PHP 5.3 (and PHP 5.2?) for Drupal 6/7, everything on this list has been accomplished, and Drupal 8 now has all PHP environments that it needs. For D6/7, spun off #2551233: Ensure that DrupalCI supports D6 testing on the 5.3 environment for parity with PIFT/PIFR as its own critical issue, but one that only blocks decommissioning of PIFT, not Drupal 8.

With that adjustment, I believe we can now call this one fixed. YEAH!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.