Blog: Don't Do This At Home #3: Terminated Query Methods
Can you spot the problem in the following snippet?
Firstly, this is a "termination method", which means that it doesn't return the query. Every termination method should take the connection as argument, otherwise you can't guarantee transactional integrity.
Secondly, this method hydrates all the Book entities satifying the queries, but just needs the first one. That's why you should use findOne() instead of find()->getFirst(), since findOne() includes a call to limit(1).
Update: There is a good reason why this just can't work (see the comments below). The following advice is therefore irrelevant, but is left published to testify that sometimes, code review is hard.
So the method should look like:
But there is more. As already mentioned in this blog, you should never include the termination method in a custom query method. What if you need to count the number of books satisfying the filters instead of returning the first result? You would have to refactor the filter part into another method, so let's do that in the first place:
Does this oblige you to write the call to findOne() manually? No, because ModelCriteria::__call() magically proxies a call from findOneByXXX($value, $con) to filterByXXX($value)->findOne($con). So the following still works:
Finally, Id is actually the primary key of the book table. findPk() is more efficient than findOne(), because it takes advantage of the instance pooling and of the recent SQL optimizations introduced in Propel 1.6.3. So the custom query method should be reduced to:
And used as follows:
Some purists would argue that the controller code should be as small as possible. My opinion is that it's not longer to write ->withPublishedComment()->findPk($id, $con) than ->findOneByIdWithPublishedComment($id, $con), and it is more readable. But it comes down to a matter of coding style.