Chris VandenHeuvel Fri Aug 27 14:40:59 -0400 2010

Subject: complex conditions

How do you form a finder call when the number of conditions can vary?

For example, let's say I'm building a search function for articles. You can search by author, category, and keyword. If any one of those is not specified in the search, then obviously I won't want it in the WHERE clause.

I'm really struggling to think of a way to make this work with the phpAR finder. Normally I'd start with an empty WHERE clause and just add pieces to it as needed. But the problem comes in when using phpAR's "prepared queries" where you pass in the sql and the variables separately. At first I'd hoped this would work:

if ($_GET['category_id']) {
    $clause .= $glue . ' category_id = ?';
    $cond_values[] = $_GET['category_id'];
    $glue = ' AND';
  }
  if ($_GET['author']) {
    $clause .= $glue . ' author like %?%';
    $cond_values[] = $_GET['author'];
    $glue = ' AND';
  }
  if ($_GET['keyword']) {
    $clause .= $glue . ' keywords like %?%';
    $cond_values[] = $_GET['keyword'];
    $glue = ' AND';
  }

$item = Item::all(array('conditions' => array($clause, $cond_values)));

In other words, pass in one single array for all of the substituted values in the clause. I'd hoped that the function would realize that there weren't enough values to cover the conditions, and look inside the array I'd passed in. Obviously it was not designed to work this way, as it just spits out a fatal error.

I'm guessing this is really easy and I'm just missing it?

Thanks.


Kien La Mon Aug 30 18:54:26 -0400 2010

I'm not seeing why your code wouldn't work. You are adding one value for every ? so that should be ok. Could you paste the error and the data in the $_GET array that causes it to fail?

Chris VandenHeuvel Mon Nov 01 16:06:51 -0400 2010

Sorry ... I wanted to get this out of my project and into a more isolated testing setup before I responded, and I never got around to it.

Anyway, it should be more clear in this updated example.

The following code:

$clause = '';
$glue = '';
$cond_values = array();
$query = array(
  'style' => 'cat',
  'color' => 'orange'
);
if (!empty($query['style'])) {
  $clause .= $glue . ' style = ?';
  $cond_values[] = $query['style'];
  $glue = ' AND';
}
if (!empty($query['color'])) {
  $clause .= $glue . ' color LIKE %?%';
  $cond_values[] = $query['color'];
  $glue = ' AND';
}
if (!empty($query['size'])) {
  $clause .= $glue . ' size LIKE %?%';
  $cond_values[] = $query['size'];
  $glue = ' AND';
}
$animals = Animal::all(array('conditions' => array($clause, $cond_values)));

Outputs this:

Fatal error: Uncaught exception 'ActiveRecord\ExpressionsException' with message 'No bound parameter for index 1' in /var/www/shared/test/lib/php-activerecord/lib/Expressions.php:111 Stack trace: #0 /var/www/shared/test/lib/php-activerecord/lib/SQLBuilder.php(310): ActiveRecord\Expressions->to_s() #1 /var/www/shared/test/lib/php-activerecord/lib/SQLBuilder.php(97): ActiveRecord\SQLBuilder->apply_where_conditions(Array) #2 [internal function]: ActiveRecord\SQLBuilder->where(' style = ? AND ...', Array) #3 /var/www/shared/test/lib/php-activerecord/lib/Table.php(164): call_user_func_array(Array, Array) #4 /var/www/shared/test/lib/php-activerecord/lib/Table.php(195): ActiveRecord\Table->options_to_sql(Array) #5 /var/www/shared/test/lib/php-activerecord/lib/Model.php(1428): ActiveRecord\Table->find(Array) #6 [internal function]: ActiveRecord\Model::find('all', Array) #7 /var/www/shared/test/lib/php-activerecord/lib/Model.php(1256): call_user_func_array('static::find', Array) #8 /var/www/crcna.org/datatest/html/ar_test/index.ph in /var/www/shared/test/lib/php-activerecord/lib/Expressions.php on line 111

But it works fine if $query has 0 or 1 items in it.

Edit: forgot the line of code where I actually call the finder.

Keith Thibodeaux Mon Nov 01 18:20:39 -0400 2010

Shouldn't $glue = "," ?

Chris VandenHeuvel Tue Nov 02 08:32:44 -0400 2010

Keith Thibodeaux wrote:

Shouldn't $glue = "," ?

No, it's building the WHERE clause separated by AND. The error doesn't appear to have anything to do with the actual SQL anyway.

Gabriel Littman Mon Jan 10 19:30:51 -0500 2011

Does it work with array_merge?

$animals = Animal::all(array('conditions' => array_merge($clause, $cond_values)));

