scoumbourdis / grocery-crud

Grocery CRUD is a PHP Codeigniter Framework library that creates a full functional CRUD system without the requirement of extra customisation to the JavaScripts or the CSS to do it so.
http://www.grocerycrud.com
GNU General Public License v3.0
1.01k stars 472 forks source link

GroceryCrud search fails if ->where() or ->set_relation() are used - FIXED #341

Open Doctor-Paul opened 8 years ago

Doctor-Paul commented 8 years ago

Using GC 1.5.2 and CI 3.0

As others have noted, "search all" fails completely if a ->where() statement has been used.

This is because GC is chaining calls to ->or_like() in addition to ->where() and the resulting SQL is simply wrong.

Search also fails for the same reason if made against a single column which is the subject of a call to ->set_relation()

I have produced a modified version of the set_ajax_list_queries() function which fixes this problem.

If a "where" exists, it generates a query string of chained OR LIKE statements and then feeds it to the ->where() function, which CI automatically ANDs with the existing where statement.

Happy to share this code, work in progress shown below:

Doctor-Paul commented 8 years ago
protected function set_ajax_list_queries($state_info = null)
{
    if(!empty($state_info->per_page))
    {
        if(empty($state_info->page) || !is_numeric($state_info->page) )
            $this->limit($state_info->per_page);
        else
        {
            $limit_page = ( ($state_info->page-1) * $state_info->per_page );
            $this->limit($state_info->per_page, $limit_page);
        }
    }

    if(!empty($state_info->order_by))
    {
        $this->order_by($state_info->order_by[0],$state_info->order_by[1]);
    }

    if(!empty($state_info->search))
    {
        if(!empty($this->relation))

            foreach($this->relation as $relation_name => $relation_values)
                $temp_relation[$this->_unique_field_name($relation_name)] = $this->_get_field_names_to_search($relation_values);

        if($state_info->search->field !== null)
        {
            if(isset($temp_relation[$state_info->search->field]))
            {
                if(is_array($temp_relation[$state_info->search->field])) {

                    if(!empty($this->where)) { // we already have a where(), so chaining or_like won't work
                        $like_where = '(';
                        $escaped_text = $this->basic_model->escape_str($state_info->search->text);

                        foreach($temp_relation[$state_info->search->field] as $search_field) {
                            $like_where .= '`'.$search_field."` LIKE '%".$escaped_text."%' OR ";
                        }

                        if ($like_where != '(') {
                            $like_where = substr($like_where, 0, -4).')'; // trim off the trailing  OR and add closing bracket
                            $this->where($like_where);
                        }

                    } else { // there isn't a where(), so just chain the or_likes

                        foreach($temp_relation[$state_info->search->field] as $search_field) {
                           $this->or_like($search_field, $state_info->search->text);
                        }

                    }

                } else {
                    $this->like($temp_relation[$state_info->search->field] , $state_info->search->text);
                }

            } elseif(isset($this->relation_n_n[$state_info->search->field])) {
                $escaped_text = $this->basic_model->escape_str($state_info->search->text);
                $this->having($state_info->search->field." LIKE '%".$escaped_text."%'");
            } else {
                $this->like($state_info->search->field , $state_info->search->text);
            }
        }
        else // search against all columns
        {
            $columns = $this->get_columns();
            $search_text = $state_info->search->text;

            if(!empty($this->where))
                foreach($this->where as $where)
                    $this->basic_model->having($where[0],$where[1],$where[2]);

            //Get the list of actual columns and then before adding it to search ..
            //compare it with the field ... does it exists? .. if yes.. great ..
            //go ahead and add it to search list.. if not.. just ignore it                
            $field_types = $this->get_field_types();
            $actual_columns = array();
            $srch = array();

            foreach($field_types as $field) {
                if( !isset($field->db_extra) || $field->db_extra != 'auto_increment' )
                    $actual_columns[] = $field->name;
            }

            foreach($columns as $column)
            {
                if(isset($temp_relation[$column->field_name]))
                {
                    if(is_array($temp_relation[$column->field_name]))
                    {
                        foreach($temp_relation[$column->field_name] as $search_field)
                        {
                            //$this->or_like($search_field, $search_text);
                            $srch[] = $search_field;
                        }

                    }
                    else
                    {
                        //$this->or_like($temp_relation[$column->field_name], $search_text);
                        $srch[] = $temp_relation[$column->field_name];
                    }
                }
                elseif(isset($this->relation_n_n[$column->field_name]))
                {
                    //@todo have a where for the relation_n_n statement
                }
                else
                {
                    if(array_search($column->field_name, $actual_columns) === false) { // amit mod - bail out if column not in db
                        continue;
                    }
                    //$this->or_like($column->field_name, $search_text);
                    $srch[] = $column->field_name;
                }
            }

            //die(var_dump($srch));
            if(!empty($this->where)) { // we already have a where(), so chaining or_like won't work
                $like_where = '(';
                $escaped_text = $this->basic_model->escape_str($search_text);

                foreach($srch as $search_field) {
                    $like_where .= '`'.$search_field."` LIKE '%".$escaped_text."%' OR ";
                }

                if ($like_where != '(') {
                    $like_where = substr($like_where, 0, -4).')'; // trim off the trailing  OR and add closing bracket
                    $this->where($like_where);
                }

            } else { // there isn't a where(), so just chain the or_likes

                foreach($srch as $search_field) {
                    $this->or_like($search_field, $search_text);
                }

            }

        }
    }
}
buoncri commented 8 years ago

