DataSourceResultService bug

[Leader]

First I must say that there is a lack of dicomentation explaing what the $properties input variable is: I just took a guess that it was the schema model fields list.

Anyway, there is a serious bug in your code, a complete lack of understanding about the behaviour of certain php functions and consequently the misuse of them: viz:

// line 277
            if (isset($properties[$field])) {
                $type = $properties[$field]['type'];
            } else if ($this->isDate($filter->value)) {
                $type = "date";
            } else if (array_key_exists($filter->operator, $this->operators) && !$this->isString($filter->value)) {
                $type = "number";
            }


    private function isDate($value)
    {
        $result = date_parse($value);
        return $result["error_count"] < 1 && checkdate($result['month'], $result['day'], $result['year']);
    }

 
You are using parse_date to test if a value is a date. NOT what it is intended for: all sorts of non-dates can be parsed into a date when they are not. It is the same as Excel converting phone numbers into dates: completely stupid.

In our case the STRING '012487377/1' is evaluated as a DATE and the code fails

ContextErrorException: Notice: Undefined index: startswith in C:\Development\dlg\src\TLF\PortalBundle\Services\DataSourceResultService.php line 294<br>

 Now I will be fixing the code locally by elliminating that set of test all together. I will specify the data type in the properties array since I designed the grid and I know what the data types are. I would expect that this is the case for 99.9% of developers using your code. Please fix the code at your end so that this does not trip other people up.

[Atanas Korchev]

Hi Andrew,

This check is added for convenience so the users don't have to specify the type of every property. If you don't need it just specify the types as shown in this demo: http://demos.telerik.com/php-ui/grid/remote-data-binding

$result->read('Orders', array('ShipName', 'Freight' => array('type' => 'number') , 'OrderDate', 'OrderID', 'ShipCity')

If you have a suggestion how to make the isDate function more robust please share it with us.

Regards,
Atanas Korchev
Telerik

 

 

[Leader]

Atanas, the idea that you can infer a date field from content is a logical fallacy, you will always get too many false positives. This is the same problem that expert Excel/VBA developers complain about: any string that can be parsed into a date will be, even when it is a phone number, account number (as in my example), or even just a long number. The/we Excel developers mainly complain that you can not turn the auto-type feature off. At least you have provided a solution with your array input feature.

However, it was the operator not being in the $stringOperators array that triggered the error; luckily I was using 'startswith' for that one. If it had been 'eq' then no error would have been thrown. So you could begin by testing the operator to see which arrays it is in: if only in $stringOperators then it is a string, etc. If it is in more than one then you have the same problem, just not as often.

All that I would say is that you make this clear in your documentation, explaining how to compose an array with type and field included. Also you should include a warning about the false positives when only using a string array not a multi-dimensional array.