Suggested behavior for Query

[tonydspaniard]

Currently in order to make a search function (not following gii approach) a user needs to do the following:

$query = Contact::find();
if(!empty($this->full_name))
{
   $query->where(['like', 'full_name', $this->full_name]);
}
// or 
if(trim($this->email) !== '')
{
   $query->where(['email' => $this->email]);
}

Or using the gii approach (creating an addCondition method) it is always required to search for empty or NULL values. Which even I think is good for demo purposes to have the addCondition on the search model reproduced by gii, I find it quite of non-sense to have the same method on each search model.

Failing to not check against NULL values will cause two different results on both scenarios. On the first one like, it throws an error:

Whereas on the second one will produce the following SQL:

SELECT * FROM tbl_contact WHERE full_name IS NULL LIMIT 10

Which IMHO I think is the wrong behavior, even if the adCondition function of the search model strictly checks for empty strings not NULL attributes.

_Suggestion_

I believe that everybody should be able to use the following:

$query = Contact::find();
$query->where(['like', 'full_name', $this->full_name]);
$query->where(['email' => $this->email]);

And NULL or empty values should be removed from the resulting Query object as it used to happen with CDbCriteria on Yii1. That will make our search classes cleaner and without the need to check whether I have NULL or empty values on my queries and also we will follow an approach that was commonly used by new users.

I do not think we should force the IS NULL search if an attribute has null value. For that purposes we already have yii\db\Expression don't you agree? I think it makes more sense and is less error prone (in terms of search results).

$query->where(['email' => yii\db\Expression('IS NULL')]);

[creocoder]

But huge number of scenarios is just fine? right? ;)

As you remeber i talked about some "secret list" when to separate. Did you see mine? No :) So sarcasm is not relevant ;)

[Ragazzo]

Will look to what it will lead afterall, one more shit-feature that will be always noticed by other fw developers as Yii2 bad practice or it will be rejected. :) Anyway my opinion is not for implementing.

[samdark]

@Ragazzo do not offtop about scenarios and "shit-feature" here and there please. This issue isn't connected.

[Ragazzo]

I am not offtop, just saying what everyone else are denying :) as you want :)

[samdark]

Well, use forums since it's scenarios are definitely not related to this issue that is about Query. Thanks.

[tonydspaniard]

just dont copy it, get some base-search-model.

Using that concept, I can also avoid the Search model all together right? Is that what you are saying, otherwise, why the search model was created as a different Model class? If you understand that concept then you understand why I wonder about the nature of Query and propose something that could ease newbie developer introduction to Yii2.

The adCondition() rejects the use of empty strings if I recall well, and @qiangxue throw the following question:

What about empty strings? Or a string of blanks?

So, I am not sure adCondition() fits even as a solution. Users will be forced to use the current solution for some to work with IS NULL and other to work as expected by that function. My concern here is that new comers will have to know that they require that function to be implement per se and some NULL values will be converted to IS NULL and that will be an issue if they create a public variable that wasn't initialized on their queries.

IMHO, @slavcodev is heading to propose the right way to do it without the need to implement extra code on our apps, extra functions on Yii2, or extra base classes and the change to current process wont be affected that much. So, if people wish to use that adCondition function on its apps or create a baseClass that adds that fine... I vote for something more efficient.

[Ragazzo]

@tonydspaniard

propose something that could ease newbie developer introduction to Yii2.

ignoring params is not that good, you said lets null or "" param value will not be counted as IS NULL or =="", i think this is unacceptable behavior.

I do not think we should force the IS NULL search if an attribute has null value. For that purposes we already have yii\db\Expression don't you agree? I think it makes more sense and is less error prone (in terms of search results).

do you really want this behavior be in query-object?

[tonydspaniard]

@ragazzo

ignoring params is not that good, you said lets null or "" param value will not be counted as IS NULL or =="", i think this is unacceptable behavior.

Read the whole thread, ideas have evolved. You just reading the first suggestion?

[Ragazzo]

