Fix reverse join inconsistencies and defaults #802
Conversation
src/Model/Join.php
Outdated
| [$this->foreign_table, $this->foreign_field] = preg_split('/\.+(?=[^\.]+$)/', $this->foreign_table); | ||
| if ($this->reverse === true) { | ||
| if (isset($this->master_field) && $this->master_field !== $id_field) { | ||
| throw (new Exception('You are trying to link tables on non-id fields. This is not implemented yet')) |
There was a problem hiding this comment.
| throw (new Exception('You are trying to link tables on non-id fields. This is not implemented yet')) | |
| throw (new Exception('Joining tables on non-id fields is not implemented yet')) |
There was a problem hiding this comment.
I wonder what is the main issue
There was a problem hiding this comment.
If i remember correctly that was because we can't rely on fields if at least one of them is not unique id field. Something along these lines. Anyway this PR tries not to solve that.
There was a problem hiding this comment.
we can't rely on fields if at least one of them is not unique id field.
is this important if all work is done by SQL?
There was a problem hiding this comment.
All work is not done by SQL I think. At least that wasn't the case time ago. It's important in what order you insert records (reverse or not) and afterwards you have to insert respective record in other table too.
Anyway I specifically didn't address that here!
This PR is just about adding more tests and a bit normalizing method how we define reverse joins. Before there was only a one and not intuitive way how to define them. And as soon as you, for example, explicitly said that join is reverse or even set master-field='id' yourself (same as default value set by ATK) it started to fail with You are trying to link tables on non-id fields. This is not implemented yet exception.
There was a problem hiding this comment.
I specifically didn't address that here
but 1/2 of the added LOC are exceptions for that, better to solve the ID limitation if possible, I am ok with merging this, but with proper issue for that limitation and adding a link to it to the code (like I tried with this PR, replace them)
There was a problem hiding this comment.
These two added expected exceptions in tests was added because previously tests was wrong. They "tested" cases which was not supported, but didn't get proper exception because of slightly wrong exception implementation in Join class (which is now fixed there).
OK created issue #803 for that and will see what can be done but outside of this PR.
Co-authored-by: Michael Voříšek <[email protected]>
Co-authored-by: Michael Voříšek <[email protected]>
Co-authored-by: Michael Voříšek <[email protected]>
Also explains, fix #799