Earlier today, I posted a brief tweet (isn't that redundant?) about return values in PHP (or really, any language). Originally it was about return values from functions (such an exciting topic, I know), but it ended up generating a fair bit of lively conversation, as well as a patch against Drupal 8. So lively, in fact, that I think it deserves more than 140 characters.
New rule:
If your function returns a collection, its null value return must also be a collection.
The original question is one of predictability: If you have a function that return an array of things, and there are no things to return, you should return an empty array. Not false, not NULL
, an empty array. Why? Because I want to foreach()
over it. Consider the following code snippets:
<?php
$items = get_items();
if ($items) {
foreach ($items as $item) {
$item->doWhatever();
}
}
?>
Because get_items()
may return NULL
or FALSE
(doesn't much matter here) if there are no items to return we need to check for a non-array value or the foreach()
will complain at us (rightly so) for passing it a non-array value. Now consider what happens if the empty-value return from get_items()
is an empty array:
<?php
$items = get_items();
foreach ($items as $item) {
$item->doWhatever();
}
?>
Much simpler. And we can simplify it even further in a number of ways:
<?php
// Easy.
foreach (get_items() as $item) {
$item->doWhatever();
}
// Slick
array_map(function($item) { $item->doWhatever(); }, get_items());
?>
"But wait!" you cry. "I want to do something different if there's no values, not just skip stuff!"
Sure. And in PHP, an empty array evaluates to boolean false. So if you want an if-else case, you can still have it, just as before:
<?php
$items = get_items();
if ($items) {
foreach ($items as $item) {
$item->doWhatever();
}
}
else {
print "Where did all my items go?";
}
?>
"But wait!" you cry. "Not having items is an error condition! array() doesn't trigger an error."
Well first, if you're doing basic error handling then the if-else check above still works exactly the same. Second, if it's truely an error condition you have no business using a magic return value for it. Throw an exception. That's what they're there for.
"But wait!" you are still crying. "I'm returning a single record, not a collection of things, so an empty array makes no sense."
Ah, now we have a valid objection, and the key point of the rule above. PHP's loose typing and super-flexible arrays hide this point, but it is key. Just because something is an array data structure doesn't mean it's a collection. In PHP, we have to distinguish between a collection and a struct ourselves.
Specifically, in PHP something is "a collection" if it is traversable. "Traversable" is PHP speak for, roughly, "I can stick it into a foreach()
and the engine won't whine at me." Arrays are traversable, but so are many objects if they implement an Iterator interface of some kind. Arrays are also various other things that objects may or may not be. That is, again, an important distinction.
If your function returns a traversable, it must always return a traversable. If your function returns something that I can foreach(), it must always return something I can foreach. In the majority of cases this means the same object type regardless of whether it contains data or not; collections can be empty and still be collections.
So if you are returning a traversable set of database records, the "no records found" case (which is totally legitimate) should be a traversable but empty set of database records. "Oops, the database broke" is not cause to return NULL
; it's cause to throw an exception. Don't break your contract: If you say you will return a traversable, return a traversable.
"But my PHPDoc says it will return NULL if nothing is found, so I'm just following my contract."
If you wrote such a contract, hire a lawyer next time you go to write an API because you suck at writing contracts. Contracts in code should make life predictable for all parties involved. "I may give you an array, or maybe NULL
, or maybe FALSE
" is not predictable. It's a stupid contract. You get one return type. That's it.
And if your contract says it returns not an array but an object collection (either one of those in SPL or one of your own), an empty collection is still correct. Any mass-operations on that collection should still work, and become no-ops. That is, the following should give no errors:
<?php
get_things()->manipulateAll()->save();
?>
If I care that get_things() returned an empty set, I'll tell you. Or rather, I'll ask you using the Countable interface, which you should implement.
"But wait!" you are still crying. "That's for collections. What about normal objects, or array-structs?"
(You're such a cry baby...)
First of all, that may or may not be an error condition. If it's an error condition, you should be throwing an exception so the question is moot. If it's not an error condition, you need to consider what you could return instead. NULL
is an option; NULL
in PHP is a value that indicates the explicit absence of a value. (Oh, PHP...) If you want to specify the explicit absence of a value, that's NULL
. It is not FALSE
. FALSE
is a boolean. It is a value, specifically the opposite of TRUE
. It is not the absence of a value of some other type. If a function can return FALSE
, then the only other thing it should be able to return is TRUE
(or NULL
, in weird cases). If you can return a non-boolean, then you must never return a boolean.
Of course, don't get too attached to that NULL
. NULL
may indicate the absence of value, but it's also a PITA to deal with. Consider the chain of method calls above. What happens if maniplateAll()
returns NULL
? That's right, fatal error. Please don't cause fatal errors. There may be appropriate cases for NULL. But consider what you could do instead that would not force me to throw is_null()
around my code, because throwing is_null()
around my code makes me sad. NULL
should be the return value of last resort, because it means nothing. (Literally.)
In short: Write good contracts. Obey your contracts. You get one, and only one, return type. Don't make me do extra work because you wrote a dumb contract.
Missing out
I've been 27 months in drupal 7 and I had never seen field_get_items().
As you say - not used in core very much!
Great post
I always get frustrated when people chain methods together that can't be chained together because nulls or falses are being returned somewhere along the way. I know I've done it many times myself as well. Thanks the reminder.
Re: Great post
Luke, are you frustrated because people are chaining things together, or because sometimes people return NULL/false when an object is expected?
Personally, I think chaining can be a good or a bad thing, depending on the context. For something like a query builder, which will probably have a fluent interface, chaining is fine. However, chaining isn't fine if it's used to 'reach into dependancies'. If you find yourself doing $blogPost->getCommentModel()->getRepository()->add($newComment), you should probably refactor so you can do $blogPost->addComment($newComment) instead. (This is known as the 'Law of Demeter', http://en.wikipedia.org/wiki/Law_of_Demeter)
That's a good question. I've
That's a good question. I've actually gone back and forth on that one. I like working with JQuery which returns a valid jQuery object that can be further filtered, refined, manipulated, etc. I don't like getting too deep into dependancies, as you mentioned, but if I'm honest, sometimes I'm just lazy and drill down to what I need. What I get most frustrated about is when things are chained that simply can not be chained based on inconsistent return values (as described in the post). If 1 out of every 1,000 situations can return a null instead of an object, that makes me a sad programmer ever so often.
Yes!
I agree with you so much.
Learned something today
thanks for teaching me something today; I was already returning empty arrays if there were no values found instead of returning Null or False, *but* I was returning False in case of an error. I have come across too much code that returned False in both cases and you have no way of knowing there was a problem with the function or if it had no results in your calling code.
I will change that now to throwing errors and always returning the array (or other iteratable object).
PHP 4 think
A lot of PHP developers developed a lot of bad habits from PHP 4, which lacked working OO, lacked exceptions, etc. Once you have those tools available, it's a whole other ball game but most people who cut their teeth on PHP 4 have a hard time switching. And of course there's hundreds of PHP 4-era tutorials and "best practices" floating around that tend to keep themselves going. Someone today whose first encounter with PHP is a PHP 4.2-era tutorial, or a mentor who learned PHP in the 4.1 era and never updated, is going to learn how to code in PHP 4, not PHP 5.
It's an uphill battle to bring those tends of thousands of PHP 4 developers into the modern era, one that, unfortunately, the core PHP community has historically done a poor job of.
Exceptions
The problem of a lot of PHP programmers is that they don't like exceptions. It's something magic for them and it comes to a return values like array if everything is ok (i mean one entity array) or null (if nothing found... this is ok in this case), but... if an error happens then they return negative integer...
That's where it's get really weird as you know that null can be returned you simply check:
if ($returnedValue) {
echo "then do something";
}
What happens is that negative number in if statement gets treated as a true and then somethings happens... typical a fatal error.
This is great article and there should be more of these.
Worth more than 140 characters
Thanks for translating 140 characters in such an entertaining piece of prose ;-) If one doesn't get it now, one never will.
You could just do
You could just do this:
<?php
foreach ((array) get_items() as $item) {
$item->doWhatever();
}
?>
Casting to an array will convert null/false to an empty array.
Cast to array
You could, but if your function's documentation (it is documented, right?) says it returns an array (or Traversable) then that's what it should do. Casting to array is still working around a problem that shouldn't exist.
Is that so?
That weird, because what I get is:
php > var_dump((array) false, (array) null);
array(1) {
[0] =>
bool(false)
}
array(0) {
}
null gets converted into empty array, but false contains false as its value.
There're plenty of tutorials
There're plenty of tutorials out there that teach you this or that part of PHP, but barely any that teach you stuff like this, so thanks for taking the time to write this up! It helps immensely to know to 1. keep this in mind and 2. why I should keep this in mind.
I now understand why so many non-PHP programmers complain about PHP as a language. Aside from its horrid naming conventions, it pretty much constantly sins against the return type contract.
If a function is getting a
If a function is getting a collection and returning an array but also returning NULL/FALSE (it's documented right?), why would you write code that assumes it's an array? Your code should handle all the possible return values of a function.
This post really should be about telling people who code in PHP to use exceptions instead of NULL/FALSE for returning an error.
You had me at hello, but ...
You lost me at
// Slick
. I'm thinkin' that should be// Inscrutable
. ; )LOL
It's rather compressed, but that's just using array_map() on an array, where the callback is an inlined anonymous function. Generally you wouldn't do that for a one liner like this, but in some cases it can be useful. You'd probably format it a bit bitter in those cases. ;-)
Exactly why PHP interfaces are pointless for contracts
This is a prime example of why interfaces in PHP are 99% useless as a contract. They don't define a return type; which is imperative in precisely these situations.
PHP 7 :-)
Just a historical note that, since this post/comment was written, PHP 7 now supports return types on all functions and methods. Everyone should be using them always, for exactly the reasons described here.
I'm sick to death of using
I'm sick to death of using is_array or some other condition. Sometimes I missed typed languages, but tbh the flexibility of PHP arrays is a godsend at times.
Great post, if this was adopted more widely, life would at times be much easier.