nope, i read it, i just pointed to where it started all from. other solutions are also not good as for me, and only makes query object behavior unexpected or complex with all additional true/false params.

[creocoder]

@qiangxue I think our first idea about simple modification and simple rule like ignore nulls in all cases is right. There is no empty string and string with spaces trouble. This trouble can be solved with default validator. And as result you will get only nulls and real values inside attributes. So all we need to change is how where() work with nulls and this is enough.

[creocoder]

@qiangxue For example look at this simple action:

    public function actionList($query = null)
    {
        $models = Tag::find()->where(['like', 'name', $query])->limit(50)->all();
        $items = [];

        foreach ($models as $model) {
            $items[] = ['name' => $model->name];
        }

        Yii::$app->response->format = Response::FORMAT_JSON;

        return $items;
    }

Currently this work wrong. At practice we have 99% cases like that.

[samdark]

Agree. For like it does't make sense to add condition if query is empty or null.

But it absolutely makes sense for where.

[qiangxue]

It's fine ignoring empty string/null for like operator, but it doesn't make sense to do the same for hash condition or equal operator because it's unexpected and may cause security problem. However, if we don't ignore null/empty string for hash condition/equal operator, then for consistency, like should behave the same.

In general, I agree with @Ragazzo that this is more like a view-layer feature. I'm not saying we should keep the current addCondition design because it is repeated in every search model. Moving it to a base class is certainly fine, but then why don't we do a step further to abstract this thing out and make it become a common function?

@Ragazzo You have designed the filter classes for debugger. Is there anything that may help here?

[Ragazzo]

but it doesn't make sense to do the same for hash condition or equal operator because it's unexpected and may cause security problem

146 % agree :)

You have designed the filter classes for debugger. Is there anything that may help here?

dont think so, because it was simple addCondition method as you see - . All custom things where handled externally to query object as you see. I also handle some match-rules externally as you see.

[Sammaye]

Can I ask for a brief overview on what has happened here?

Basically I am converting some Yii1 models to Yii2 now. I want to replicate Yii1's method of compare() so that it can tell the difference (for me) between operators like <, >, = and LIKE.

At the moment it seems as though I can automatically produce LIKE conditions and that I would need either:

• A separate base model
• Or duplicated code

To handle this and get it working correctly.

Should I wait for this or should I implement something of my own?

[qiangxue]

Perhaps we should consider adding a trait that provides commonly used query methods.

Based on the above discussion, it seems to me there are two kind of query needs:

• the query parameters are received from user input
• the query parameters are provided by developers

While they share a lot of commonalities, there are also a lot of differences, like we discussed above. A typical example is how to treat the empty values: for the first situation we don't want to ignore empty values, while the second situation, we want to ignore them.

The Query object provides the methods for the first situation. The proposed trait should handle the second situation.

[hijarian]

@creocoder To cite your example:

public function actionList($query = null)

Why you default $query to null if you know beforehand that you're going to use it in the LIKE query? Default it to '' and your "99% of problems" will be solved without changing the fundamental behavior of query syntax.

I can't and won't decide for everyone, but I personally think that if you tell in your infrastructure code the following:

$query->where(['like', 'attribute', '']);

You're clearly stating that you want WHERE attribute LIKE '', which is returning Empty set right now for me in MySQL 5.5 and is the functional equivalent of WHERE attribute = '' overall.

In similar way, if you tell

$query->where(['like', 'attribute', NULL]);

You should get an exception before this query will even reach the underlying RDBMS, because the data layer knows already that you want to make WHERE attribute LIKE null, which is insanity as it'll never return a value to you even if the value of the field will really be NULL in database table.

If you know that your user can send you ?attribute=&other_attribute=somevalue in the request, it's your business rule whether to treat the attribute as non existent at all in query (i. e., to be null) or as an empty string. At the level of where() method you're telling your data layer precise instructions about what query to execute.

As @Ragazzo have already said, there's the data layer (among other infrastructure) and the presentation layer, and the domain logic layer in between them, and when you're on the level of data layer you are expected to be decoupled from the user input long ago and to know what you're doing.

