jonathangeiger / kohana-jelly

See the link below for the most up-to-date code
https://github.com/creatoro/jelly
MIT License
146 stars 34 forks source link

[Unstable] Extending the Builder #167

Closed agentphoenix closed 14 years ago

agentphoenix commented 14 years ago

I'm not sure if this is a bug or me just doing things wrong, so I'm looking for clarification on this as well as reporting something strange.

The application I'm building with Jelly is massive and there are a ton of database interactions. When I was reading through the user guide, I noticed the section on extending the builder. After reading through it, it seemed like it was the perfect solution for being able to grab information quickly, like active users or grabbing a unique setting key. I decided to use the settings table as a testbed for this and it worked like a charm. I created a method that extended the builder so that I could easily pass a specific key through the method.

Jelly::query('setting')->key('date_format')->select();

This worked perfectly, but I noticed today that when I was trying to save records, it was failing.

$item = Jelly::factory('setting', 1);
$item->value = 'foo';
$item->save();

When I dumped the contents of the model, it was showing the field as being updated, but the database was never updated. I did some digging through Jelly's code and came across something interesting when I dumped the $this->_result variable from inside Jelly's builder ... it was trying to set where statement as WHERE setting_key = 1. The key is a string, but I was trying to pull based on ID. (Obviously if I wanted to pull based off of key, I could just do Jelly::query('setting', 'date_format')->select() now.)

Clearly, that key method is being called every time instead of just when I call it. So the question is, is this a bug or the proper behavior? If it is the proper behavior, where would I put methods for quickly getting at information without having to write a ton of extra code? I'd still like to be able to do something like

Jelly::query('setting')->key('date_format')->select();

Should that key() method go in the model or extend the builder like I'm doing now? Thanks!

jonathangeiger commented 14 years ago

Can you post your Setting model and its builder? I'm specifically interested in what you set as your primary key and how you're overriding unique_key().

First, (and this is somewhat unrelated) you don't necessarily have to use the key() method. It is implicitly used in the following cases:

Jelly::query($model, $id);
Jelly::factory($model, $id);

That said, you should be overriding unique_key similar to the following:

public function unique_key($value)
{
    if (is_numeric($value))
    {
        return $this->_meta->primary_key();
    }
    else
    {
        return $this->_meta->name_key();
    }
}

So that when unique_key() encounters a numeric value, the primary key column is used, and when a string based value is passed, your name_key (setting_key) is used.

This does need to be better documented.

agentphoenix commented 14 years ago

Here is my settings model.

class Model_Setting extends Jelly_Model {

    public static function initialize(Jelly_Meta $meta)
    {
        $meta->fields(array(
            'id' => Jelly::field('primary', array(
                'column' => 'setting_id'
            )),
            'key' => Jelly::field('string', array(
                'column' => 'setting_key'
            )),
            'value' => Jelly::field('text', array(
                'column' => 'setting_value'
            )),
            'label' => Jelly::field('string', array(
                'column' => 'setting_label'
            )),
            'user_created' => Jelly::field('enum', array(
                'column' => 'setting_user_created',
                'choices' => array('y','n'),
                'default' => 'y'
            )),
        ));
    }

    public static function get_settings($value)
    {
        // create a new class
        $obj = new stdClass;

        if (is_array($value))
        {
            $select = $value;
        }
        else
        {
            $select[] = $value;
        }

        $query = Jelly::query('setting')->select();

        if ($query)
        {
            foreach ($query as $i)
            {
                if (in_array($i->key, $select))
                {
                    $obj->{$i->key} = $i->value;
                }
            }
        }

        return $obj;
    }
}

And this is what I used for the settings builder.

class Model_Builder_Setting extends Jelly_Builder {

    public function key($value)
    {
        return $this->where('key', '=', $value)->limit(1);
    }
}

The result of using this model/builder combination means that when I try to do Jelly::factory('setting', 1)->set(array('value' => 'Foo'))->save();, it doesn't work because it's trying to load the database record with a key of 1 instead of the primary key of 1. When I comment out the builder stuff, it works like it's supposed to.

Thanks for the tip about the unique key method, I think that's actually going to solve my issue. The only thing I haven't been able to find is examples of how to set up name keys. It's mentioned in the docs, but I didn't notice any examples of how to do it. So given this model, how would I set up the setting_key field as a name key so that I can use the unique_key method that you posted?

jonathangeiger commented 14 years ago

Yea, you shouldn't be overriding key() like that. Rather, do what I showed you earlier for unique_key().

To set your name_key, do this:

class Model_Setting extends Jelly_Model {

    public static function initialize(Jelly_Meta $meta)
    {
        $meta->name_key('key')
             ->fields(array(
                // ...
        ));
    }
}
agentphoenix commented 14 years ago

Awesome! Thanks for you help, this is huge.

banks commented 14 years ago

Also agentpheonix, your get_settings method is exactly the sort of thing that should be in the builder not the model. We try to separate model for anything that is actually relating to a single model instance. Any listing/retrieving data should really happen in the builder if you are following our intentions for logic separation...

agentphoenix commented 14 years ago

Good to know. So what exactly should be going in the model (besides the initialization)?

banks commented 14 years ago

Anything that relates directly to the Behavior or that specific instance. For example, if you have a model representing a message in an email-like system, the model instance would contain methods to send, reply, encode text etc and builder would be used for all listing or mark all as read or other similar multiple-row manipulation.

These aren't strict rules so much as making your data model constant and logical. I find some ORMs blur model code (that actually applies to an instance) with listing/multiple-row manipulation such that the strength of the model analogy is broken and you can end up with quite un-intuitive interfaces. In my mind Jellys distinction is a usefull tool for keeping code clear even whm your doing pretty complex and magical things.