[2.1.x] Model alias in getter function not accessible

[michanismus]

Example:
Defined relation with alias "roles" in Model named "User".

I defined a getter function named getRoles() where I want to access $this->roles results in error: Undefined property: User::$roles

The relation:

/**
 * @HasMany(
 *     "id",
 *     "Model\UserRole",
 *     "userId",
 *     {
 *         "alias": "roles"
 *     }
 * )
 */

The function:

    public function getRoles()
    {
        if ($this->getDirtyState() == self::DIRTY_STATE_PERSISTENT) {
            $roles = array();
            foreach ($this->roles as $role) {
                $roles[] = $role->getRole();
            }

            return $roles;
        }

        return $this->_roles;
    }

[michanismus]

@sergeyklay I can use the alias anywhere else, but it is impossible to create/overwrite a getter function for the alias in the model. I use the latest 2.1.x branch. Error happens since your last 2.0.x merge.
Guess it is related to #11286

[michanismus]

Found the commit which changes the behaviour:
6d17755

BTW, why try to find a setter function and use it when I want to access the property directly? I mean, when I want to use a defined setter I simply call it.
What if I want to bypass logic of a setter? Now it is impossible when the property is defined as protected or private.

In my opinion this is useless and produces a lot of errors/complications. I wanna choose to use a setter or not.

[sergeyklay]

@nsossonko

[bmoore]

I cannot find this commit in the 2.0.x history. Does this issue still exist with my changes?

Also, there is no restriction to create and utilize your own setter methods. This just provides magic so you can use $user->role = instead of $user->setRole($role);

[bmoore]

I do see this commit on 2.1.x

So this get function is in the user model? What is the visibility of the roles and _roles properties? Could you provide an expected and actual vardump ?

[michanismus]

2.1.x branch and yes issue exists with your changes.
The problem is, I want to decide to use a setter or not.
And it breaks the getter function for aliases. Take a look at my function, the alias (not existing property) is not accessible inside the function anymore.

[michanismus]

I have to recompile phalcon with your changes. Takes a couple of minutes, then I will provide a call stack...

[michanismus]

Here is a call stack for the error:

ErrorException: Undefined property: Horus\AdminBundle\Model\User::$roles
 at Horus\Helper\ErrorHandler->handleError(User.php:206)
 at Horus\AdminBundle\Model\User->getRoles(User.php:206)
 at Horus\AdminBundle\Model\User->setRoles(User.php:219)
 at Phalcon\Mvc\Model->_possibleSetter(Unknown Source)
 at Phalcon\Mvc\Model->__set(Unknown Source)
 at Phalcon\Mvc\Model->__get(Unknown Source)
 at Horus\AdminBundle\Model\User->getRoles(User.php:206)
 at Horus\AdminBundle\Helper\Auth\Auth->setUser(Auth.php:120)
 at Horus\AdminBundle\Helper\Auth\Auth->authenticate(Auth.php:157)
 at Horus\AdminBundle\Controller\AuthController->loginAction(AuthController.php:53)
 at Phalcon\Dispatcher->callActionMethod(Unknown Source)
 at Phalcon\Dispatcher->dispatch(Unknown Source)
 at Phalcon\Mvc\Application->handle(Unknown Source)
 at Horus\Kernel->handle(Kernel.php:219)
 at (main)(index_dev.php:9)

and these are my functions:

    public function getRoles()
    {
        if ($this->getDirtyState() == self::DIRTY_STATE_PERSISTENT) {
            $roles = array();
            foreach ($this->roles as $role) {
                $roles[] = $role->getRole();
            }

            return $roles;
        }

        return $this->_roles;
    }

    public function setRoles($roles)
    {
        if ($this->getDirtyState() == self::DIRTY_STATE_PERSISTENT) {
            foreach ($this->getRoles() as $name) {
                if (!in_array($name, $roles)) {
                    UserRole::createBuilder()
                        ->where('userId = :id: AND role = :role:')
                        ->getQuery()
                        ->execute(array(
                            'id'   => $this->getId(),
                            'role' => $name
                        ))
                        ->delete();
                }
            }

            $roles = $this->getRoles();

            foreach ($roles as $name) {
                if (!in_array($name, $roles)) {
                    (new UserRole)
                        ->setUserId($this->id)
                        ->setRole($name)
                        ->save();
                }
            }
        } else {
            $this->_roles = $roles;
        }

        return $this;
    }