:+1:

multimulti commented 8 years ago

:+1: Perfect, please include this in the next version of GC!

moritzgloeckl commented 8 years ago

Hi, this problem still persists when you select a table which has a self referencing field. For example I have a table named Company, with a field name headquarter which is a foreign key to Company. Error is: Column in where clause is ambiguous.

multimulti commented 8 years ago

Only for clarification: The code above fixes two things for me which are broken in stock GC 1.5.3:

moritzgloeckl commented 8 years ago

@multimulti I'm also using GC 1.5.3 and ajax_list_info throws this error for me, with the updated function:

Column 'name' in where clause is ambiguous

SELECT * FROM company LEFT JOIN company as j56bb8bb3 ON j56bb8bb3.company_id = company.headquarter WHERE company.deleted != '1' AND name LIKE '%test%' ESCAPE '!'

Maybe I did something wrong? I just replaced the original function with the updated one from above...

Doctor-Paul commented 8 years ago

GC does throw errors if you use set_relation() and two tables contain the same field/column name.

Can you add the table name to your query so that the SQL comes out as "... AND company.name LIKE....."?

Otherwise, I work round it by making sure I use unique field names - so change the 'name' field in the 'company' table to 'company_name' or similar.

Something at the back of my mind is telling me that this "ambiguous clause" problem has been fixed in another thread, either here on Github or on the GC forum. If I'm right and I can find it I will post it here.

A further clarification: My function also includes code from Amit which stops "dummy columns" causing search errors.

moritzgloeckl commented 8 years ago

What do you mean with add the table name? The error is in ajax_list_info, and I'm using your function. I can't use unique field names, because the table is referencing itself... The company has a headquarter, which is also a company!

Doctor-Paul commented 8 years ago

My code doesn't address the self-referencing problem, which I think is a known issue with GC. I suggest that you check out the GC forum - for example this post addresses the problem and proposes changes to two GC functions to fix it (you need to scroll down a bit to find the proposed code):

http://www.grocerycrud.com/forums/topic/921-ajax-list-info-ambiguous-field-in-search/

Here is the key code:

1- Modify the get_columns method. 2- Modify the showList method.

1.1- Add these lines before the first foreach:

$ci =& get_instance(); $base_table_fields = $ci->db->list_fields($this->basic_db_table);

1.2- Add the next code inner the second foreach:

