Issue#5561: Taxon root is not being checked #5662
Conversation
vcraescu
commented
Jul 30, 2016
vcraescu
commented
| Q | A |
|---|---|
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Related tickets | fixes #5561 |
| License | MIT |
|
Thank you Viorel! :) |
|
uw :) |
| $queryBuilder = $this->createQueryBuilder('o'); | ||
| $queryBuilder | ||
| ->innerJoin('o.taxons', 'taxon') | ||
| ->andWhere($queryBuilder->expr()->eq('o.root', ':root')) |
There was a problem hiding this comment.
I may be missing something, but I cannot find root field on any Product mapping.
There was a problem hiding this comment.
Tests may pass as this method is only used in OrmFinder in SearchBundle.
There was a problem hiding this comment.
that's correct. it should be 'taxon' instead of 'o'. my fault!
There was a problem hiding this comment.
Anyway, for nested set tree, left / right conditions should be enough - https://en.wikipedia.org/wiki/Nested_set_model. Moreover, we can replace this:
->andWhere($queryBuilder->expr()->orX(
'taxon = :taxon',
':left < taxon.left AND taxon.right < :right'
))
with:
->andWhere(':left <= taxon.left AND taxon.right <= :right')
But there should be some automatic tests made before (if we need this method at all).
There was a problem hiding this comment.
As far as I can tell they are not enough. You can have left and right value which match the condition from above but the node actually have a different root. I came up with this fix because in sylius admin we found some products being listed under a taxon even they were never assigned to it. Of course it's not easy to replicate this issue but it happens.
There was a problem hiding this comment.
If parent's left is always smaller than child's left and parent's right is always bigger than child's right, there is no way that this issue may exist (assuming that nested set tree implementation is correct).
Which version do you use? I remember that there were some issues when we used both nested set tree and softdeletable back in the days. (This may still exist in newer versions if using the same database then and now).
There was a problem hiding this comment.
Tested on sylius 0.18. Also tested on fresh sylius instance.
There was a problem hiding this comment.
Anyway, I created a new pr for the error you spot.
https://github.com/Sylius/Sylius/pull/5676/files
There was a problem hiding this comment.
Isn't quite the same thing?