[nsossonko]

This has to do with triggering the error specifically at the end. I think it would be better to just get rid of that entirely and revert to old behavior regarding that. Just removing this block: https://github.com/phalcon/cphalcon/blob/2.1.x/phalcon/mvc/model.zep#L4165 should fix this issue. In reality, it does sync up behavior with the getter but, unlike in the getter scenario, Phalcon caches relations on non-existent properties using the alias name so it shouldn't really be synced. Alternatively, a check can be made to ascertain whether or not there are aliases with that name...

[michanismus]

Guess the problem is the __get function finds a relationship and tries to set the property. The property, the alias, does not exist at this moment and your new auto setter is called. If the setter tries to use my getter or the relation itself, it ends up in a loop and crashs.

https://github.com/phalcon/cphalcon/blob/2.1.x/phalcon/mvc/model.zep#L4254

The definition of an NULL property as placeholder for the relation alias does not work, the relation wont never get initialized.

[michanismus]

@nsossonko Yes, a check for an alias in __set would fix it. But anyway, I think there should be a discussion about the "try to find a setter and execute it behaviour". What if I want to bypass setter logic in some cases? At the moment it is impossible, the setter is always called.

[nsossonko]

What's the point of getter/setter functionality if the setter is not called? How does it make sense to bypass a setter? If you really want to bypass the setter, just define the property as a public property.

[michanismus]

The point is, it was possible before.
Now an exception is thrown if the property is not public. Before it was possible to set a protected property, now it is not.
Another example. I defined a function setDate(...) which does some stuff but has nothing to do with a property "date" itself. I cant set a public property "date" for the model anymore because $model->date will always call the setDate function. The point is, I can't do it anymore, not matter if it makes sense or not.

[michanismus]

Your question "How does it make sense to bypass a setter?". Right, so why don't use $model->setSomething() instead of using $model->something and try to execute a setter?

[nsossonko]

It's pretty common convention in any getter/setter implementation to call the setter automatically whenever the property is set. Previously this was buggy because it would not call the setter (in fact, it called the setter when assign was called, but not when the property was directly adjusted). The point of getter/setter is to protect your object so that external callers cannot do bad (unexpected) stuff to it. Also, about "before it was possible to set a protected property" that in and of itself is not correct for the most part as it defeats the purpose of protected properties. In your setDate example, you should be able to set a public property 'date' as it will bypass the __set call entirely. You just can't create a protected 'date' property and expect it to be settable from outside the class (standard PHP behavior and expectation).

[michanismus]

You are right, the previous __set function bypassed standard behavior and expectation.
The setDate example, you are wrong. If the property is not defined __set will called and in the end the setDate function is called, I tested it. So it is not possible to add a public property while script is executed no matter if it makes sense or not.

Anyway, there is still the big problem with aliases what needs to be fixed asap.

[bmoore]

The issue you seem to be mentioning with the date is that you're setting an undefined property at runtime. Php is flexible and can do that, but there are some caveats.

Php behavior by default will directly access a public property, setting and getting. If a property is not defined or is hidden (private or protected), then it will try to use magic (__set or __get) I believe if a property is set on runtime, it is treated as public, but also follows the rules of an undefined property, so it will continue to use the setter getter magic funtionality.

As for your issue with roles, it is a similar issue with runtime setting of undefined properties as alias rules. Can this roles property not be defined as private or protected ahead of time in the head of your user class? The previous functionality of the __set method was wrong, so reverting is not an acceptable solution, but maybe the way relations are handled needs to be reconsidered.

A question I have, is why are the unit tests around relationships not failing, but your code is broken (edge case)?

I had mentioned that a discussion should be had on how phalcon magic should be used. We should still discuss repercussions with current implementation.

I do not think this is a quick fix issue, so In the meantime, if your code depends on old improper functionality, is it not a possibility to use a past known working commit?

[michanismus]

@bmoore

Can this roles property not be defined as private or protected ahead of time in the head of your user class?

No. Otherwise the relation wont be initialized by __set function because its is already defined.

A question I have, is why are the unit tests around relationships not failing, but your code is broken (edge case)?

I dont think my solution is an uncommon case.

