-
Notifications
You must be signed in to change notification settings - Fork 11k
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.
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
[5.4] Add PHP 7.2 to travis #20258
[5.4] Add PHP 7.2 to travis #20258
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems strange that you need to use If you dont know what the result is going to be before using Kinds of feels like we are hiding a bigger issue? Or at the minimum we are not being consistent in the tests... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue I assume is that php doesn't allow passing null to count anymore? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Put another way; it would seem, according to how this test is written, that you cannot safely use So just because the test passes doesnt mean we are solving the underlying issue? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It just needs to be "countable". PHP 7.2 has decided that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeh, in PHP 7.2, those are the types considered countable:
Seems like a fair change perhaps. Ping @taylorotwell. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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? ¯_(ツ)_/¯ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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:
I'm really surprised that there was no vote against this change. |
||
} | ||
|
||
public function testRelationIsProperlyInitialized() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1036,10 +1036,12 @@ public function testForPageAfterIdCorrectlyPaginates() | |
EloquentTestUser::create(['id' => 2, 'email' => '[email protected]']); | ||
|
||
$results = EloquentTestUser::forPageAfterId(15, 1); | ||
$this->assertEquals(1, count($results)); | ||
$this->assertInstanceOf('Illuminate\Database\Eloquent\Builder', $results); | ||
$this->assertEquals(2, $results->first()->id); | ||
|
||
$results = EloquentTestUser::orderBy('id', 'desc')->forPageAfterId(15, 1); | ||
$this->assertEquals(1, count($results)); | ||
$this->assertInstanceOf('Illuminate\Database\Eloquent\Builder', $results); | ||
$this->assertEquals(2, $results->first()->id); | ||
} | ||
|
||
public function testMorphToRelationsAcrossDatabaseConnections() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How will this affect 7.1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.