[5.4] Add PHP 7.2 to travis #20258

fernandobandeira · 2017-07-26T00:10:14Z

This PR adds PHP 7.2 as an allowed failure to travis, we need to fix some things for 5.4/5.5 to work with 7.2.

Old PR for 5.5 branch: #20253

GrahamCampbell · 2017-07-26T00:52:03Z

👎 For the allow failures. The tests should instead be fixed.

taylorotwell · 2017-07-26T02:28:47Z

Can we get a list of tests that are broken?

morloderex · 2017-07-26T07:08:24Z

@taylorotwell Here is a gists with the textdox output with tests runned with php 7.2beta1, and master of this repo. (one test is not related to the count() warning).

As it have been runned locally i found 8 tests in here which is failing (travis only 30) I'll try to find out which ones.

https://gist.github.com/morloderex/be60ae1bf83d8dbeffaaed983fb9f902

lk77 · 2017-07-26T07:49:17Z

extracted from https://gist.github.com/morloderex/be60ae1bf83d8dbeffaaed983fb9f902

Illuminate\Tests\Cache\CacheRepository
[ ] Get minutes (Failing not related to Count() fixed in 5.4 branch)

Illuminate\Tests\Database\DatabaseEloquentBuilder

[ ] Query scopes
[ ] Real nested where with multiple scopes and one dead scope

Illuminate\Tests\Database\DatabaseEloquentGlobalScopes

[ ] Closure global scope is applied
[ ] Closure global scope can be removed
[ ] Global scope can be removed after the query is executed

Illuminate\Tests\Database\DatabaseEloquentHasMany

[ ] Find or new method returns new model with foreign key set
[ ] First or new method finds first model
[ ] First or new method with values finds first model
[ ] First or new method returns new model with foreign key set
[ ] First or new method with values creates new model with foreign key set
[ ] First or create method finds first model
[ ] First or create method with values finds first model
[ ] First or create method creates new model with foreign key set
[ ] First or create method with values creates new model with foreign key set
[ ] Update or create method finds first model and updates
[ ] Update or create method creates new model with foreign key set
[ ] Models are properly matched to parents

Illuminate\Tests\Database\DatabaseEloquentIntegration

[ ] For page after id correctly paginates
[ ] Relations are preloaded in global scope

Illuminate\Tests\Database\DatabaseEloquentModel
[ ] Scopes method

Illuminate\Tests\Database\DatabaseEloquentMorph

[ ] Find or new method returns new model with morph keys set
[ ] First or new method finds first model
[ ] First or new method with value finds first model
[ ] First or new method returns new model with morph keys set
[ ] First or new method with values returns new model with morph keys set
[ ] First or create method finds first model
[ ] First or create method with values finds first model
[ ] First or create method creates new morph model
[ ] First or create method with values creates new morph model
[ ] Update or create method finds first model and updates
[ ] Update or create method creates new morph model

Illuminate\Tests\Database\DatabaseEloquentSoftDeletesIntegration

[ ] Has one relationship can be soft deleted
[ ] Belongs to relationship can be soft deleted
[ ] Has many relationship can be soft deleted
[ ] Second level relationship can be soft deleted

Illuminate\Tests\Integration\Cache\CacheLock

[ ] memcached locks can be acquired and released
[ ] memcached locks can block for seconds
[ ] locks can run callbacks

Illuminate\Tests\Integration\Database\EloquentBelongsToManyTest\EloquentBelongsToMany
[ ] Detach method

fernandobandeira · 2017-07-26T11:28:09Z

The return from travis says that most of those are fixed using a ternary on these files:
Builder.php
SoftDeletingScope.php

However some are failing inside mockery (maybe upstream?):
Expectation.php $_expectedArgs isn't always an array.

There's also some count calls inside the tests that are throwing the exception.
Finally that other test failing on master was fixed yesterday by @themsaid so it should be alright, it's just the count calls.

I'll remove the allow failures from it, we shouldn't have any problems fixing this...
I can start fixing it on this PR or send a separate PR for it. (if you'd prefer to let someone from the team fix it there's no problem too, it's all up to you, I've created the PR just to get some attention on this 😄).

taylorotwell · 2017-07-26T12:32:52Z

Let's get it passing on this PR.

deleugpn · 2017-07-26T12:56:40Z

If anybody wants to help @fernandobandeira to solve this, you can simply open a PR in his fork and if he merges, this PR picks that up automatically.

reference: #19245

fernandobandeira · 2017-07-26T14:39:31Z

Should be fixed now, there's 14 remaining failing tests which are being caused by mockery, I've sent a PR on mockery/mockery#771 to address that, I'll ping when that one is merged/released.

laurencei · 2017-07-27T03:04:59Z

tests/Database/DatabaseEloquentBelongsToManyTest.php

