1
\$\begingroup\$

I was just wondering if there was a better way to write this block of code in php. I have a string with values separated by commas. I need to prepare that string to pass it on to the query using IN clause.

 $sql = "SELECT domain, items FROM rep_skills WHERE report_id = $id ";
 $data = $conn->query($sql)->fetch_object();

 while($data){
     // can this be done in one line? 
     $array =  explode(",",$data->items); // string here is 39,40,41
     $items = implode("','",$array);  // string here is 39','40','41
     // end of block

     $result = $conn->query("SELECT description 
           FROM ".$tables[$data->domain]." 
           WHERE item_id IN ('".$items."')");

     $description = '';
     while($row = $result->fetch_object()){
        $details .= '- '.$row->description;
    }
}
\$\endgroup\$
7
  • 1
    \$\begingroup\$ Where does $records come from? How can you be certain that it contains only comma-separated numbers? \$\endgroup\$ Sep 27 '18 at 22:01
  • \$\begingroup\$ pulled from database. 100% certain. Data is validated before input. \$\endgroup\$
    – joanb
    Sep 27 '18 at 22:16
  • \$\begingroup\$ In that case, why not write a JOIN query instead? \$\endgroup\$ Sep 27 '18 at 22:18
  • \$\begingroup\$ How would that be? i'll update the full code so you can see \$\endgroup\$
    – joanb
    Sep 27 '18 at 22:33
  • 1
    \$\begingroup\$ Oh, that is a very weird database schema: there's a separate table for each domain, and the items are stored as comma-separated lists rather than as a table. The bad schema design is forcing you to write bad code to query it. \$\endgroup\$ Sep 27 '18 at 23:27
1
\$\begingroup\$

Not withstanding what other have mentioned.

$array =  explode(",",$data->items); // string here is 39,40,41
$items = implode("','",$array);  // string here is 39','40','41

You can simply replace the comas with ',', Like this

$items = str_replace(',', "','", $data->items);

One thing to note is you should never concatenate data into SQL, because it opens the possibility of SqlInjection. There is maybe 3 points that you are open to that. Even if you think the data is safe, you should still be using prepared statements. You never know when code will need to be changed and by not doing it, later you may change where this data comes from and allow user input into those variables. Sometimes, even, this data could be set in a place far removed from your query.

Just my thoughts.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the input! I am learning about prepared statements and starting to implement those in my projects now. Thank you again! \$\endgroup\$
    – joanb
    Oct 2 '18 at 15:40
1
\$\begingroup\$

It's a very weird database schema in the first place.

There should be never a thing like comma-separated values in one cell. Learn the database normalization and then create a link table wghich can be used to get all your records in a single query with JOIN.

There should be never a thing like multiple tables of the same structure. there should be only one table with a field to distinguish the kind of domain.

\$\endgroup\$
1
  • \$\begingroup\$ OK I get the one table. There are 7 domains so table can go this table name -> lookup_descriptions, columns -> id, item_id, description, domain; then lookup based on domain. the dropdown is a multi select. How do I go about not storing comma-separate values \$\endgroup\$
    – joanb
    Sep 28 '18 at 17:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.