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

ManyToMany if through is array, problem #50

Closed dotcreative closed 14 years ago

dotcreative commented 14 years ago

If I set through as an array in the model I will get a wrong $this->through. A possible solution would be this: else { // Use the first column as the model list($model, $col1) = explode('.', $this->through[0]); list($model, $col2) = explode('.', $this->through[1]);

        $this->through = array(
            'model' => $model,
            'columns' => array($col1, $col2)
        );
    }

A second solution, and I would recommend this is to change the documentation to use the correct format: 'through' = array( 'model' => 'model', 'columns' => array( 'model:foreign_key' 'foreign:foregn_key' ) )

and drop the else from the code.

jonathangeiger commented 14 years ago

Is there are problem with the way it's supposed to be set? Why can't you just use that way?

dotcreative commented 14 years ago

In your example here:

            // Or if you need to specify the columns in the pivot table:
            'through' => array(
                author_roles.author_id,
                author_roles.role_id,
            ),

you say that through model and columns can be set like above. If they are set like this, your code doesn't build correctly $this->through. Check your code please. In my first post are 2 ways to solve this.

Edit: In your code you use:

    // $this->through is an array of two elements
    else
    {
        // Use the first column as the model
        list($model) = explode('.', $this->through[0]);

        $this->through = array(
            'model' => $model,
            'columns' => $this->through
        );
    }

this sets:

'columns' = array('model.model:foreign_key', 'model.foreign:foreign_key')

which is not correct.

There are 2 ways to solve this, either by building the correct $this->through from your example, or change your example and lose the else completely.

dotcreative commented 14 years ago

I commited to my branch the solution of dropping else and changing the documentation. It should be cleaner.

here is the commit

banks commented 14 years ago

@dotcreative, thanks for reporting this but you need to give a better explanation of the problem. We have tests that show this works for basic usages so far so either you are doing something we haven't tested yet or you are doing something we never intended to work. Either could be true but just saying "your code doesn't build correctly $this->through." doesn't help us much determine exactly what you are having a difficulty with.

Can you explain what exactly you are trying and what exactly is 'wrong' about what Jelly does (example of the query or return value that is wrong etc) that would be a great help, thanks.

If there is no actual problem and it just doesn;t work like you expected, you can either fork and update the docs or just add a request to the documentation issue #46

dotcreative commented 14 years ago

@banks:

I really thought I explained everything :(. Maybe I'm not that good at explaining.

    // We can work with nothing passed or just a model
    if (empty($this->through) OR is_string($this->through))
    {
        if (empty($this->through))
        {
            // Find the join table based on the two model names pluralized, 
            // sorted alphabetically and with an underscore separating them
            $through = array(
                inflector::plural($this->foreign['model']), 
                inflector::plural($model)
            );

            // Sort
            sort($through);

            // Bring them back together
            $this->through = implode('_', $through);
        }

        $this->through = array(
            'model' => $this->through,
            'columns' => array(
                inflector::singular($model).':foreign_key',
                inflector::singular($this->foreign['model']).':foreign_key',
            )
        );
    }
    // $this->through is an array of two elements
    else
    {
        // Use the first column as the model
        list($model) = explode('.', $this->through[0]);

        $this->through = array(
            'model' => $model,
            'columns' => $this->through
        );
    }

This is the code from manytomany, initialize method.

There are 3 situations. If through is not defined at all, if through is a string and if through is an array.

For the first 2 options, the code builds $this->through in the following format:

$this->through = array(
    'model' => 'model',
    'columns' => array(
        'model:foreign_key'
        'foreign:foregn_key'
    )
)

in the third case (if 'through' is an array') it builds it this way:

$this->through = array(
    'model' => 'model',
    'columns' => array(
        'model.model:foreign_key'
        'model.foreign:foregn_key'
    )
)

See the difference?

banks commented 14 years ago

OK thanks for that. We'll take a look - you patch is probably the right solution.

jonathangeiger commented 14 years ago

I think your work is good. I'll be pulling it in soon.

jonathangeiger commented 14 years ago

Thanks for all your help!

dotcreative commented 14 years ago

Thank you Jonathan and Banks for what it seems a great ORM. Glad I can help.