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

Searching for Numbers (other than id) #23

Closed
mrmonat opened this issue Aug 23, 2018 · 11 comments
Closed

Searching for Numbers (other than id) #23

mrmonat opened this issue Aug 23, 2018 · 11 comments

Comments

@mrmonat
Copy link

mrmonat commented Aug 23, 2018

When entering a numeric search query in the global search / the resource specific search bar the resulting SQL Query always searches for the id, so for example searching for an numeric article number is not possible.

Version: 1.0.6

@mrmonat mrmonat changed the title Searching for Numbers Searching for Numbers (other than id) Aug 23, 2018
@necenzurat
Copy link

same problem, only not searchable with +0....rest of the number

@RicardoRamirezR
Copy link

Same here, the resulting query is:

SELECT Count(*) AS aggregate
FROM   `users`
WHERE  `users`.`id` = '12832401'
   AND ( `users`.`id` LIKE '%12832401%'
          OR `users`.`cid` LIKE '%12832401%'
          OR `users`.`first_name` LIKE '%12832401%'
          OR `users`.`last_name` LIKE '%12832401%'
          OR `users`.`email` LIKE '%12832401%' )

@alexpgates
Copy link

FWIW, you can get this working by commenting lines 61-63 in nova/src/PerformsQueries.php

// if (is_numeric($search) && in_array($query->getModel()->getKeyType(), ['int', 'integer'])) {
//     $query->whereKey($search);
// }

Obviously it makes searching by ID basically impossible, but if you aren't relying on that in your searches, this seems to be an okay hack until there's a better fix.

@bomshteyn
Copy link

Hi @alexpgates ,

Thanks for pointing to the right place,

I have changed the applySearch function to the following:

protected static function applySearch($query, $search)
    {
        return $query->where(function ($query) use ($search) {
            $model = $query->getModel();
            $keyColumn = $model->getQualifiedKeyName();

            foreach (static::searchableColumns() as $column) {
                $qualifiedColumn = $model->qualifyColumn($column);
                if ($qualifiedColumn === $keyColumn){
                    $query->orWhere($qualifiedColumn, '=', $search);
                } else {
                    $query->orWhere($qualifiedColumn, 'like', '%'.$search.'%');
                }
            }
        });
    }

This way you can still search by ID while being able to search for other stuff as well

@justincone
Copy link

The code from @alexpgates works for me.

Should you submit a PR (even though this isn't the actual code base for Nova)?

@bomshteyn
Copy link

Hi @davidhemphill thanks for todays release, is fixing this issue in the plan?

@RicardoRamirezR
Copy link

No @davidhemphill here, but the answer is no, not yet

@ragingdave
Copy link

This is potentially a show-stopper for basically any app that has numeric fields that should be searchable. I would think that this use case could be very common and to hard code the id constraint seems......odd. I would think that instead of using a where key = number, it should be OR where key = number.

@rickmacgillis
Copy link

I just deliberately caused an error in the search to tell me what it's searching for in Nova 1.0.14 and this issue is still prevalent.

select * from `dgf_users`
    where `dgf_users`.`id` = 123 and
        (`dgf_users`.`name` like %123% or
         `dgf_users`.`email` like %123% or
         `dgf_users`.`cs_pin` like %123% or
         `dgf_users`.`id` like %123%)
         and `dgf_users`.`deleted_at` is null
    order by `dgf_users`.`id` desc
    limit 26 offset 0

Any change 1.0.15 can ditch the issue since it affects so many people? A smart way to fix it would be to see if id is one of the search features, or simply check if any other integer values are being searched on and if not, THEN add the id as a possible search parameter. However, the id should never be mandatory along with the other fields.

@RicardoRamirezR
Copy link

RicardoRamirezR commented Sep 11, 2018

this is now fixed in 1.0.15

@rickmacgillis
Copy link

Correct. @mrmonat please close this ticket so their team can focus on the next pressing matter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants