Nested conditions are powerful in the new database API but complicated to build. It's very common to need to match more than one field. It's fairly common to need compound conditions--(w and x) or (y and z).

With db_insert and db_update we can feed a simple array of items to be inserted or updated. To facilitate nested select queries, we should have similar functionality.

Attached patch adds a simple and flexible helper method, conditionItems(), to the SelectQuery class. This method can be fed simple arrays of values to easily construct nested conditions.

Sample uses:


// Select nodes with status 1 and promoted 1.
$query->conditionItems(array('n.status' => 1, 'n.promoted' => 1));

// Select nodes with status 1 or promoted 1.
// Use the second argument (defaults to 'AND').
$query->conditionItems(array('n.status' => 1, 'n.promoted' => 1), 'OR');

// Select nodes with status 1 or promoted 1 of type page or story.
$query->conditionItems(array('n.status' => 1, 'n.promoted' => 1, 'n.type' => array('page', 'story')));

// Select nodes with status 1 or promoted 1 of type page or story.
// An array of values receives the 'IN' operator.
$query->conditionItems(array('n.status' => 1, 'n.promoted' => 1, 'n.type' => array('page', 'story')));

// Select nodes with status 1 or promoted 1 of type page or story or a status of 0 and type of blog or image.
// Feed an array of condition sets.
$conditions = array(
  array(
    'n.promoted' => 1,
    'n.type' => array('page', 'story'),
  ),
  array(
    'n.status' => 0,
    'n.type' => array('blog', 'image'),
  ),
);
$query->conditionItems($conditions);

By comparison, the last example above currently would require something like:


$or_condition = db_or();
$and_condition = db_and();
$and_condition->condition('n.promoted', 1);
$and_condition->condition('n.type', array('page', 'story'), 'IN');
$or_condition->condition($and_condition);
$and_condition = db_and();
$and_condition->condition('n.status', 0);
$and_condition->condition('n.type', array('blog', 'image'), 'IN');
$or_condition->condition($and_condition);
$query->condition($or_condition);

One place this patch should help simplify code is load functions where we're matching by a combination of fields.

Questions:

* A better name for the method?
* Probably this belongs instead in the DatabaseCondition class?

CommentFileSizeAuthor
select-conditionItems.patch1.75 KBnedjo
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Needs review » Closed (won't fix)

The default is "and", so your examples can be rewritten as:

db_select('table')
 ->condition(db_or()
   ->condition('n.promoted', 1)
   ->condition('n.type', array('page', 'story'), 'IN')
 )
 ->condition(db_or()
   ->condition('n.status', 0)
   ->condition('n.type', array('blog', 'image'), 'IN')
 )
)

So I don't see any use for this patch.

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented
Status: Closed (won't fix) » Needs review

@Damien Tournoud Thanks for the review. Agreed that your version is better than the example I gave.

But I think the potential use case still stands, which is: have a method that simplifies the most common types of nested conditions--and can be called from API functions with mixed data.

The example you gave would indeed work for a particular query. But it would need to be considerably more complex to be used in an API loader function where it could receive a variety of mixed data (e.g., either single values or arrays of values, a single set of data to match or multiple sets).

It might help to explain my process here. I started with the need for locale APIs, including load: #361597: CRUD API for locale source and locale target strings. As I wrote a load function, I found that most of it was very generic. So I drafted a generic loader: #365899: API methods for schema-based load and delete operations.

But in considering that, it seemed that at least some of the loader functionality belongs at the database API level. Maybe almost all or even all of it does. If so, though, we could use a smarter condition adder.

Currently, we have different loaders for various sorts of objects. Looking at node_load_multiple(), for example, we have code there to convert either an array of nids or an array of conditions into condition objects:


    if ($nids) {
      $query->condition('n.nid', $nids, 'IN');
    }
    if ($conditions) {
      foreach ($conditions as $field => $value) {
        $query->condition('n.' . $field, $value);
      }
    }

This allows modules calling the loader to feed a simple array of values to match.

So far so good, though really there's no reason the loader should be limited to a single set of conditions. What if we want to load nodes by the criteria above, e.g. nodes that are either 'page' or 'story'? We could do that by altering the code a bit:

      foreach ($conditions as $field => $value) {
        $query->condition('n.' . $field, $value, is_array($value) ? 'IN' : '=');
      }

Fine. But what if we want some additional complexity, like the other examples I gave? And shouldn't our APIs be flexible enough to be used for more than just the most basic types of queries? We soon get into something like the full function in this patch--and we need to repeat it in each loader we write.

So, again, my point is not that we can't do this with the existing condition() method. Of course we can. It's that we could use a method that is smart enough to handle the most common nested condition types.

Besides reducing code duplication, this could allow us to improve the parallelism between load APIs, so that they all accept criteria data in the same format.

Or, maybe, it could help us eliminitate the need to have a new API for each type of object we want to load.

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Needs review » Needs work

Conditions are centrally managed as part of the DatabaseCondition object. Any changes to conditional handling, such as adding more utility methods, should be done to the conditional interface and then implemented on the conditional object and those query classes that implement the conditional interface. It should not be select-specific.

That said, I'm still not sure I see the use case here. All you're doing is pushing the foreach() loop inside the query object. There's still a foreach loop. It makes sense to me to keep that exposed so you know what it is you're doing, and can vary your behavior accordingly if needed. I still am not seeing the use case that would be so much easier by moving the wrapper inside the query object rather than outside it. (I'm not against doing so in general; that's why we added fields() to SelectQuery. But I don't see the use case here.)

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Subscribing, I'm not entirely sure about the use cases here either though. For most loading functions, you usually know in advance what you're going to load - i.e. the ten IDs returned by a pager query. While node/user load allowed for conditions, these are quite rarely used, and usually only on one thing ($user->name etc.) - which is often on administrative/login etc. pages rather than high traffic ones. For specifics, it's usually better to do a query, then load based on that - and it's very rare that this would add more than one or two queries on a page compared to the current approach.

I think what's probably needed here is a use case in either core or contrib, where we're loading from a table on a regular basis, with something which can't be handled by the 'dumb' conditions array in the existing multiple load functions in core. If there's an obvious candidate, and it'd otherwise stop us re-using code between these, then I'm all for adding it as long as the default case remains simple.

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Needs work » Closed (won't fix)

I'm going to go as far as saying that the DB layer should not be handling every possible edge/use case. The query builders should be building queries, not handling all kinds of business logic and ORM logic. Leave that to another layer.

This issue seems more relevant to standardizing loading mechanisms: #460320: Standarized, pluggable entity loading (nodes, users, taxonomy, files, comments)