@@ -116,7 +116,7 @@ public function testModelsAreProperlyMatchedToParents()
        $this->assertEquals(2, $models[1]->foo[0]->pivot->user_id);
        $this->assertEquals(2, $models[1]->foo[1]->pivot->user_id);
        $this->assertEquals(2, count($models[1]->foo));
-        $this->assertEquals(0, count($models[2]->foo));
+        $this->assertEquals(0, count((array) $models[2]->foo));


It seems strange that you need to use (array) on the results which are 0 to make them pass, but not on the results that are not 0.

If you dont know what the result is going to be before using count() - that could be a problem.

Kinds of feels like we are hiding a bigger issue? Or at the minimum we are not being consistent in the tests...

The issue I assume is that php doesn't allow passing null to count anymore?

Put another way; it would seem, according to how this test is written, that you cannot safely use count() on a model that is similar in principal to $models[2]->foo in PHP7.2, unless you always typehint it to an array first.

So just because the test passes doesnt mean we are solving the underlying issue?

unless you always typehint it to an array first.

It just needs to be "countable". PHP 7.2 has decided that null is not countable, where as previous versions say that it is, and that it's count is 0.

Right - exactly. So to me, these tests prove that we are only hiding the issue?

The real "solution" would be to allow the result to always be "countable"? Otherwise you could no longer use count() safely on a model result? That might exclude many apps from updating, because it would be reasonable to assume count() is being used fairly extensively out there currently...

edit: I think the real fix is leaving these tests unchanged, and changing the core "somehow" to allow them to pass as they were originally...

Actually only array and objects that implement countable can be used with count

Yeh, in PHP 7.2, those are the types considered countable:array and Countable. In 7.1 and earlier, null was also countable.

I'm 👍 for changing it on 5.5

Seems like a fair change perhaps. Ping @taylorotwell.

If we are going to change it in 5.5 - then the current test's should be left unchanged? That way we can prove the problem is solved?

Because technically we are now saying that 5.4 is not really suitable for PHP 7.2? You have situations where the current code will fail - so hiding those results in a changed test will trip people up?

Plus - this is a change that even people with their own test suites might miss during an upgrade. I doubt people are having full application test suites that always specifically tests for a null return (i.e. a 0 count) result as part of the test. So what will occur is people start upgrading, think they have "all green" - and when it hits production start getting errors.

Whilst it's not Laravel's fault - the reality is 5.4 is probably only suitable up to PHP 7.1, and 5.5 is suitable for 7.2 on onwards? ¯_(ツ)_/¯

Where is the documentation on the PHP website about this breaking change? Also, we should not need to change the tests at all. That is just masking other issues here.

@taylorotwell - I found the RFC here: https://wiki.php.net/rfc/counting_non_countables

There is a discussion here: https://externals.io/message/96211. This is the money quote from that discussion:

I'd agree that count(null) has no real benefit, and is probably an error.

@laurencei I agree with your point and actually I don''t mind if you guys want to close this and support php7.2 only on 5.5. However IMO dropping the support just because ppl might upgrade their PHP version w.o reading the breaking changes also doesn't seems like the best solution for our problem.

Also I found funny the excuse given on the Backward Incompatible Changes section of the RFC:

Environments that display warnings or convert them to more severe errors/exceptions would be affected, but this should just bring attention to a bug in the code.

I'm really surprised that there was no vote against this change.

taylorotwell · 2017-07-27T13:10:14Z

You should allow failures until PHP 7.2 is closer to release. Libraries aren't updated yet.

fernandobandeira · 2017-07-27T13:30:26Z

@taylorotwell Sure, changed it.

taylorotwell · 2017-07-27T17:59:45Z

.travis.yml

@@ -28,7 +31,7 @@ services:
  - redis-server

 before_install:
-  - if [[ $TRAVIS_PHP_VERSION != 7.1 ]] ; then phpenv config-rm xdebug.ini; fi
+  - if [[ $TRAVIS_PHP_VERSION != 7.2 ]] ; then phpenv config-rm xdebug.ini; fi


How will this affect 7.1?

This line basic disables xdebug on laravel tests, when 7.1 was added to the travis file xdebug were'nt supporting it (there was no file to remove then the build would throw an error and finish), later it started supporting 7.1 and we removed this if statement on master branch #19211.

The only change here is that 7.1 tests will run faster.

laurencei · 2017-07-30T22:38:12Z

ping @taylorotwell

You said

Also, we should not need to change the tests at all. That is just masking other issues here.

But this merge includes the changed tests.

Not sure if that was intentional or not?

Leave it with you.

taylorotwell · 2017-07-30T22:47:24Z

A bunch of count tests still fail and in fact the build fails entirely on Travis with some Xdebug error:

https://travis-ci.org/laravel/framework/jobs/259191249

Need to get this messed fixed ASAP.

fernandobandeira mentioned this pull request Jul 26, 2017

[5.5] Add PHP 7.2 to travis #20253