I guess because I use both, a get and a set function for a relation and in the set function I merge existing records with new and/or delete unused. Therefor I call the get function in the set function which tries to access the property "roles" which is the alias. All ends up in a loop and PHP crashs because __get adds the relation as property and my set function is called again.

Take a look:
https://github.com/phalcon/cphalcon/blob/master/phalcon/mvc/model.zep#L4190

I do not think this is a quick fix issue, so In the meantime, if your code depends on old improper functionality, is it not a possibility to use a past known working commit?

Unfortunately I have to, yes. A fix for relations would be very welcome.

[bmoore]

Looking through the code and the back trace, __get tried to assign the property roles at run time. This triggers __set which checks for relations first. S ce the relation want set, it then looks for the setter. This setRoles then calls getRoles which then tries to access a still undefined $this->roles which then throws your exception. That's the loop problem, partly in the way your get and set methods call eachother as you have mentioned.

The fact that the alias will break when you define the property for roles makes me think that the way __get and __set handle relationships may need to be rearchitechted.

I have a quick question, if we were to swap the assignment in __get around line 4250

            /**
             * Assign the result to the object
             */
            if typeof result == "object" {

                /**
                 * We assign the result to the instance avoiding future queries
                 */
                let this->{lowerProperty} = result;

                /**
                 * For belongs-to relations we store the object in the related bag
                 */
                if result instanceof ModelInterface {
                    let this->_related[lowerProperty] = result;
                }
            }

            return result;
        }

With

            /**
             * Assign the result to the object
             */
            if typeof result == "object" {
                /**
                 * For belongs-to relations we store the object in the related bag
                 */
                if result instanceof ModelInterface {
                    let this->_related[lowerProperty] = result;
                }

                /**
                 * We assign the result to the instance avoiding future queries
                 */
                let this->{lowerProperty} = result;
            }

            return result;
        }

Would we still have this loop? I'm trying to see where the logic fails, and it seems to fail by continuing beyond where it should in __set. Im thinking it should return something in__set where it tests for relationships. I am on a plane without access to my laptop to attempt this.

This could also be an issue of phalcon trying to do too much with one property, holding relation stuff. I will continue to look into this.

[michanismus]

I test your replacement. Give me some minutes ;)

[michanismus]

Nope, that didn't work.
I also tried to replace all statements like "this->{lowerKey} = item" with internal write attribute function "this->writeAttribute(lowerKey, item)" but it didn't work as well. Of course not, because it tries to set the property the same way.

[michanismus]

I can fix my code by replacing "$this->roles" with $this->getRelated('roles'). I can live with this solution but the loop problem (get -> set -> get) still remains.

@bmoore

I had mentioned that a discussion should be had on how phalcon magic should be used. We should still discuss repercussions with current implementation.

Right. Wouldn't it be better to drop getter and setter functions support in magic completely?
__get and __set should handle relations if they are accessed the direct way "$this->roles" and protected and private properties are ignored!?
If a property is public it is accessible anyway, for the rest an Exception will be thrown. The user has to define setter and/or getter functions to access these properties which is the standard behavior and expectation like @nsossonko mentioned.

[nsossonko]

@michanismus why not rename your setter to _setRoles (or something similar) to avoid the auto-calling? I re-read your code and it seems to me it is prone to this error for which there's not much that can be done. The cycle is: getter->__get->__set->setter->getter. The underlying property is never being successfully set because your setter is in the way. So why not just rename your setter (which is not a true setter anyhow) to something that won't conflict with this?

[michanismus]

@nsossonko Thats true but won't change the problem, which is simple. If I call a relationship getter in a relationship setter for the same relationship, it will end in a loop and an error (If I use the alias as property to access it).

[michanismus]

@nsossonko Thats why I asked if its maybe better to drop getter and setter functionality in magic completely.

[bmoore]

So you found a workaround? And yes, there's a partial benefit with keeping expectations along with standard php. I started developing on the model section with my patches as I had issues with the magic. What I wrote handled an glaring upfront issue with way the documentation and tests described the magic. Overall, I am discontent with the way the setter getter actions in models are handled. @phalcon can we open a discussion about the reasoning and plans for this?

[nsossonko]

At the cost of not allowing setters for relationships. It's not really a bug because you defined a setter for the relationship, so you should be setting the property there, not attempting to get it. If you want to define a utility function for relationships it should not be named like a setter is meant to be named.

