Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[5.4] Add PHP 7.2 to travis #20258

Merged
merged 3 commits into from Jul 30, 2017
Merged

[5.4] Add PHP 7.2 to travis #20258

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0ef2d9c
Test PHP 7.2
fernandobandeira Jul 26, 2017
2a9bc58
Fix styleCI
fernandobandeira Jul 26, 2017
456dfce
Allow PHP 7.2 to fail
fernandobandeira Jul 27, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion .travis.yml
View file
Open in desktop
Expand Up @@ -4,12 +4,15 @@ php:
- 5.6
- 7.0.21
- 7.1
- 7.2

env:
global:
- setup=basic

matrix:
allow_failures:
- php: 7.2
fast_finish: true
include:
- php: 5.6
Expand All @@ -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
Copy link
Member

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?

Copy link
Contributor Author

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.

- echo "extension = memcached.so" >> ~/.phpenv/versions/$(phpenv version-name)/etc/php.ini
- printf "\n" | pecl install -f redis
- travis_retry composer self-update
Expand Down
2 changes: 1 addition & 1 deletion src/Illuminate/Database/Eloquent/Builder.php
View file
Open in desktop
Expand Up @@ -933,7 +933,7 @@ protected function callScope(callable $scope, $parameters = [])

$result = $scope(...array_values($parameters)) ?: $this;

if (count($query->wheres) > $originalWhereCount) {
if (count((array) $query->wheres) > $originalWhereCount) {
$this->addNewWheresWithinGroup($query, $originalWhereCount);
}

Expand Down
2 changes: 1 addition & 1 deletion src/Illuminate/Database/Eloquent/SoftDeletingScope.php
View file
Open in desktop
Expand Up @@ -52,7 +52,7 @@ public function extend(Builder $builder)
*/
protected function getDeletedAtColumn(Builder $builder)
{
if (count($builder->getQuery()->joins) > 0) {
if (count((array) $builder->getQuery()->joins) > 0) {
return $builder->getModel()->getQualifiedDeletedAtColumn();
}

Expand Down
2 changes: 1 addition & 1 deletion tests/Database/DatabaseEloquentBelongsToManyTest.php
View file
Open in desktop
Expand Up @@ -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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

@laurencei laurencei Jul 27, 2017 •
edited

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@laurencei laurencei Jul 27, 2017 •
edited

Choose a reason for hiding this comment

The 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 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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 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? ¯_(ツ)_/¯

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

@laurencei laurencei Jul 27, 2017 •
edited

Choose a reason for hiding this comment

The 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:

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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:

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.

}

public function testRelationIsProperlyInitialized()
Expand Down
2 changes: 1 addition & 1 deletion tests/Database/DatabaseEloquentHasManyTest.php
View file
Open in desktop
Expand Up @@ -181,7 +181,7 @@ public function testModelsAreProperlyMatchedToParents()
$this->assertEquals(2, $models[1]->foo[0]->foreign_key);
$this->assertEquals(2, $models[1]->foo[1]->foreign_key);
$this->assertEquals(2, count($models[1]->foo));
$this->assertEquals(0, count($models[2]->foo));
$this->assertEquals(0, count((array) $models[2]->foo));
}

public function testCreateManyCreatesARelatedModelForEachRecord()
Expand Down
4 changes: 2 additions & 2 deletions tests/Database/DatabaseEloquentHasManyThroughTest.php
View file
Open in desktop
Expand Up @@ -98,7 +98,7 @@ public function testModelsAreProperlyMatchedToParents()
$this->assertEquals(2, $models[1]->foo[0]->country_id);
$this->assertEquals(2, $models[1]->foo[1]->country_id);
$this->assertEquals(2, count($models[1]->foo));
$this->assertEquals(0, count($models[2]->foo));
$this->assertEquals(0, count((array) $models[2]->foo));
}

public function testModelsAreProperlyMatchedToParentsWithNonPrimaryKey()
Expand Down Expand Up @@ -129,7 +129,7 @@ public function testModelsAreProperlyMatchedToParentsWithNonPrimaryKey()
$this->assertEquals(2, $models[1]->foo[0]->country_id);
$this->assertEquals(2, $models[1]->foo[1]->country_id);
$this->assertEquals(2, count($models[1]->foo));
$this->assertEquals(0, count($models[2]->foo));
$this->assertEquals(0, count((array) $models[2]->foo));
}

public function testAllColumnsAreSelectedByDefault()
Expand Down
6 changes: 4 additions & 2 deletions tests/Database/DatabaseEloquentIntegrationTest.php
View file
Open in desktop
Expand Up @@ -1036,10 +1036,12 @@ public function testForPageAfterIdCorrectlyPaginates()
EloquentTestUser::create(['id' => 2, 'email' => 'abigailotwell@gmail.com']);

$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()
Expand Down