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

Conditions on boolean column type work unexpectedly #530

Closed
mvorisek opened this issue Mar 26, 2020 · 9 comments · Fixed by #532 or #535
Closed

Conditions on boolean column type work unexpectedly #530

mvorisek opened this issue Mar 26, 2020 · 9 comments · Fixed by #532 or #535
Assignees

Comments

@mvorisek
Copy link
Member

mvorisek commented Mar 26, 2020

When boolean field is added to a model like:

$m->addField('payment_is_paid', ['type' => 'boolean', 'required' => true]);

Then when condition like:

$q = '20201093';
$cond = [];
$cond[] = ['payment_is_paid', 'like', '%' . $q . '%'];
$m->addCondition($cond);

is added it filters all records with the flag set to 0, here is a part of the generated query:

... `inv`.`payment_is_paid` like 0 ...

instead of:

... `inv`.`payment_is_paid` like '%20201093%' ...



Check the behaviour of integer and date(time) types too.

@mvorisek mvorisek changed the title CRITICAL: Conditions on boolean column work unexpectedly CRITICAL: Conditions on boolean column type work unexpectedly Mar 26, 2020
@PhilippGrashoff
Copy link
Contributor

To my understanding, boolean can take only 2 values, so having a like condition on such a field doesn't seem very sensible. Despite that, I am with you that the query should be created correctly! In this case, it shouldnt match any records, but it does.

@DarkSide666
Copy link
Member

DarkSide666 commented Mar 26, 2020

Well... Thing is that atk4/data as such do not support like conditions at all. But it also don't throw any exception if condition syntax is not one of allowed/implemented ones.

What you have written above is wrong. I think you should have used expression in condition to define like comparison and then it should work correctly.
Something like this:
$cond[] = ['payment_is_paid', 'like', $m->expr('\'%[]%\'', [$q])];
or even better
$cond[] = $m->expr('[] like []', [$m->getField('payment_is_paid), '%'.$q.'%']);

But I see that in atk4/ui it's also implemented without expression and that's wrong and unsafe.

@PhilippGrashoff
Copy link
Contributor

PhilippGrashoff commented Mar 26, 2020

Hmm, I have quite some likes in my code, they work as wanted. Implementation is exactly as mvorisek has:
->addCondition('value', 'like', '%' . $search . '%')
Also I think that that would be the natural syntax for defining a like condition, matches the structure of all other conditions.

@PhilippGrashoff
Copy link
Contributor

Also, there are like tests for Persistence/Array_ . So atk data does support like it seems. Syntax there is as in mvoriseks example above:

        $m->addCondition('country', 'LIKE', 'La%');
        $result = $m->action('select')->get();
        $this->assertEquals(3, count($result));
        $this->assertEquals($a['countries'][3], $result[3]);
        $this->assertEquals($a['countries'][7], $result[7]);

Taken from PersistentArrayTest->testLike()

@mvorisek
Copy link
Member Author

mvorisek commented Mar 26, 2020

I came to this when using Grid/CRUD search which adds like '%<query>%' condition to every field. Similar usage on every column is on Aucomplete too. This implies a real-world use-case of like and ir should be supported on any column.

I think like '%<query>%' is legit, but the condition value has to remain string as like is solely string operator or more exactly the "wildcard like", i.e. when used with value containing '%' (otherwise like is equal to = operator).

@DarkSide666
Copy link
Member

Oh, you're right.
Anyway thing is that given string value %20201093% should be cast to value which boolean field supports - so to boolean. That should be true. So, not sure why it renders as 0. Will have to test more.

BTW, when we will merge in that big Field refactor PR, then there will be toString method for each field type. This method then can be used to cast value of any field type to string.

@mvorisek
Copy link
Member Author

mvorisek commented Mar 26, 2020

@DarkSide666 Yes and no. Consider regexp operator. Should we cast the right side value of that operator - regex - too? No. The same for like if there is a wildcard character.

@DarkSide666
Copy link
Member

Fix is here #532

I know it's not perfect and allthis should be done a bit different, but can't be done easily unless we refactor Field classes/types (PR) and also maybe this one #524

@mvorisek mvorisek changed the title CRITICAL: Conditions on boolean column type work unexpectedly Conditions on boolean column type work unexpectedly Mar 27, 2020
@mvorisek
Copy link
Member Author

Issue not fixed with PR #532, verifying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants