DomainObject::one should return false instead of throwing an exception if none found?

[Jud]

I am writing lots of logic that involves checking to see if an entry exists in the db, and if it doesn't, create a new entry. In my case, there is a unique key on the two fields I am querying on, so the finder will always return 1 or 0 results.

::one seems like the way to go for this:

public static function findOrCreateWithOrderAndProduct($order, $product){
  if(!($entry = static::one('orders_id = ? and products_id = ?', $order, $product))){
    $entry = new self;
    $entry->orders_id = $order;
    $entry->save();
  }
  return $entry;
}

The above code doesn't work if there are not matching entries found because ::one expects a single item in the collection. I'm sure there is a reason for this, but to me, it seems like you could allow 0 or 1, and assert on > 1 returned entries. I might be missing a case where, when returning 0 entries, it would be a good thing to assert().

[bjornpost]

I think the exception is good. If you're unsure about how many items will be returned, you should use find instead. In your example, you could wrap the static::one() call in a try/catch statement.

[Jud]

@bjornpost I could wrap it in a try/catch, but can you think of an example where throwing an exception when zero rows are returned is a good thing?

Or another way, when would you use ::one (without checking existence) when you couldn't use ::byId?

Maybe @lox could shed some light.

[mtibben]

I tend to agree. Exceptions should be used for exceptional circumstances, but using an exception here leads to needing to use exceptions for flow control.

I would be in favour of having 2 methods - perhaps a ::one method that returns null and an ::assertOne method that throws an exception

Not sure that would be great for backwards compatibility though, and you'd want the behaviour to be consistent across all finder methods.

[harto]

How about a new ::exists() method that returns true for >0 rows?

[Jud]

I agree that backwards compatibility would be an issue with this change - but the behavior could be deprecated with a warning and changed in v2.0.

I can implement my own one method on top of DomainObject like:

public static function one(){
  try{
    return call_user_func_array(array('parent', 'one'), func_get_args());
  } catch(Exception $e) {
    return null;
  }
}

But I wanted to post this because there is not an easy way to query for DomainObjects that are unique key constrained on multiple keys, without having to find a collection of items, check the count, grab the first. For example (my earlier example, but with find):

public static function findOrCreateWithOrderAndProduct($order, $product){
  $entry = static::find('orders_id = ? and products_id = ?', $order, $product);
  if(!$entry->count()){
    $entry = new self;
    $entry->orders_id = $order;
    $entry->save();
  } else {
    $entry = $order_product->first();
  }

  return $entry;
}

Which is 3 extra lines of code every time want to find an object that I know only has 0 or 1 elements.

[Jud]

::exists is close, but it feels like extra work for something that can be inferred by the next action I am going to take. Not to mention that without caching I would make two db queries instead of one.

There are cases where php tends to get in the way of code being succinct (e.g. the inability to have objects evaluate to false when cast as a bool, so you end up doing lots of if(count($collection)) or if($collection->count())'s). But I think this is a case where the majority of use cases could become more succinct, and a minority of use cases would write an assertOne wrapper if the really needed to make sure there was only one returned row.

Anyway, its no big deal to me - I'm sure @lox knows the direction he wants to take the project. I can write a wrapper either way.

[lox]

My philosophy on one() is that you are making an assertion that there is exactly one result and you want that single result. Failure to meet this contract should generate an exception. Where I can I avoid returning a mix of objects and booleans, it makes for unpredictable API's, IMO.

Were you aware of the Collection::orCreate() method? It's designed to solve this problem. I should document it :)

<?php

$entry = Entry::findByOrderIdAndProductId($order, $product)
   ->orCreate(array('order_id'=>$order))->one();

Does that help?

The pull request that the orCreate happening in was here: https://github.com/lox/pheasant/pull/45

[Jud]

@lox agreed on keeping the API consistent. It seems like Pheasant returns an object for pretty much all methods, so starting to return booleans, even if consistent across all finders would seem a bit out of place.

::orCreate() accomplishes what I need, I'm surprised I missed it.