@qiangxue In no way "spaces only" strings like ' ' or '\t\v\t\n \t \n' should be treated as '', because this way you crippling the possibilities of your query-constructing methods without any chance for developers to recover. What if your user really want to search for strings consisting of exactly five spaces and nothing else?

If you wanted the comments from framework users at all, of course.

[qiangxue]

Below is my proposal to solve this issue:

Add Query::filter() which sets the WHERE part of a query, like where(). The only difference between filter() and where() is that the former will preprocess the input data. If the input data is empty (null, empty string, blank string, or empty array), the corresponding condition will be ignored.

For example, $query->filter(['name' => $name]) or $query->filter(['like', 'name', $name]) will not generate a WHERE part if $name is empty.

Similarly, we add Query::andFilter() and orFilter().

This should allow us to greatly simply the search model generated by Gii.

What do you think?

[Sammaye]

Sounds good and fits with function naming like array_filter

[Ragazzo]

how about callback for this functions ?

[samdark]

How would you use callback in this case?

[Sammaye]

Yeah, this seems to have very specific functionality, not sure if a callback is needed or usable honestly.

[samdark]

Sounds OK since it's common enough.

[samdark]

Who's going to handle it? I can take it if noone want to.

[Ragazzo]

@samdark for the custom filter, something like builtin thing in rails / ruby.

[samdark]

Any real world example?

[Ragazzo]

filtering in like to make it possible to handle different characters like %/\, currently we need to pass fourth param, but as for me callback is more flexible.

[samdark]

What this callback would do exactly in case of %/\? Can show some pseudo-code?

[Ragazzo]

it will escape or not escape custom characters like now fourth param is doing, however as for me anonymous function is better in this case since it can handle some logic.

[samdark]

Why can't you do it w/o anonymous function?

That's what you're proposing:

$name = '%test%';
$query->filter(['name' => $name], function($value) {
    return escape($value);
});

That's what can be done currently:

$name = escape('%test%');
$query->filter(['name' => $name]);

[Ragazzo]

sure, but anonymous function can handle different logic - not one row call, also to be true we can skip this filter methods if going with your argumentation )

[samdark]

There are no benefits using anonymous function in this particular case. Everything that can be handled in an anomous function can be handled without it in this case.

filter method is just a shortcut. It may have been implemented as a helper but a method of the query fits better.

[Ragazzo]

maybe trait since it can be re-usable?

[samdark]

Where it can be reused?

[qiangxue]

@samdark feel free to work on it. Thanks.

[qiangxue]

Note that the syntax of filter() is exactly the same as where(). The only difference is that filter() will remove empty params.

[samdark]

Yep. I'll reuse where.

[Ragazzo]

it can be used in similar search models that for example was built in debug extension. anyway if you dont want to do it, i am fine )

[samdark]

@qiangxue should filter methods be added to QueryInterface?

[cebe]

I think not as this would mean all nosql dbms must implement LIKE syntax.

[qiangxue]

should filter methods be added to QueryInterface?

I think so since the interface contains where(). @cebe: the filter() method doesn't specify whether LIKE should be supported or not. It's just a variant of where().

[samdark]

What should be done in the following case?

$query = new Query;
$query->filter('id = :id AND staus=:status', [':id' => 1, ':status' => '']);

[qiangxue]

Should be the same as where(). Basically, filter() should not handle bind parameters.

[samdark]

There's a name conflict with elastic search. There's filter operation and the corresponding method already.

[cebe]

need a different name for this then. elasticsearch has filter and query as reserved terms.

[samdark]

Any suggestions?

[Sammaye]

Short of making it long like filterEmpty() though ES' filter is deep within the query structure itself

[qiangxue]

How about renaming ES's filter() to addFilter()?

[samdark]

It's not adding but replacing same as where https://github.com/yiisoft/yii2/blob/master/extensions/elasticsearch/Query.php#L462

[qiangxue]

I understand. I just want to keep filter() in yii\db\Query since it is much more commonly used. Of course, if we can come up with a good alternative, we can keep ES unchanged.