Phalcon\Mvc\Model __set method failure #11286

Closed
bmoore opened this Issue Jan 8, 2016 · 10 comments

Comments

Projects
None yet
6 participants
@bmoore
Contributor

bmoore commented Jan 8, 2016

After reviewing the documentation, and working with phalcon models. I found an issue using model setters. https://docs.phalconphp.com/en/latest/reference/models.html#public-properties-vs-setters-getters mentions that we can automagically use a method named "set" + camelize(property) to handle setting non-public properties. This however is not what is happening.

Consider the example

class User extends \Phalcon\Mvc\Model
{
    protected $id;
    public function setId($id) {
        echo "Got here.";
        $this->id = $id;
    }
}

...

$user = new User();
$user->save(['id' => 1], ['id']); // This will print "Got here."
...
$user = User::findFirst(1);
$user->id = 13; // This does assign 13 as the id but does not print "Got here."

After digging through the code, I noticed that the model magic __set method made no check for the possible setter, and instead just assigns the property.

phalcon/mvc/model.zep
...
abstract class Model implements EntityInterface, ModelInterface, ResultInterface, InjectionAwareInterface, \Serializable, \JsonSerializable
{
...
    public function __set(string property, value)
    {
        var lowerProperty, related, modelName, manager, lowerKey,
        relation, referencedModel, key, item, dirtyState;
        ...
        /**
         * Fallback assigning the value to the instance
         */
        let this->{property} = value;
        return value;
    }
    ...

This makes no mention or use of the possible setter.
Furthurmore second to last line completely bypasses the purpose of making a property private or protected, as it will always set the property regardless.

I can see the line in Model::assign() that utilizes the possible setter.

    let possibleSetter = "set" . camelize(attributeField);
        if method_exists(this, possibleSetter) {
            this->{possibleSetter}(value);
        } else {
            let this->{attributeField} = value;
        }

I believe this should be refactored so the possible setter is utilized in __set and also fix the issue with __set completely bypassing private and protected properties.

I will provide a proof-of-concept patch in the near future.

@bmoore

This comment has been minimized.

Show comment
Hide comment
@bmoore

bmoore Feb 1, 2016

Contributor

Does anyone here have any thoughts with this one? I've been trying to rewrite it possibly using the ReflectionProperty class, but was getting segfault errors from it. Now I am getting real errors on TravisCI though I cannot yet run the unit tests locally.

P.S. Is there a guide somewhere about getting set-up for running test locally?

Contributor

bmoore commented Feb 1, 2016

Does anyone here have any thoughts with this one? I've been trying to rewrite it possibly using the ReflectionProperty class, but was getting segfault errors from it. Now I am getting real errors on TravisCI though I cannot yet run the unit tests locally.

P.S. Is there a guide somewhere about getting set-up for running test locally?

@sergeyklay

This comment has been minimized.

Show comment
Hide comment
@sergeyklay

sergeyklay Feb 12, 2016

Member

Could you please check now 2.1.x branch, after accepting #11401

Member

sergeyklay commented Feb 12, 2016

Could you please check now 2.1.x branch, after accepting #11401

sergeyklay added a commit that referenced this issue Feb 17, 2016

Merge pull request #11320 from bmoore/2.1.x
Minor rewrite to assign and __set issue #11286

sergeyklay added a commit that referenced this issue Feb 17, 2016

Merge pull request #11417 from bmoore/2.0.x-patch-11286
[2.0.x] Issue #11286 - Model property visibility and setter functionality
@michanismus

This comment has been minimized.

Show comment
Hide comment
@michanismus

michanismus Feb 17, 2016

Contributor

I cant access my model aliases (relations) anymore with 2.1.x

Contributor

michanismus commented Feb 17, 2016

I cant access my model aliases (relations) anymore with 2.1.x

@sergeyklay

This comment has been minimized.

Show comment
Hide comment
@sergeyklay

sergeyklay Feb 17, 2016

Member

@michanismus Can you please provide script to reproduce?

Member

sergeyklay commented Feb 17, 2016

@michanismus Can you please provide script to reproduce?

@michanismus

This comment has been minimized.

Show comment
Hide comment
@michanismus

michanismus Feb 17, 2016

Contributor

@sergeyklay I opened a new issue #11427

Contributor

michanismus commented Feb 17, 2016

@sergeyklay I opened a new issue #11427

@sergeyklay

This comment has been minimized.

Show comment
Hide comment
@sergeyklay

sergeyklay Feb 18, 2016

Member

@bmoore Issue is solved?

Member

sergeyklay commented Feb 18, 2016

@bmoore Issue is solved?

@michanismus

This comment has been minimized.

Show comment
Hide comment
@michanismus

michanismus Feb 18, 2016

Contributor

Sorry for push, but we need a solution. The new behaviour breaks a lot of code for me!

Why should the model always search and use an available setter? If I want to use a setter, I call it!
As I mentioned, with the new behaviour it is impossible to create a getter function for Model aliases. Inside the function the property is not accessible anymore.

I think it would better to extend the Model class to gain "auto use setter" behaviour and revert the changes.

Contributor

michanismus commented Feb 18, 2016

Sorry for push, but we need a solution. The new behaviour breaks a lot of code for me!

Why should the model always search and use an available setter? If I want to use a setter, I call it!
As I mentioned, with the new behaviour it is impossible to create a getter function for Model aliases. Inside the function the property is not accessible anymore.

I think it would better to extend the Model class to gain "auto use setter" behaviour and revert the changes.

@Surt

This comment has been minimized.

Show comment
Hide comment
@Surt

Surt Mar 28, 2016

Contributor

@michanismus +1 to the optional behaviour parameter, so we can decide if the object uses or not the auto use setter.

and of course, + 1 to autosetter use, afterFetch and beforeSave are really difficult to manage if I want my model to take care of data transformations etc

Contributor

Surt commented Mar 28, 2016

@michanismus +1 to the optional behaviour parameter, so we can decide if the object uses or not the auto use setter.

and of course, + 1 to autosetter use, afterFetch and beforeSave are really difficult to manage if I want my model to take care of data transformations etc

@sergeyklay sergeyklay added this to the 2.1.0 milestone Mar 28, 2016

@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag May 25, 2016

Member

Wait wait wait. I don't understand, when they should be set ? It's normal behaviour. Afterfetch means AFTER FETCHING, like SELECTING, SETTING VALUES ETC

Member

Jurigag commented May 25, 2016

Wait wait wait. I don't understand, when they should be set ? It's normal behaviour. Afterfetch means AFTER FETCHING, like SELECTING, SETTING VALUES ETC

@Surt

This comment has been minimized.

Show comment
Hide comment
@Surt

Surt May 25, 2016

Contributor

[edited: wrong conclusion]

I did a clean test from scratch and everything is working fine. My fault.

Contributor

Surt commented May 25, 2016

[edited: wrong conclusion]

I did a clean test from scratch and everything is working fine. My fault.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment