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

Conditional "many" relationships #103

Closed leth closed 14 years ago

leth commented 14 years ago

I have a feature request; for something-to-many relationships it would be nice to be able to specify additional conditions on the relationship.

E.g. consider a MAC address registration system for a network:

Tables

User: id ...

User_Mac: user_id mac_id valid_from valid_untill

Mac address: mac_id mac_address

It would be nice to be able to define conditions on relationships, so that you could have relationships like active_registrations and expired_registrations.

The example is many-many, but it would be useful for one-many relationships too.

(I am aware this example is a little contrived :P)

ccazette commented 14 years ago

You should do this via extending the Builder IMO. http://jelly.jonathan-geiger.com/docs/jelly.extending-builder

leth commented 14 years ago

It's certinally possible to do it that way, but i think it'd be nicer not to.

Perhaps a new field option could specify a method to call on the builder object, which defines the filter for that field. That way you could keep the business logic in the builder.

jonathangeiger commented 14 years ago

I'm not entirely sure this is a good idea. There are currently several ways to do this and it might be too broad of an idea to implement effectively.

Aside from the following as ccazette:

// Create active() and expired() methods in the Jelly Builder
Jelly::select('user')->get('registrations')->active();
Jelly::select('user')->get('registrations')->expired();

You can also just override the get() method in your model to do something like this:

class Model_User extends Jelly_Model
{
    public function get($name)
    {
        if ($name === 'expired_registrations')
        {
            return $this->get('registrations')->where('expired', '=', 1);
        }
        else if ($name === 'active_registrations')
        {
            return $this->get('registrations')->where('active', '=', 1);
        }

        return parent::get($name);
    }
}

// Create active() and expired() methods in the Jelly Builder
$user->active_registrations;
$user->expired_registrations;

I can see how providing an official way of achieving the second example would be useful, but I'd have to think about the edge-cases a bit before I considered it.

banks commented 14 years ago

This is an interesting idea - to have 'filterable' relationships - but one that is perfectly possible now and no change to the core seems justifed to implement an alternative, less flexible method. Here is my justification:

You can implement the behaviour you want with very little code in at least two ways using the current module.

  1. as ccazette mentions, you could define custom query builder methods to fetch results.
  2. perhaps even neater would be to define you own extensions to the relationship field classes that implement whatever filtering logic you need.

    One of the fundamental concepts of Jelly is that nearly all the logic happens in the fields and that customizing the behaviour of those fields is powerful and simple by extending the field classes. To hard code additional logic into the core to support such an edge case feature seems like a bad decision to me. The whole point of the field architecture is that people extend them to implement their own advanced features so changing the core for something like this seems the wrong solution.

Feel free to make a case for it with strong examples and an explanation why a change to the core would make more sense than the current solutions. We'll reconsider if there is a strong case but I can't see one at the moment.

leth commented 14 years ago

Sure, I'll have a look at extending the relationships to do this, and get back to you once I have.

leth commented 14 years ago

I've created a module which extends jelly to implement this, good work on the design guys, it was a piece of cake. At the moment there are probably some serious problems with the table aliasing, but I'll fix those later. http://github.com/leth/jelly-filtered

There's some example model declarations in this branch of my test fork http://github.com/leth/jelly-tests/tree/jelly-filtered

Let me know what you think :)