Closed

fernandobandeira changed the title ~~[5.4] Add PHP 7.2 to travis~~ [5.4][WIP] Add PHP 7.2 to travis Jul 26, 2017

fernandobandeira added 2 commits July 26, 2017 11:26

Test PHP 7.2

0ef2d9c

Fix styleCI

2a9bc58

fernandobandeira changed the title ~~[5.4][WIP] Add PHP 7.2 to travis~~ [5.4] Add PHP 7.2 to travis Jul 26, 2017

laurencei reviewed Jul 27, 2017

View reviewed changes

Allow PHP 7.2 to fail

456dfce

taylorotwell reviewed Jul 27, 2017

View reviewed changes

taylorotwell merged commit 8b1b894 into laravel:5.4 Jul 30, 2017

fernandobandeira mentioned this pull request Jul 30, 2017

[5.5] Fix travis CI error #20329

Merged

laurencei mentioned this pull request Aug 1, 2017

[5.4] Tweak tests for PHP7.2 #20378

Merged

paulofreitas mentioned this pull request Nov 22, 2017

SoftDelete trait throws PHP error with PHP7.2 #22162

Closed

laurencei mentioned this pull request Nov 30, 2017

Add note about 7.2.0 compat laravel/docs#3889

Closed

9bo9bo added a commit to 9bo9bo/framework that referenced this pull request Mar 6, 2019

backport: Add PHP 7.2 to travis laravel#20258

786b37f

mtvbrianking mentioned this pull request Mar 22, 2019

Soft delete "count" warning Laravel 5.3 PHP 5.6 #27968

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[5.4] Add PHP 7.2 to travis #20258

[5.4] Add PHP 7.2 to travis #20258

fernandobandeira commented Jul 26, 2017

GrahamCampbell commented Jul 26, 2017

taylorotwell commented Jul 26, 2017

morloderex commented Jul 26, 2017 •

edited

Loading

lk77 commented Jul 26, 2017

fernandobandeira commented Jul 26, 2017 •

edited

Loading

taylorotwell commented Jul 26, 2017

deleugpn commented Jul 26, 2017

fernandobandeira commented Jul 26, 2017 •

edited

Loading

laurencei Jul 27, 2017

GrahamCampbell Jul 27, 2017

laurencei Jul 27, 2017 •

edited

Loading

GrahamCampbell Jul 27, 2017

laurencei Jul 27, 2017 •

edited

Loading

GrahamCampbell Jul 27, 2017 •

edited

Loading

laurencei Jul 27, 2017

taylorotwell Jul 27, 2017

laurencei Jul 27, 2017 •

edited

Loading

fernandobandeira Jul 27, 2017

taylorotwell commented Jul 27, 2017

fernandobandeira commented Jul 27, 2017

taylorotwell Jul 27, 2017

fernandobandeira Jul 27, 2017 •

edited

Loading

laurencei commented Jul 30, 2017

taylorotwell commented Jul 30, 2017

[5.4] Add PHP 7.2 to travis #20258

[5.4] Add PHP 7.2 to travis #20258

Conversation

fernandobandeira commented Jul 26, 2017

GrahamCampbell commented Jul 26, 2017

taylorotwell commented Jul 26, 2017

morloderex commented Jul 26, 2017 • edited Loading

lk77 commented Jul 26, 2017

fernandobandeira commented Jul 26, 2017 • edited Loading

taylorotwell commented Jul 26, 2017

deleugpn commented Jul 26, 2017

fernandobandeira commented Jul 26, 2017 • edited Loading

laurencei Jul 27, 2017

Choose a reason for hiding this comment

GrahamCampbell Jul 27, 2017

Choose a reason for hiding this comment

laurencei Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

GrahamCampbell Jul 27, 2017

Choose a reason for hiding this comment

laurencei Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

GrahamCampbell Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

laurencei Jul 27, 2017

Choose a reason for hiding this comment

taylorotwell Jul 27, 2017

Choose a reason for hiding this comment

laurencei Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

fernandobandeira Jul 27, 2017

Choose a reason for hiding this comment

taylorotwell commented Jul 27, 2017

fernandobandeira commented Jul 27, 2017

taylorotwell Jul 27, 2017

Choose a reason for hiding this comment

fernandobandeira Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

laurencei commented Jul 30, 2017

taylorotwell commented Jul 30, 2017

morloderex commented Jul 26, 2017 •

edited

Loading

fernandobandeira commented Jul 26, 2017 •

edited

Loading

fernandobandeira commented Jul 26, 2017 •

edited

Loading

laurencei Jul 27, 2017 •

edited

Loading

laurencei Jul 27, 2017 •

edited

Loading

GrahamCampbell Jul 27, 2017 •

edited

Loading

laurencei Jul 27, 2017 •

edited

Loading

fernandobandeira Jul 27, 2017 •

edited

Loading