-
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.3] Add test to proove issue with numeric aggregation #14991
Conversation
/** | ||
* Get a database connection instance. | ||
* | ||
* @return Illuminate\Database\Connection |
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.
missing leading slash (please only added it to phpdoc, and not to code)
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.
The file is not in a namespace. Therefor it should not be needed? Or am I wrong?
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.
Our phpdoc requires a leading slash.
@GrahamCampbell is this considered a bug or do you want me to make everything pretty first? |
EloquentBuilderTestUser::create(['id' => 1, 'email' => '[email protected]', 'created_at' => '2016-08-10 09:21:00']); | ||
EloquentBuilderTestUser::create(['id' => 2, 'email' => '[email protected]', 'created_at' => '2016-08-01 12:00:00']); | ||
|
||
$this->assertEquals('2016-08-10 09:21:00', EloquentBuilderTestUser::max('created_at')); |
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 is not numeric.
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 method only returns ints and floats.
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.
@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?
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.
Well, this method never claimed to support doing things like this?
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.
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?
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.
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
Thanks for the PR. Please could you send to 5.1 so we can merge it along with #14994. |
* Add test Add test to see if min and max can be used on datetime type of fields Refs. #14991 * Remove leading slashes * Fix phpdoc class reference * Add leading slash
I dont know if this is the place to place the test.
I am having issues with min() and max() on the eloquent builder is expected to be a float or int.
I have a scenario, where I am interrested in seeing the highest and lowest datetime in the database.
The problem seems to have been implemented in following pull request:
#14793