should be like Animal::all(array('conditions' => array("this = ? AND that = ?", this, that)));

NOT

Animal::all(array('conditions' => array("this = ? AND that = ?", array(this, that))));

Gabe

Gabriel Littman Mon Jan 10 19:33:05 -0500 2011

maybe better to unshift

unshift($cond_values, $clause)

$animals = Animal::all(array('conditions' => $cond_values));

Stefan W Tue Jan 11 08:22:09 -0500 2011

I believe you have multiple problems with your example code some relevant, some possibly not:

1. When doing a like match in the way that you are I think it will generate "size LIKE \%'orange'\%" when in fact you need the wildcards included in the value. You should add them to your value, not the SQL.

2. If you want to save yourself some trouble use aliases instead of numbers. e.g.

if (!empty($query['size'])) {
$clause .= $glue . ' size LIKE :size';
$cond_values[':size'] = $query['size'];
$glue = ' AND';
}

3. Concatenating strings in this way turns into what we call in the business a " tremendous ball-ache" very quickly. It's much better to build a class that is responsible for building the SQL in a dynamic and reversable way. If you just want it to do conditions it could be quite simple.

Though all that being said I still can't see anything that would obviously cause the error you are getting.

Chris VandenHeuvel Tue Jan 11 11:34:18 -0500 2011

Thanks for the help all.

Stefan W wrote:

1. When doing a like match in the way that you are I think it will generate "size LIKE 'orange'" when in fact you need the wildcards included in the value. You should add them to your value, not the SQL.

[Edit: Totally didn't grasp what you meant by adding it to the value. Seems to work fine, and I can't duplicate the problem I was having where the finder wasn't getting the right results]

Jono B Tue Jan 11 13:19:43 -0500 2011

Maybe some valid points....but its still better than anything else out there at the moment.

And furthermore, since its open source, it requires a community. Kien and Jacques dont have any responsibility in trying to build the community...its up to everyone to chip in and help if they want something better.

Stefan W Tue Jan 11 15:16:22 -0500 2011

Chris,

The place holder-style implementation is known as a prepared statement. It essentially eliminates SQL injection attacks because you're giving the DBMS two things - the "code"/SQL and the values, (rather than a single string that may contain both) and telling it to combine them based on what it knows to be safe. Traditionally people would use something like realescapestring but this can apparently be compromised by things such as inconsistent character encodings between PHP and the database.

Anyway with prepared statements you should always rely on your database to insert the quotation marks, not try and do it yourself.

I would suggest you enable the mysql log and observe what the ORM is doing as you navigate though your application. Doing this will generally show you exactly what is wrong and allows you to copy out statements and run them yourself though workbench or phpmyadmin to see the raw result.

In about 6 months of use I've yet to find any serious issues with the ORM. Its reason'd'etre seems to be that it's easy to use and you really can't fault it on that. It doesn't have all the features of other ORMs but it will have you up and running more quickly and generate only 10% of the code you'd otherwise need (and need to continue to maintain).

Regarding like queries it does indeed support them and I use them frequently. If you wanted you could just use find_by_sql and work almost entierly in SQL. For example:

YourModel::find_by_sql('SELECT * FROM table WHERE title LIKE :value', array(':value'=>'%this is the value%'));

Chris VandenHeuvel Tue Jan 11 17:05:20 -0500 2011

Stefan,
Of course you are right. I didn't even grasp what you meant by adding the wildcards to the value until I saw your example. I got way off track with how I was trying to do this.

I deleted my [first] little rant because it was triggered by a bug that isn't actually there AND a perceived lack of development activity that has now appeared in the sidebar of this site since this morning. (Am I crazy or was the sidebar showing the last commit to be 25 days ago earlier today?)

I still think this project has two major red flags:

Slow development - why have no features or fixes been introduced in a stable release since last June?

Misguided feature focus - Consider this scenario:
Because there's no HABTM, we have to declare many-to-many relationships using 'has_many through.' That means we need to create separate model classes for each association table. On the other hand, we have all the sexy Rails automatic table name guessing that relies on pluralization and conversion from camelcase class names and all that. So, it makes sense to save the programmer from writing the one line it would take to declare a table name in the model, but not save him/her from writing entire needless models for each association?

PHP really needs an "official" AR implementation that has healthy development behind it. This has the potential to be that. But 1.0 releases don't have things like this: (http://www.phpactiverecord.org/boards/4/topics/226-associations-for-lookup-tables) and healthy development doesn't leave something like that sitting for 2 months. This looks like a beta and acts like a beta. So why not admit that and call it one, so that they attract beta testers instead of people wondering if it's right for a real-world project?

(1-11/11)