Why are my models allowing you to set values for private variables?

Hello,

I have my Users model, and I've set password to be private. Why is it allowing me to update this value using $user->password = value; as if it was public? Is there some magic setter screwing with this?

If I print_r my Users object, I receive this:

Users Object
(
    [password:Users:private] => valueHere
)

As you can see, it's even labeled as private. I don't want outside code updating these variables, which is why they are private.

What is allowing this to take place?

Thanks.



17.9k

Could you provide full code of your model's class?

I imagine your model extends the default Phalcon model? That default model has a magic __get() method that can access private variables.

One approach I can think of is to namespace your private variables. Then you can override __get() to return NULL if one of those namespaced variables is being requested. So let's say password becomes p__password. Your model can then implement __get() like so:

public function __get($name){
    if(substr($name,0,3) === 'p__'))
        return NULL;
    else
        return parent::__get($name);
}

Admittedly it's a bit hack-ish, but it should work.

edited Nov '14

@Conradaek - Here's the code for my model (cut down, but the rest is completely irrelevant to my issue):

class Users extends \Phalcon\Mvc\Model {
 // These are the fields in my database.
  private $id;
  public $username;
  public $email;
  private $password;

  public function getId() {
      return $this->id;
  }

  // Should take care of hashing the new password.
  public function setPassword($password) {
      $this->password = $this->hashPassword($password); // This method exists in my real code, omitted from the example
  }

  public function getPassword() {
      return $this->password;
  }

}

As you can see, id and password are private. I have provided as my desire is for id to be immutable, I have provided a getter and no setter. As password must be hashed behind the scenes, I have made a setter that takes care of hashing.

@quasipickle - I understand your example. It is hack-ish, but if it comes to that then I guess I must do it. However, I have one question about your method. - These private variables are the fields in my database... wouldn't giving them a prefix screw with the saving of their values? And then I would have to create a hacky work around for that?

What blows my mind is that this functionality would even be allowed.

If I do $user->id = 95;, it simply works. That defeats the entire purpose of a private variable.

However, if I do this: echo $user->id;, I get the expected result of:

Notice: Access to undefined property PrivateMessages::id in C:\xampp\htdocs\bbg-phalcon\app\controllers\IndexController.php



48.3k

This sounds like one for the issue tracker on github. My impression is that not a lot of small issues are going to be addressed for the next 2-3 months as they push to finish phalcon 2.0.

edited Nov '14

@dschissler - I think you're right. I'll check the issues and add it if I don't see any mention of it.

I have created an issue for this: https://github.com/phalcon/cphalcon/issues/3033

I wouldn't consider this a bug. Maybe poorly documented behaviour, but not a bug.

Being able to set properties "magically" is very often functionality provided by an ORM layer. Being able to set column (object) properties without needing a function call is used all the time. Perhaps a new feature could be added to Phalcon's base model to allow the coder (you) to set a "don't magically set private variables" flag.

Another approach might be to add some code to initialize() - find all the private variables in your object and store them. Then override __set() to not do anything if the requested variable is one of the private variables. Code shown in the accepted answer here: http://stackoverflow.com/questions/7338386/check-variable-is-public-php might be of use.

Now that I think of it - the only point of __get() & __set() is to access private variables. You might be able to get away with overloading those methods entirely and have them do nothing.

I wouldn't consider this a bug. Maybe poorly documented behaviour, but not a bug.

Being able to set properties "magically" is very often functionality provided by an ORM layer. Being able to set column (object) properties without needing a function call is used all the time. Perhaps a new feature could be added to Phalcon's base model to allow the coder (you) to set a "don't magically set private variables" flag.

Another approach might be to add some code to initialize() - find all the private variables in your object and store them. Then override __set() to not do anything if the requested variable is one of the private variables. Code shown in the accepted answer here: http://stackoverflow.com/questions/7338386/check-variable-is-public-php might be of use.

Now that I think of it - the only point of __get() & __set() is to access private variables. You might be able to get away with overloading those methods entirely and have them do nothing.

I understand what you're saying and appreciate the suggestions. I will attempt to work something out for this.

I must respectfully disagree that this is not a bug. I understand and appreciate the utility of the __get() and __set() methods and I have found use for them in the past. However, in this scenario, it defeats the entire benefit of making a variable private. Take a look at the excerpt from my Users class posted above - I have made password private specifically because it is application breaking if someone is able to access that variable without using the setPassword method.

I am unaware of why __set() is even used on the models, as I'm still getting familar with Phalcon, but I do know that ORM supports the use of setters and getters if they are present. At the very least, the default functionality should be __get() and __set making use of those getters and setters also.

Again, I appreciate your feedback. I'll look into overriding the default behavior and post what I find.

I appreciate your appreciation ;)

I think the ORM was built without consideration of the code you're writing interacting with code you don't write. That's the situation you're in and I think that's the only situation where this behaviour with __get() and __set() becomes an issue. Everyone else can just... not use them.

Reading your bug report - I do agree that there is an inconsistency between getting and setting.