3

This question already has an answer here:

I'm going to build some public PHP packages, Following standards is a priority for me.

PHP lets users call methods even if they don't pass required parameters to it.

My question. Should I check parameter before using it in methods? (like following example)

public function setVariable($value)
{
    if(isset($value))
        throw new \InvalidArgumentException("Value must be an int value!");
    $this->variable = (int) $value;
}

Or just do my job!

public function setVariable($value)
{
    $this->variable = (int) $value;
}

marked as duplicate by JeffO, durron597, gnat, user40980, GlenH7 Jun 16 '15 at 21:32

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

  • 1
    Nitpick: Your empty($value) check doesn't make sense. It will pass for anything that isn't empty (eg Hello world), but will fail for 0, which is an int value. How about using is_int() instead? – yannis Jun 15 '15 at 12:57
  • "PHP lets users call methods even if they don't pass required parameters to it." - Not really, you get an E_WARNING! – MrWhite Jun 15 '15 at 13:29
  • In your first example, you've changed empty($value) to isset() and in doing so reversed the logic - this is definitely wrong in this context! As @Yannis suggests, for a required parameter you only need to check is_int(). Or in your case !is_int(). – MrWhite Jun 15 '15 at 20:06
  • consider my question more generally. it's not exactly about int, if it was is I would ask in Stack-Overflow. – Milad Jun 15 '15 at 20:14
  • 1
    I've removed the question about type hinting in php as that is a completely different question than the one asked here. I also removed the opinion and request for discussion. Please read the help center and avoid asking questions that are calls for discussion (“I’m curious if other people feel like I do.”) or rants in disguise (“PHP sucks, do you agree?”) – user40980 Jun 16 '15 at 0:40
9

I'm going to build some public public PHP packages.

Public as in "Open Source", "public to the whole world"? Then do yourself a favor and do as much validation as you can as long as you do not suffer serious performance problems. Even when you provide some documentation saying "this function does not handle left out parameters", expect a lot of people completely ignoring your package's documentation not reading all the gory details of your documentation in full. And if a problem occurs, the error message should users give a clear description of what they did wrong, otherwise they will blame your package, might blame you, and send you dozens of support mails.

So I recommend not only to throw an exception, but to make your the error messages in those exceptions are as clear as possible.

2

As of PHP5, type hinting is a thing. The most important distinctions are the following:

  • You cannot type hint scalar/numerical, string, Resource, or Trait data.
  • You can force parameters that are objects, arrays, or anonymous functions.
  • You can type hint using interface names, class names, object keyword, array keyword, or callable keyword (for anonymous functions/callbacks).
  • When a type hinted function or method is called with mismatched parameters, a catchable fatal error will occur (not an Exception).
  • If you specify null as a default value, type hinting is somewhat useless.

I would suggest at least checking isset and is_null before running your algorithm, and I highly suggest further type checking with things like is_string, is_int, is_float, etc. Type casting is also valid. Edit: this only applies for when you cannot use type hinting for a function, e.g. scalar parameters.

Returning false or throwing an Exception are all context specific questions -- you have to determine which fits each particular function/algorithm in your package, which will include considering how someone using your code should interact with it. My favorite example of contrived Exception throwing is Sentry. I mean, look at all of those catch blocks. Maybe you'll want your code to throw that many things to provide different information, or perhaps you can return a boolean, or perhaps you can throw a new Exception("Message text"). It's a personal choice for each piece of code. I recommend looking at potential applications where your package will be useful and see how well your package would integrate with those coding styles.

tl;dr version

The only primitives you can type hint are array and object. You can type cast or type check other primitives. Function/method calls with parameters that fail to satisfy type hints results in a catchable fatal error. What to do when parameters sent to your function/method are mismatched is a personal, context specific choice that nobody can definitively answer without seeing your code.

Edit: see also https://stackoverflow.com/a/5724696/2103394

  • Why check "isset and is_null"? – MrWhite Jun 15 '15 at 17:37
  • For scalar variables or if there is some other reason to not use type hinting. I'll make that more clear. – Jonathan Voss Jun 15 '15 at 17:40
  • you mean, we should use isset() before is_string (4example)? – Milad Jun 15 '15 at 19:20
0

I try to follow the following:

For externally-called code, do as much explicit error handling as possible, to provide the best experience for the caller and highest level of protection for your own code against inbound data that might partially succeed in unintended ways.

For private calls, I'd explicitly handle bad data if it was possible for that data to somehow get through the call without failing violently on its own. So, I guess I'd call this defensive trapping.

It seems to me that API writers should try to handle invalid data and provide great error messages on all external calls, both to help the user and to help yourself when you invariable have to support some production issue.

Not the answer you're looking for? Browse other questions tagged or ask your own question.