fuel / orm

Fuel PHP Framework - Fuel v1.x ORM
http://fuelphp.com/docs/packages/orm/intro.html
151 stars 95 forks source link

Sort order on a many-to-many relationship - possible ORM bug? #363

Closed nickautomatic closed 9 years ago

nickautomatic commented 9 years ago

I'm raising this as an issue at the request of @WanWizard - for the full background and more details, see this forum post and this question on Stack Overflow.

In short, it seems like there's a possible bug with the ORM's ability to sort by a field on the through table in a many-to-many relationship. The approach outlined in the docs (see the "ordering on a column in the through table" section on this page) - ie. using a simple "order_by" on the relevant field - currently doesn't work as far as I can tell (related items are returned but not ordered by the relevant field). The only way I can get sorting to work is to wrap this "order_by" in a "conditions" clause, but there seems to be no way of doing this that works for both lazy and eager loading.

I think the problem is with the way the ORM aliases the through table. Eager loading will work if I reference the through table by name (ie. 'order_by' => 'items_items.order'), but this causes a Fuel error when lazy loading:

Unknown column 'items_items.order' in 'order clause' with query: 
"SELECT `t0`.`title` AS `t0_c0`, `t0`.`uid` AS `t0_c1`, `t0`.`type` AS `t0_c2`,
`t0`.`created_at` AS `t0_c3`, `t0`.`updated_at` AS `t0_c4`, `t0`.`id` AS `t0_c5`
FROM `items` AS `t0` JOIN `items_items` AS `t0_through` ON (`t0_through`.
`child_id` = `t0`.`id`) WHERE `t0_through`.`parent_id` = '1' ORDER BY 
`items_items`.`order` ASC"

...as you can see, the items_items table is being aliased as t0_through in the JOIN clause, but not in the ORDER BY clause. I can get lazy loading working via the hacky approach of using 'order_by' => 't0_through.order', but this is obviously non-ideal, and (it turns out) then breaks eager loading ("Unknown column 't0_through.order' in 'order clause'"), because when eager loading the through table gets aliased as "t1_through" (I'm guessing depending on the query this could change anyway).

For the record, @WanWizard suggested that this issue may be related to this one.

nickautomatic commented 9 years ago

Hi again. Is this definitely fixed? Excuse my slowness responding to this, but I'm still, after multiple attempts, unable to get sorting on a field in a through table to work with both eager- and lazy-loading.

I even tried setting up a simple test case with a fresh install of Fuel (with the above fix added to the ORM, of course), but even there I was still unable to get it to work. If it's any use, my test case project is here - it's just a fork of Fuel, with a very simple Item model added (nb. the ORM is unpatched, so if you check the project out you'll still need to update the ORM - I wasn't totally sure how to do this with Composer).

What I'm wondering is: (a) Is there a combination of a version of Fuel + a version of the ORM where this is known to work? (b) Is there a test suite for the ORM at all? (Looking at the tests might help me figure out what I'm doing wrong!) (c) Failing that, is there something obviously wrong with my Item model? As far as I can see, the "children" relationship is set up correctly according to the ORM's documentation, but maybe I'm doing something wrong...?

Any help would be hugely appreciated - many thanks in advance.

WanWizard commented 9 years ago

All commits happen against the latest develop branch, in this case 1.8/develop. I don't think ORM has direct dependencies with the Fuel core, so you should be able to run the develop version of ORM with a 1.7.x release version of the core and other packages.

I only test changes in a 1.8/develop environment (as this will become the next release version), and it worked fine there (using the syntax in the docs).

nickautomatic commented 9 years ago

Thanks for the quick response. I tried what you suggest (though I couldn't use the latest version of 1.8/develop - see the footnote below for details), and I've finally made some progress, in that I've found a way of getting sorting to work that seems to work for both eager- and lazy-loading. However, it's still different from the syntax in the docs (and I haven't yet tested it with more complex queries in my main application), so I thought I'd let you know.

The approach that seems to be working is to wrap the 'order_by' parameters in a 'conditions' array:

'conditions'       => array(
    'order_by'     => array(
        'items_items.order' => 'ASC' // custom through table ordering
    ),
),

(See my code here for a fully commented explanation).

Effectively, it's a combination of what the docs suggest and my hack that worked for lazy-loading only. I'm pretty sure I tried this back when I first ran into this problem but it didn't work in the 1.7/master version, so I think your patch probably did fix this, albeit (as far as I can see) not in quite the way that's currently documented.

So my question is: does the above look ok? Is that how you tested it? Or is this still a bit of a hack that accidentally works, but which it isn't safe to rely on? To be honest, I do still feel a little uneasy that I still can't get the documented syntax to work.


Incidentally - while I'm here - for me on 1.8/develop Model_Item::find('all') throws a Fuel Error ("Cannot access protected property Orm\Query::$alias") from commit 122b49ba24e97 onwards, so I couldn't test with any subsequent commit. I'll open this as a separate issue (tbh, this isn't a problem for me, as long as I can get sorting to work, but I guess it's worth noting anyway).

nickautomatic commented 9 years ago

By the way, if you'd like to check my code, here's how to get it into the same state I tested it in:

(nb. if you have the same problem I had, you'll probably need to set the ORM back to commit 60cc4eb too)

nickautomatic commented 9 years ago

Update: I've now tested the above approach with my existing application and sorting is now working with no problems (it's a fairly complex app, and all tests are passing, so hopefully that's a good sign). I still had to use the 'conditions' clause, though (see above), rather than the syntax from the docs.

WanWizard commented 9 years ago

Given the fact that you also seem to have other weird issues (#375) in your setup, are you sure you are using either the release version 1.7.2. for both the core and all core packages (like ORM), or 1.8/develop for all of them?

All our apps run on 1.8/develop using these features without problems.

nickautomatic commented 9 years ago

Please excuse my slowness replying!

I am in fact using the 1.8/develop ORM with the 1.7.2 version of core (you did suggest in your earlier comment above that this should be ok), so I guess that's probably why the behaviour I'm getting doesn't quite match the documented syntax (although I guess that does suggest there may be a dependency between ORM and core?)

Anyway, for the record -- in case anyone else has this problem -- it does seem like the above solves the problem above while still being compatible with 1.7.2 core.

Thanks again for all your help - it's been a huge relief to finally be able to get this one working!