Crud: [Bug] Date Range Filter ignores date_range_options->locale

[axelzuzek]

Bug report

Date Range Filter ignores date_range_options->locale

What I did

   $this->crud->addFilter([ // daterange filter
            'type' => 'date_range',
            'name' => 'from_to2',
            'label' => 'Datesearch',
            'locale' => ['format' => 'DD/MM/YYYY'],
            'date_range_options' => [
                'locale' => ['format' => 'DD/MM/YYYY']
            ]

        ],
            false,
            function ($value) { // if the filter is active, apply these constraints
                $dates = json_decode($value);
                $this->crud->addClause('where', 'created_at', '>=', $dates->from);
                $this->crud->addClause('where', 'created_at', '<=', $dates->to . ' 23:59:59');
            });

What I expected to happen

Date format in filter DD/MM/YYYY

What happened

Date format MM/DD/YYYY in filter

What I've already tried to fix it

upgraded from 3.6 to 4.0 ;-)

Backpack, Laravel, PHP, DB version

Backpack 4.0.*
Laravel 6.*
PHP 7.2.18
MySql 5.7.*

When I run php artisan backpack:version the output is:
PHP 7.2.11 (cli) (built: Oct 10 2018 02:04:07) ( ZTS MSVC15 (Visual C++ 2017) x64 )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.2.0, Copyright (c) 1998-2018 Zend Technologies

LARAVEL VERSION:

laravel/framework v6.0.4 The Laravel Framework.

BACKPACK VERSION:

backpack/crud v4.x-dev f84faa4 Quickly build an admin interfaces using Laravel 6, CoreUI, Boostrap 4 and jQuery.
backpack/generators 2.0.4 Generate files for laravel projects
backpack/langfilemanager 2.0.1 An interface to edit language files, for Laravel Backpack.
backpack/permissionmanager 5.0.2 Users and permissions management interface for Laravel 5 using Backpack CRUD.

[jsvini]

@axelzuzek I made a pull request to fix that https://github.com/Laravel-Backpack/CRUD/pull/2113
ops, this pull is for datepicker and not date_range

[pxpm]

Hello @axelzuzek

Thanks for reporting it. In fact this question is way more complicated than adding a property to the field and allow developers to change their format.

I'v spent some hours around it and have got some good info.

So first thing I did was to allow the filter to accept format option. Everything went well, if not specified revert to default YYYY-MM-DD.

First try, change format => 'DD/MM/YYYY. Search results are broken.

Search around and find that when using where('date', <=, '$someDate') mysql actualy does a character comparison, and this will only work if date is ALWAYS SPECIFIED as 'YYYY-MM-DD'

Example from sitepoint answer:

comparison tests on strings (Which these are. PHP doesnt know the difference between a string and a date.) are done character-to-character, left to right. 2 = 2, 0 = 0, 1>0, therefore string 1 > string 2.

IF your strings are guaranteed to be in the format YYYY-MM-DD, there wont be a problem.
If it was YYYY-DD-MM or DD/MM/YYYY or MM/DD/YYYY, you’d have problems.

(Just for example’s sake…)
YYYY-DD-MM
Str1 = ‘2010-30-01’, Str2 = ‘2010-05-02’. Str1 > Str2.

DD/MM/YYYY
Str1 = ‘30/01/2010’ , Str2 = ‘05/02/2010’. Str1 > Str2.

MM/DD/YYYY
Str1 = ‘12/05/2009’, Str2 = ‘01/05/2010’. Str1 > Str2.

So i though, maybe we can guarantee that we compare the two times in the same format. :bulb:

First attempt get the date in UNIX_TIMESTAMP from database and compare with strtotime() our values. If i can get date in unix from db and compare unix date from filter we are done. World is ours! :earth_africa:

Searching, learning, trying, failing ... success! I could achieve comparing two dates as UNIX. :tada:

