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.

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.3] Add test to proove issue with numeric aggregation #14991

Closed
wants to merge 2 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 71 additions & 0 deletions tests/Database/DatabaseEloquentBuilderTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,43 @@

class DatabaseEloquentBuilderTest extends PHPUnit_Framework_TestCase
{
public function setUp()
{
$db = new Illuminate\Database\Capsule\Manager;

$db->addConnection([
'driver' => 'sqlite',
'database' => ':memory:',
]);

$db->bootEloquent();
$db->setAsGlobal();

$this->createSchema();
}

/**
* Setup the database schema.
*
* @return void
*/
public function createSchema()
{
$this->schema()->create('users', function ($table) {
$table->increments('id');
$table->string('email')->unique();
$table->timestamps();
});
}

/**
* Tear down the database schema.
*
* @return void
*/
public function tearDown()
{
$this->schema()->drop('users');
m::close();
}

Expand Down Expand Up @@ -511,6 +546,15 @@ public function testDeleteOverride()
$this->assertEquals(['foo' => $builder], $builder->delete());
}

public function testAggregatedValuesOfDatetimeField()
{
EloquentBuilderTestUser::create(['id' => 1, 'email' => 'test1@test.test', 'created_at' => '2016-08-10 09:21:00']);
EloquentBuilderTestUser::create(['id' => 2, 'email' => 'test2@test.test', 'created_at' => '2016-08-01 12:00:00']);

$this->assertEquals('2016-08-10 09:21:00', EloquentBuilderTestUser::max('created_at'));
Copy link
Member

Choose a reason for hiding this comment

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

This is not numeric.

Copy link
Member

Choose a reason for hiding this comment

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

This method only returns ints and floats.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@GrahamCampbell I know. But isn't it wrong?

If I cant use the max('date') on the QueryBuilder on a datetime field. How do you want me to get the maximum date then?

Copy link
Member

Choose a reason for hiding this comment

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

Well, this method never claimed to support doing things like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you are saying that I will have to make some kind of raw SQL to archive what has been possible before the numeric update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also I think the phpdoc said that it could return a string or mixed before. The behavior has not changed.

If you are at larecon I won't mind discussing it with you

$this->assertEquals('2016-08-01 12:00:00', EloquentBuilderTestUser::min('created_at'));
}

public function testWithCount()
{
$model = new EloquentBuilderTestModelParentStub;
Expand Down Expand Up @@ -724,6 +768,26 @@ protected function getMockQueryBuilder()

return $query;
}

/**
* Get a database connection instance.
*
* @return Illuminate\Database\Connection
Copy link
Member

Choose a reason for hiding this comment

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

missing leading slash (please only added it to phpdoc, and not to code)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The file is not in a namespace. Therefor it should not be needed? Or am I wrong?

Copy link
Member

Choose a reason for hiding this comment

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

Our phpdoc requires a leading slash.

*/
protected function connection()
{
return Illuminate\Database\Eloquent\Model::getConnectionResolver()->connection();
}

/**
* Get a schema builder instance.
*
* @return Illuminate\Database\Schema\Builder
Copy link
Member

Choose a reason for hiding this comment

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

same

*/
protected function schema()
{
return $this->connection()->getSchemaBuilder();
}
}

class EloquentBuilderTestScopeStub extends Illuminate\Database\Eloquent\Model
Expand Down Expand Up @@ -844,3 +908,10 @@ public function bazes()
return $this->hasMany('EloquentBuilderTestModelFarRelatedStub', 'foreign_key', 'id', 'bar');
}
}

class EloquentBuilderTestUser extends Illuminate\Database\Eloquent\Model
{
protected $table = 'users';
protected $guarded = [];
public $timestamps = false;
}