The new functionality is actually really cool in that it allows custom behavior for setting relations in Phalcon. I don't think requiring clients to not trick the system with fake setters to get the property is worth getting rid of this new (consistent) functionality, IMHO. Getter/setter "magic" functionality is part of most frameworks, btw.

[michanismus]

@nsossonko Yes I should set the property in a relationship setter but what is a function good for if we cannot do other stuff there. And who says I am not allowed or it is not "standard" to call the getter for the same relation in the setter method?

I didn't mean to drop relations in magic functions, I meant to drop support for properties/fields. If a field is defined as protected, treat it like a protected. You have to use a set function for this field/property. It would be, like you said, standard behavior and expectation, and don't use __set as proxy therefor.

[michanismus]

@Jurigag The problem is the following. The magic __get function is looking for a getter function, getRoles in this case, but the same function tries to access $this->roles which is a relation.
Now there is an infinite loop because the property does not exists because it is virtual and a relation.

[Jurigag]

No no, i mean why after adding method getRoles phalcon tries to call magic getter anyway when you call getRoles - or i don't get something?

The problem from your issue is okay, you overrided getter - so you need to add some logic to set roles property or set them before calling this method.

[michanismus]

Thats exactly the problem - it is a relation, property rolesdoes not exists, so the magic __get function is called and the magic function sees the getter function which is named getRoles... now the "magic" happens and it ends in an infinite loop.

I still think it is a very bad idea to look and call a getter/setter function automatically just for the fact it is there. I mean, if I want to use the getter/setter which I was implementing, then I would simply call it.

But I guess these are bad arguments cause it is already merged...

[Jurigag]

Then if you want you to work like this you just need to add this property to your class if you want to access it and set it somewhere with getRelated.

[michanismus]

Thats right - it is just an example. I now use getRelated() all the time and never ever again access these relations like a object property $model->relationName.
My bigger concerns are the automatically called getter/setter functions if they are there.
Magic __get and __set for relations, yes, but generally for attributes + relations = no.

Just my 2 cents ;)

I think we can close this issue anyway.

[Jurigag]

No it's not automitcall called - it's only called if there is no property defined and you have getter defined - then there is getter called. You should use properties and getters anyway.

[michanismus]

Maybe there should be an ORM setup and/or INI setting to enable/disable auto use of setter/getter in magic function __get/__set!?

What do you think?

[michanismus]

@Jurigag Take a look at the call stack and the functions I posted.
#11427 (comment)

The buggy _possibleSetter function was removed meanwhile. ;)
So I hope/guess it is not a problem anymore..
Issue was opened 14 month ago.

But my question is still relevant. What do you think about a setting for automatically called getter/setter methods in magic functions?

[Jurigag]

If you don't have property added and have public setter/getter then when setting/getting non existing property then it should be using getter/setter for sure. Don't like to change it, not see reall why, because otherwise we would just simply need to produce exception - you mean want it like this? This could good though - i could remember that i forgot to add getter/setter.

[michanismus]

Yes/No :)

I think it would probably be the best to enable/disable the whole behavior.

If enabled, __get and __set is looking for getter/setter functions and call them if available, like it is handled right now.
Otherwise just throw an exception. Maybe enabled by default but possible to disable it via setup() or php ini.

For me it is still not logical to do this automatically. For defined model relations, yes, no doubt about it. But for model attributes or helper properties? I think not, but everyone should be able to decide for himself/herself.

[Jurigag]

Yea then i would agree - i have getters for everything so it could be quite nice to know if i have __get or __set used anywhere. For now im adding way to disable usage of setters for assign method which
is used in save/create quite often - i have properties defined but it's using without need setters anyway. In next versions we could add the same for way to disable __get/__set for stuff not being relations.

[michanismus]

Sounds great! Thumb up therefor.
Just give the users the possibility to decide if they want to use it or not.

[sergeyklay]

I want to close this issue because I don't want a bunch of these lying around. I'd like to add support for anything/everything at some point, but keeping the issue open doesn't help that. No work was put towards this. I agree with @niden We can't provide ability for some but break a lot of applications for others. Probably we need a more elegant and safety solution. If anyone wants to get started, I'd love that. Closing.