PHP provides a decent model of class member visibility, with public, private, and protected members to help you define tight APIs for your objects and show other developers how your object is supposed to be used. But used naively, PHP’s ‘magic methods’ can easily and subtly subvert this system, making everything public.
If you’re still new to object oriented programming in PHP5, think of “public” as roughly analogous to “my function’s arguments” and “private” as “local variables inside the function”. You wouldn’t want someone calling your function and messing with the local vars, and you wouldn’t want someone using your object messing with its private members.
Magic methods provide functionality like catching references to methods and properties which are not visible to us, and doing special things with them. Magic methods have always struck me as a bit weird, and whenever you bring them up in discussions online, there’s always a few people with reservations about them – efficiency, clarity, use-cases and so on.
I’m still in two minds; they can be useful in some circumstances, but here’s one reason why they could be considered harmful: Used carelessly, they can easily enable an OOP antipattern where all class members become public, even those declared as private or protected in the class definition.
This is really very bad indeed. with just a few lines of seemingly simple __call magic, you can invalidate your object’s internal scope and expose parts of your object which are not intended to be exposed. This also makes it easy for other people to unintentionally abuse your object later on, and paves the way for all kinds of headaches down the road. It’s also easy to do by accident when playing with magic methods.
Here’s the offending code. I call it ‘the most exquisitely hideous bit of PHP I’ve ever written’:
Looks pretty simple. Kinda redundant maybe, but not really harmful, right? It’s about the worst, most dangerous piece of code I’ve ever written. Let’s take a look at how it destroys the PHP object model.
The __call method is executed whenever you try to call a method which is not visible or is undefined. It can be useful for a few obscure cases, like providing default getters and setters, or doing magic things like java-esque overloading in PHP.
Inside that method, we check if the method we were trying to call exists in the class. If it does, we execute it.
Now you should be saying: Hang on a second, __call is executed if the method doesn’t exist in the class. And then we check to see if the method exists in the class… Surely the method won’t exist, because if it did, __call wouldn’t have been called, right?
Wrong. The problem is one of scope. You already know about scope; remember those local variables inside a function? They’re not accessible to the outside world because they fall outside the scope of the rest of the script.
When we define a member as ‘public’, we’re saying “let this member be visible to the global scope”. When we define a member as ‘private’ we’re saying “let this member be visible only to ourselves”, and when we say ‘protected’ we mean “let this member be visible only to ourselves and classes which extend us”.
Let’s augment that Foo class with a private method, which shouldn’t be visible to the outside world:
Now from outside that class, if we try to say $foo->bar(); it should fail, yeah? Because it’s private, not visible to the external scope. And without that __call magic, that’s exactly what would happen.
But remember what __call does – it gets fired when we try to access something which is not visible to us. bar is not visible, so __call gets fired.
Then the code inside __call checks to see if bar exists within Foo. Because __call is a member of Foo, it has access to the internal scope, so bar *is* visible from __call, and method_exists returns true. Then we execute it using call_user_func_array, and that’s how we poke holes in privacy in PHP. We declared a method as private, added some __call magic, and now it’s public. The same goes for private and protected properties with __get and __set too.
You might think that this is a somewhat academic problem – that no-one would write code like that in the real world, at least not intentionally. But on a google search for “__call call_user_func_array” there are 4 examples on the first page alone which unwittingly fall into this trap. Even more hits on koders.com. It’s really, really easy to do, because combining __call and call_user_func_array like this is a really obvious ‘clever’ way to use them. It’s just subtly dangerous too. Be warned!
Oh, and here’s an added bonus.When another object extends the one with the clever __call trick, the same magic function will mean that the child class is exposed in the same way, all its protected methods become public. Ouch.
Yay for openness. Finally:
Again, all protected methods from AnotherClass are now public. Yep, that little __call method can totally wreck the OO model in PHP if not used with extreme caution.
There was some great discusson when I submitted this to reddit a few months back – check out the comment thread. I decided it was worth a blog post of its own when I was writing my post on method polymorphism in PHP and nearly made the same mistake!
#1 by zaadjis on November 1, 2010 - 11:30 am
In this case, method_exists is the wrong tool for the job. Use http://php.net/reflection instead (you did know that PHP has a reflection API, right?):
$rm = new ReflectionMethod(__CLASS__, $func);
if ($rm->isPublic()) return call_user_func_array(array($this, $func), $args);
complete example: http://gist.github.com/658712
#2 by Howard Yeend on November 1, 2010 - 3:50 pm
nice. Yeah I meant to add a section to this post explaining how to do it properly. Now I don’t need to!
#3 by Theodore R. Smith on November 1, 2010 - 5:32 pm
But… what is it possibly useful for?!
#4 by Howard Yeend on November 2, 2010 - 1:00 am
I was trying to produce a generic getter/setter. If a method is not defined, we can define some default action using __call. If we later want to build a custom getter, we simply define the method explicitly.
Pseudocode:
function __call($method, $arguments){
$methodPrefix = first 3 chars of $method
$method = substr($method,3);
if $methodPrefix = "get" {
return $this->data[$method];
} else {
$this->data[$method] = $arguments;
}
}
Now I can call getFoo and setFoo and they’ll get and set elements in my object’s data array even though they’re not defined, If I later want to override the default getter/setter provided by my __call code, I simply need to explicitly define the method.
That’s just a simple use case. It’s debatable whether you gain anything from this. I personally lean towards the “it’s a cool trick but really you’re just being lazy – define the getters and setters explicitly and enjoy clearer code” camp.
#5 by aeetos on November 12, 2010 - 7:02 am
I have managed product development on a very large PHP codebase for a few years now, and I can say with certainty magic getters, setters, and this magic call method can wreak havoc with your quest to build maintainable code. Our team cowers in fear whenever we find a business-critical class that extends from “Base”, which is an abstract class in our codebase that implements all this goodness and more.
Aside from the technical problems you’ve mentioned, here’s another one that’s a bit more subtle: magic methods like this hurt the self-documenting nature of your code, especially when coupled with a good document commentator (think PHPDoc) and a good IDE. This may take me two minutes to write, but it will save my company and my team hours of pain:
/**
* Get the customer’s first name.
*
* @return string the customer’s first name
*/
public function getFirstName() {
return $this->first_name;
}
Now, if I’m in an editor like Komodo, and I type this:
$customer = new Customer();
$customer->
Up pops a menu that shows my getFirstName() method along with the description of what it does.
#6 by Montreal Web Design on January 13, 2011 - 12:10 am
Visibility is more of a documentation, and for api interface. It is not really to prevent you from calling private methods.