jpfuentes2 / php-activerecord

ActiveRecord implementation for PHP
http://www.phpactiverecord.org/
Other
1.32k stars 443 forks source link

Model::__isset() to return same as __get() #156

Open silentworks opened 13 years ago

silentworks commented 13 years ago

I had a issue with using ActiveRecord with Twig Templating Engine due to the way how ActiveRecord looks up isset(), I changed it to match the get() method and it all seems to work fine now.

public function __isset($name)
    {
        // check for getter
        if (method_exists($this, "get_$name"))
        {
            $name = "get_$name";
            $value = $this->$name();
            return $value;
        }

        return $this->read_attribute($name);
    }
terite commented 13 years ago

I chose to solve this problem a different way.

... but I've since deleted the repo. whoops

silentworks commented 13 years ago

Thank you for that, this works perfectly. If anymore bugs show up when using ActiveRecord with Twig I will let you know.

greut commented 12 years ago

the above link is broken: https://github.com/terite/php-activerecord/commit/68bec6cd93786b26514a9d9ff37b54a67610ea2c

Having a proper test would be fine.

guillemcanal commented 11 years ago

Thank you for that ;)

bshevchenko commented 11 years ago

Thanks a lot!!!!

bshevchenko commented 11 years ago

Hmm... i take it back. set_timestamps() checks the updated_at and created_at attributes through the __isset method.

Everything will work fine if you change the set_timestamps() as follows:

public function set_timestamps()
{
    $now = date('Y-m-d H:i:s');

    if (isset($this->attributes['updated_at']))
        $this->updated_at = $now;

    if (isset($this->attributes['created_at']) && $this->is_new_record())
        $this->created_at = $now;
}
bshevchenko commented 11 years ago

This is really bad solution. After these changes PHPActiveRecord began throw ActiveRecordException("Call to undefined method: $method") every time I tried to get the value of an empty model attribute. I had to comment out throw and add "return null" instead in Model.php, __call method.

anther commented 11 years ago

I feel like you guys are complicating a lot. This is object oriented programming with a public interface!

You don't need to modify Model.php..., this ruins your ability to upgrade phpactiverecord. Create a subclass that all of your Models that are used in Twig inherit from, especially if you're not planning on writing tests.

and do something similar to this or modify it to what you need.

class TwigModel extends ActiveRecord\Model
 {
    public function __isset($attribute_name)
    {
        try
        {
            if($this->__get($attribute_name))
            {
                return true;
            }
            else
            {
                return false;
            }
        }
        catch(UndefinedPropertyException $e)
        {
            return false;
        }
        catch(ActiveRecordException $e)
        {
            return false;
        }
    }
 }

User.php

User extends TwigModel
{
   $normal phpactiverecord stuff
}

I don't think commenting out exceptions counts as a proper fix ;p.

bshevchenko commented 11 years ago

anther, if you use your decision, you will have to create two identical models for each case, except for the fact that they will inherit from other classes. One for views and one for controllers.

bshevchenko commented 11 years ago

Oh, sorry for inattention. I thought that your post contained a specific solution, not advice on the correct extension =)

And that's the way a working solution (using your advice):

use ActiveRecord\ActiveRecordException;

class TwigModel extends \ActiveRecord\Model
{
    public function __isset($attribute_name)
    {
        try
        {
            if($this->__get($attribute_name))
                return true;
            else
                return false;
        }
        catch(UndefinedPropertyException $e)
        {
            return false;
        }
    }

    public function __call($method, $args)
    {
        try
        {
            $result = parent::__call($method, $args);
        }
        catch(ActiveRecordException $e)
        {
            return null;
        }

        return $result;
    }
}

Simply inherit all the models from this class if you are going to use Twig.

eps90 commented 10 years ago

Hi, is this fix will ever be in this repo ? :) I fixed it in this way:

public function __isset($attribute_name)
    {
        if (array_key_exists($attribute_name,$this->attributes) || array_key_exists($attribute_name,static::$alias_attribute)) {
            return true;
        }
        try {
            $this->__get($attribute_name);
            return true;
        } catch (UndefinedPropertyException $e) {
            return false;
        }
    }

I'm using this version in many projects that use php-activerecord and there are no errors, but I want to keep it up-to-date so I can't still using my fork..

dfyx commented 10 years ago

Another version that doesn't rely on catching exceptions and also doesn't call any (potentially expensive) getters.

    /**
     * Determines if an attribute exists for this {@link Model}.
     *
     * @param string $attribute_name
     * @return boolean
     */
    public function __isset($attribute_name)
    {
        // check for aliased attribute
        if (array_key_exists($attribute_name, static::$alias_attribute))
            return true;

        // check for attribute
        if (array_key_exists($attribute_name,$this->attributes))
            return true;

        // check for getters
        if (method_exists($this, "get_${attribute_name}"))
            return true;

        // check relationships if no attribute
        if (array_key_exists($attribute_name,$this->__relationships))
            return true;

        return false;
    }

Edit: note that $attributes and $relationships are private. Therefore it is not possible to use this through inheritance.

dfyx commented 10 years ago

And another one that works through inheritance (uses ReflectionProperty to access private properties)

class Model extends \ActiveRecord\Model {
    /**
     * Determines if an attribute exists for this {@link Model}.
     *
     * @param string $attribute_name
     * @return boolean
     */
    public function __isset($attribute_name)
    {
        if(parent::__isset($attribute_name)) {
            return true;
        }

        // check for getters
        if (method_exists($this, "get_${attribute_name}"))
            return true;

        // check relationships if no attribute
        $parent = new \ReflectionClass('ActiveRecord\Model');
        $relationships = $parent->getProperty('__relationships');
        $relationships->setAccessible(true);

        if (array_key_exists($attribute_name, $relationships->getValue($this)))
            return true;

        return false;
    }
}
dfyx commented 9 years ago

I just noticed that both of the above have a bug when accessing a relationship that hasn't been loaded yet. Here's a fix that works with inheritance and doesn't need reflections:

class Model extends \ActiveRecord\Model {
    /**
     * Determines if an attribute exists for this {@link Model}.
     *
     * @param string $attribute_name
     * @return boolean
     */
    public function __isset($attribute_name) {
        if(parent::__isset($attribute_name))
            return true;

        // check for getters
        if (method_exists($this, "get_${attribute_name}"))
            return true;

        // check for relationships
        if (static::table()->has_relationship($attribute_name))
            return true;

        return false;
    }
}
cvanschalkwijk commented 9 years ago

@dfyx could you submit a PR for this?

nasrulhazim commented 8 years ago

thanks @dfyx ! it works now with Twig.

renanconstancio commented 7 years ago

Aqui aparentemente funcionou tudo certinho. Obrigado a Anther commented on 29 Mar 2013.