else { $fields = $base_table_fields; foreach ($fields as $field_name) { if ($field_name == $column) { $new_column = $this->basic_db_table.'.'.$column; $column = $new_column; $this->columns[$col_num] = $new_column; $this->display_as[$column] = ucfirst(strreplace('',' ', $field_name)); } } } 2.1 - Add this foreach after the $data->columns = $this->get_columns();

foreach ($data->columns as $column) { $new_column_name = substr(strstr($column->field_name, "."), 1);

if ( ! empty($new_column_name)) { $column->field_name = $new_column_name; } }

This work for me, Try with a test environment or separate example.

moritzgloeckl commented 8 years ago

I already tried this... It doesn't work unfortunately. Since you wrote the new ajax_list_info, maybe you know where the LEFT JOIN in the select comes from? It isn't even needed in the query for the search, so maybe the easiest solution is to just remove it. Where does this get added or how do I remove it? Thank you

Doctor-Paul commented 8 years ago

I believe this is how GC implements the set_relation() calls - this is to make search work with the "target" table of the set_relation() call. e.g. "company" is stored as an id in another table, but you can get search results for, say, the company name when searching that first table thanks to the JOIN.

GC is built on top of CodeIgniter and makes calls to CI's Query Builder Class, which is where the actual SQL will be generated.

Good luck with drilling down into that!

PS Have you actually tried using unique column names for all the tables in the database? (Or at least for the .name field in different tables) It may just fix the ambiguity problem.

moritzgloeckl commented 8 years ago

Hmm okay, well my other approach would be to give the company where I select from a name. Any guess where this comes from?

Doctor-Paul commented 8 years ago

Don't quite follow I'm afraid, but the answer is likely to be "deep inside CodeIgniter" :-(

scoumbourdis commented 8 years ago

Hello @Doctor-Paul do you mind to send me a pull request of your code change in order to review the changes and maybe merge it with the grocery CRUD master code? If you don't know how to do it please let me know so I can help you with that :)

I think this bug mentioned is fixed at the version 1.5.3 but not sure though!

Thanks Johnny

Doctor-Paul commented 8 years ago

Haven't done this before but I can see the pull request button - I'll give it a go and shout if I need help.

Doctor-Paul commented 8 years ago

OK, I'm shouting :-)

Pull request asks me to compare two versions of the code - can you talk me through the process?

moritzgloeckl commented 8 years ago

@Doctor-Paul I fixed the problem myself, I modified your code and added a $this->get_table() in front of the $search_field variable. Maybe you want to update your code? It works perfectly now for me. (I actually changed it just in the if after the //die(var_dump($srch)); So the new statement in the foreach now looks like this:

$like_where .= '`'.$this->get_table().'`.`'.$search_field."` LIKE '%".$escaped_text."%' OR ";
razorlegacy commented 8 years ago

This message was created automatically by mail delivery software.

A message that you sent could not be delivered to one or more of its recipients. This is a temporary error. The following address(es) deferred:

michaelm.nyc@gmail.com Domain michaelmohammed.com has exceeded the max emails per hour (5/5 (100%)) allowed. Message will be reattempted later

------- This is a copy of the message, including all the headers. ------ Received: from github-smtp2-ext3.iad.github.net ([192.30.252.194]:54650 helo=github-smtp2b-ext-cp1-prd.iad.github.net) by vps.michaelmohammed.com with esmtps (TLSv1.2:DHE-RSA-AES256-GCM-SHA384:256) (Exim 4.86) (envelope-from noreply@github.com) id 1aG8SU-000298-QX for mike@michaelmohammed.com; Mon, 04 Jan 2016 11:58:19 -0500 Date: Mon, 04 Jan 2016 08:57:34 -0800 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=github.com; s=pf2014; t=1451926654; bh=l8Y1GUheAAYlljwcinXeAdg1/lwKxQaa9hNIwq8jLiA=; h=From:Reply-To:To:In-Reply-To:References:Subject:List-ID: List-Archive:List-Post:List-Unsubscribe:From; b=Q5+AuJUeqkH36PZxFd7EKt7Tb3+inE0SP6LnA5/DGWBtUnAez+/5TKxWGmi13IOpG 1kMVJc0VfGxOCb6p/awjoRwRDYSAk7XW8Ah8SOeNecIG1ZkQ0kxbkhWz6/Zb2xmsYV DrTyRpvOvk1fEB2nWpU4b1f/aI0cdzBtss7myv2I= From: Moe notifications@github.com Reply-To: scoumbourdis/grocery-crud reply@reply.github.com To: scoumbourdis/grocery-crud grocery-crud@noreply.github.com Message-ID: scoumbourdis/grocery-crud/issues/341/168733568@github.com In-Reply-To: scoumbourdis/grocery-crud/issues/341@github.com References: scoumbourdis/grocery-crud/issues/341@github.com Subject: Re: [grocery-crud] GroceryCrud search fails if ->where() or ->set_relation() are used - FIXED (#341) Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="--==_mimepart_568aa47e72738_a3a3f95c48432a0270948"; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: list X-GitHub-Sender: m1s73r X-GitHub-Recipient: razorlegacy List-ID: scoumbourdis/grocery-crud List-Archive: https://github.com/scoumbourdis/grocery-crud List-Post: mailto:reply@reply.github.com List-Unsubscribe: mailto:unsub+0000f6993bfa3713d41d7827570ba70a893db7b5f6adcb3592cf0000000112a2667e92a169ce07300f7e@reply.github.com, https://github.com/notifications/unsubscribe/AAD2mShkpnf7ZsbV2Ux8mPgIlSABpUqDks5pWpv-gaJpZM4GvhZd X-Auto-Response-Suppress: All X-GitHub-Recipient-Address: mike@michaelmohammed.com X-Spam-Status: No, score=-4.1 X-Spam-Score: -40 X-Spam-Bar: ---- X-Ham-Report: Spam detection software, running on the system "vps.michaelmohammed.com", has NOT identified this incoming email as spam. The original message has been attached to this so you can view it or label similar future email. If you have any questions, see root\@localhost for details.

Content preview: @Doctor-Paul I fixed the problem myself, I modified your code and added a $this->get_table() in front of the $search_field variable. Maybe you want to update your code? It works perfectly now for me. (I actually changed it just in the if after the //die(var_dump($srch)); So the new statement in the foreach now looks like this: [...]

Content analysis details: (-4.1 points, 5.0 required)

pts rule name description


-5.0 RCVD_IN_DNSWL_HI RBL: Sender listed at http://www.dnswl.org/, high trust [192.30.252.194 listed in list.dnswl.org] -0.0 RCVD_IN_MSPIKE_H4 RBL: Very Good reputation (+4) [192.30.252.194 listed in wl.mailspike.net] 0.0 HTML_MESSAGE BODY: HTML included in message 1.0 HTML_IMAGE_ONLY_16 BODY: HTML: images with 1200-1600 bytes of words -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature -0.0 RCVD_IN_MSPIKE_WL Mailspike good senders X-Spam-Flag: NO

----==_mimepart_568aa47e72738_a3a3f95c48432a0270948 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit

@Doctor-Paul I fixed the problem myself, I modified your code and added a $this->get_table() in front of the $search_field variable. Maybe you want to update your code? It works perfectly now for me. (I actually changed it just in the if after the //die(var_dump($srch)); So the new statement in the foreach now looks like this:

$like_where .= '`'.$this->get_table().'`.`'.$search_field."` LIKE '%".$escaped_text."%' OR ";

Reply to this email directly or view it on GitHub: https://github.com/scoumbourdis/grocery-crud/issues/341#issuecomment-168733568 ----==_mimepart_568aa47e72738_a3a3f95c48432a0270948 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 7bit

@Doctor-Paul I fixed the problem myself, I modified your code and added a $this->get_table() in front of the $search_field variable. Maybe you want to update your code? It works perfectly now for me. (I actually changed it just in the if after the //die(var_dump($srch)); So the new statement in the foreach now looks like this:

$like_where .= '`'.$this->get_table().'`.`'.$search_field."` LIKE '%".$escaped_text."%' OR ";


Reply to this email directly or view it on GitHub.

----==_mimepart_568aa47e72738_a3a3f95c48432a0270948--

Doctor-Paul commented 8 years ago

Excellent!

I will certainly add that to my code, many thanks.

EDIT: This may solve your problem, but I think it will break the search if set_relation() is used to link to another table.

moritzgloeckl commented 8 years ago

Do you have an example where it might break? It seems to work for me!

Doctor-Paul commented 8 years ago

It's just a suspicion at the moment, will try to look into it further although it will be a few days before I can do that.

multimulti commented 8 years ago

@Doctor-Paul I have found one bug in the code in your second post here: When no explicit column ("Search all") is selected in the drop-down right to the search text, the search always returns no results. The expected behavior is to then search in ALL columns and return the results found.

But, again, thank you a lot for your code, apart from that it seems to work perfectly.

darkbe commented 8 years ago

Is there workaround for this "Search all" problem?

darkbe commented 8 years ago

I am using the version 1.5.4 of GC and ci 3.0.4. with original "employees_management" example. I set $crud->where('employees.officeCode', '1');

I use "Search all" option to "mu" string, i get mysql error: Unknown column 'jd29d42cf.city' in 'where clause' - Invalid query

Sql is: SELECT * FROM employees LEFT JOIN offices as jd29d42cf ON jd29d42cf.officeCode = employees.officeCode WHERE employees.officeCode = '1' AND (lastName LIKE '%mu%' OR firstName LIKE '%mu%' OR extension LIKE '%mu%' OR email LIKE '%mu%' OR jd29d42cf.city LIKE '%mu%' OR file_url LIKE '%mu%' OR jobTitle LIKE '%mu%') HAVING employees.officeCode = '1'

Blair2004 commented 7 years ago

Hi, i may have a clue for this. Actually, when you have many table connected together (relation), proceeding a search, i'll use temp relation table and all col name of the main table defined with "set_table". So to have this fixed, it's required for all thoses col name to be prefixed with the main table name like this "exemple.ID" if ID is the ambiguous field.

First thing to have it fixed, create a new method on Grocery_Crud_Model.php

public function get_table_name() { return $this->table_name; }

Then, edit the Grocery_Crud.php library file and reach the method "set_ajax_list_queries". Within this method, to the last "else" statement, change this :

                    if(array_search($column->field_name, $actual_columns) === false) {
                        continue;
                    }
                    $this->or_like( $column->field_name, $search_text);

Into this :

$table_name = $this->basic_model->get_table_name(); if(array_search($column->field_name, $actual_columns) === false) { continue; } $this->or_like( $table_name . '.' . $column->field_name, $search_text);

Hope it will help.

mckaygerhard commented 7 years ago

the title are market as "FIXED" well this break the preview of the images! this patch are incomplete and buggy

mnadeemijaz commented 7 years ago

grocery crude not working with search all at the time of using where like $crud->set_table('members'); $crud->set_relation('session_id','club_sessions','title',array('club_id' => $this->session->userdata('club_id'))); $crud->where('members.club_id',$this->session->userdata('club_id')); if i select any field name then it is working well but if i use search filter then it is not working i also change the above code of GC but..... please help me to solve this problem

sephinah commented 7 years ago

Hi pips! im having the same problem with i would like to know how to fix for the search box / and search filter for columns im having set->relation_n_n table...

where should i start

please reply.

mckaygerhard commented 7 years ago

the patch provided in this issue works, but break the image previews!

scoumbourdis commented 7 years ago

Cancel Sent by Android Phone

On 19 Jul 2017 5:02 pm, "PICCORO Lenz McKAY" notifications@github.com wrote:

the patch provided in this issue works, but break the image previews!

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/scoumbourdis/grocery-crud/issues/341#issuecomment-316397328, or mute the thread https://github.com/notifications/unsubscribe-auth/AAuS8_qdO8wy9vM9EOupFnOF_Fao5j_dks5sPgxYgaJpZM4GvhZd .