$this->crud->addFilter([ // daterange filter
           'type' => 'date_range',
           'name' => 'date_range',
           'label'=> 'Date range',
           'locale' => [
               'format' => 'DD/MM/YYYY'
           ]
         ],
         false,
         function ($value) { // if the filter is active, apply these constraints
             $dates = json_decode($value);

             //forcing test dates, i explain later why :)

             $dates->from = '01-11-2019';
             $dates->to = '30-11-2019';

             $this->crud->query->whereRaw("UNIX_TIMESTAMP(STR_TO_DATE(date,'%Y-%m-%d')) >= '".strtotime($dates->from)."'")
             ->whereRaw("UNIX_TIMESTAMP(STR_TO_DATE(date,'%Y-%m-%d')) <= '".strtotime($dates->to)."'");
         });

I just want to make sure I can strtotime() no matter what format ... * I Can't*

Better explained with examples:

strtotime('30-11-2019') => works
strtotime('30/11/2019') => don't work, returns false.

That is how strtotime is implemented. From documentation:

Dates in the m/d/y or d-m-y formats are disambiguated by looking at the separator between the various components: if the separator is a slash (/), then the American m/d/y is assumed; whereas if the separator is a dash (-) or a dot (.), then the European d-m-y format is assumed.

So, ofcourse knowing this any developer could set their format => 'DD/MM/YYYY' and work around this issue to get the desired results.

But I think what people would be looking for is to replace only for presentation in the interface ?

Or do you guys think who uses another format than the convention wants to really receive those values as 25/11/2019 in their filter ?

• They could not apply the addClause() directly, because that will lead to wrong results.
• Have to use some kinky sql to get date comparison

Let me know your thougths. :+1:

Best,
Pedro

[axelzuzek]

Hi Pedro,

I am not shure if I got the basic problem, but why not use Carbon for the date parsing part?

Example
// user chosen format
$formatstring = 'DD/MM/YYYY';
$timezone = 'Europe/London';
$from = Carbon::createFromFormat($formatstring, $dates->from, $timezone);
$to= Carbon::createFromFormat($formatstring, $dates->to, $timezone);

... crud->query->whereBetween('column_name', [$from->copy()->startOfDay(), $to->copy()->endOfDay()])

[pxpm]

Hello @axelzuzek ,

Didn't test with Carbon, but it's probably one solution.

The main problem i see here is the added complexity, using carbon or converting to UNIX_TIMESTAMP, they both work, but in the end, we always end up constructing a date instance so it can be compared.

For you, in your project, does it make sense to have all that headache, or you can change format, user will see in your format, but in the end you just do:

$this->crud->addClause('where','date','>=',$filterValue);

???

And only need to change filter code in case you have specific data comparison to do different than the "regular" ?

Or, as i said in PR #2253 we can add a forceFormat => true that makes the data end up as user formatted if not defined we keep the simplex operation just showing data different, but inner works are the same ?

I hope i made myself clear on this one.

Best,
Pedro

[axelzuzek]

Hi Pedro,

for my current project it's only important that I have a way to change the date format that is displayed to the user in the daterange dropdown input field - currently it's "11/21/2019 - 11/21/2019" and no one gets this in Europe as month and day are swapped. Most backpack devs in Europe will have this problem. For me it doesn't matter if I have to do the datestring parsing and query part by myself.

Viewed from the backpack goals it would probably be nicer if the programmers "interface" is

1) (optional) set your date format string in the daterange filter: 'format' => 'DD/MM/YYYY' with config("backpack.base.default_date_format") as default
2) get the dates as Carbon Date Objects (Laravel standard) in the filter function ($value) {
$value->dates->from // instance of Carbon\Carbon
$value->dates->to

Carbons default getter returns "date_to_string" in standard YYYY-MM-DD so
$this->crud->addClause('where','date','>=',$values->dates->from);
would work too. And in addition you can use all other carbon methods like ->subDays(8) ->startOfDay() and many more if needed

Best regards
Axel

[pxpm]

Thanks for quick reply @axelzuzek !

Really appreciate.

I couldn't find better way to do it, if we get the values as Carbon instances.

But well .. for now this is the code that passes the value to filter callback:

public function applyFilter(CrudFilter $filter, $input = null)
    {
        ....
            if (is_callable($filter->logic)) {

                ($filter->logic)($input->get($filter->options['name']));
            } else {
                $this->addDefaultFilterLogic($filter->name, $filter->logic, $input->all());
            }
       ...
    }

I am not seeing an easy way to introduce there the carbon interface unless we do some

$convertToCarbon = ['date_range','date','datetime_range'];

if(in_array($convertToCarbon, $filter->options['name']))) {

//we also need to make a convention for this options array. In date_range filter is this one.
$filterOptions= $filter->options['date_range_options'] ?? [];

 ($filter->logic)(Carbon::createFromFormat($filterOptions['format'] ?? config('backpack.base.default_date_format'),$input->get($filter->options['name']));

}

Didn't test it, but we should expect a carbon instance in filter callback. Are we sure we allways want a carbon instance in callbacks that uses dates/times ?

I updated the PR to show the format from config instead of the hardcoded YYYY-MM-DD.

I will let @tabacitu give a look at our conversation and decide if we should go by returning carbon instances or keep this simpler way.

Note: I vouche to keep things simple with only presentation changes. Developers could create a carbon instance if they want in filter callback!

Best,
Pedro

[axelzuzek]

@pxpm thx I appreciate it anyways!

[tabacitu]

Hi guys!

In my opinion, the values you get in the callback should be mysql date strings (YYYY-MM-DD). I think that's the best compromise in terms of intuition and ease of use.

I think:

• that's what's convenient to me, as a programmer, because I can add it to queries, as @pxpm mentioned;
• that's more intuitive than a Carbon date (arguably);

Then again, having the date as Carbon could be convenient for some use cases, but it could become unintuitive if you think of the column as an extension of the Model attribute:

• If the model attribute is cast as Date, Eloquent will return it as a Carbon date; otherwise it will be string. Then similarly, you might expect the value in the closure to be a Date here too, right?
• The opposite side of the coin is even more interesting - if you've specifically decided NOT to cast the attribute as Date, so that you DO NOT receive a Carbon date - you'd then be surprised it's a Carbon date right?

Two ways to avoid this:
(1) The value to be mysql date all the time.
(2) The value to be cast according to the Model.

Then again, you could think of the value as a direct response from the input. Not an attribute on the model. Right? And in most cases, I expect the JS date plugins to show one date format to the user, but store a mysql date in the actual input, because that's what I, as a developer, expect - for JS to take care of this for me. So from this point of view, only solution (1) above would make sense.

Since:

• having a mysql date is a solution for both ways of thinking above;
• having a Carbon date would imply changing the applyFilter functionality and having some filters behaving some way, and other filters another way;
• most of the time you only use the value to add it to a query (like the example);
• it would be super-easy for the dev to wrap a Carbon instance around a mysql date;

I think the best way to go here would be to just assume that if you're a developer, you'll want a mysql date in code. You don't care what the admin sees or uses, you want a date format you can work with in code.

Do you guys agree with my reasoning? I guess I could be convinced solution 2 would be better, since Carbon is backwards-compatible that way (their default getter is mysql date).

Cheers!

[pxpm]

Well you summarized and polished my thoughts.

The PR for solution 1 is layin' around: #2253

Best,
Pedro

[axelzuzek]

@tabacitu I think you are right and solution 1 is more intuitive for most developers - it is more consistent to get a date string there and not an object. On the other hand coupling it to the attribute-cast settings of the model seems elegant, but probably confusing if you are not aware of that.

Best regards
Axel

[pxpm]

This is beeing addressed in #2253

I will be closing this so we don't have discussions in multiple places.

I'v already linked this issue in that PR. And I think now is getting close to be finished and merged.

Thanks again @axelzuzek for bring this to our attention.

Wish you the best,
Pedro