Fixed #8252 - circular references in location parents #8253
Conversation
There was a problem hiding this comment.
This is excellent, and something we have been thinking about doing for a long time!
Would you be interested in making some changes to this? Well, to be honest, a decent number of changes - but most of them pretty easy. We've been thinking about this for a while and I figured we can catch a few more error situations with this validator, and I think it won't be that big of a shift.
My concern is if we have location A has 'child' B, and location B has 'child' C, and location C has 'child' A -
A->B->C->A ...
That's still a circular reference, but I don't think this code would catch it.
The idea I had was that we could collect an array of ID's as we're going through the main while loop, grabbing the .id attribute of any item we iterate through. If we find any parent_id that's in that array, then we can fail the validation. Additionally, as a failsafe, it'd be really cool to have an integer that we increment every time we go through the loop - and if we get above, say, 50 - also fail the validator. (A sort-of "Belt-and-Suspenders" approach). We should also try and catch if something is being set to be its own parent ( A->A) - we do that now in the controllers but would love it if we could yank that out and have the Validator handle it for us - so much cleaner!
Also, one thing I really love on your PR is that you handled language files - that's not common with most of the PR's we get! We'd like to approach it a little differently, though. We use a platform called CrowdIn to handle our translations, so we'd like to just see the English translation there, and leave the rest out - CrowdIn will fill them in for us. We may end up tweaking the language for that, as "Circular Reference" is certainly a normal Computer Science phrase, but our users are mostly IT people and that phrase may not make as much intuitive sense for them. But we'll handle that part, so you can leave the error message as-is. If you can just yank the non-English translations that'd be perfect.
Again, thank you so much for this PR, this is something we've had in our minds for a while now, we just never found the time to actually get it done.
|
|
||
| // If we’re editing an existing model and there is a parent value set… | ||
| while ($data_pk && $value_pk) { | ||
| // It’s not valid for any parent id to be equel to the existing model’s id |
travismiller
force-pushed
the
locations-parent-validation
branch
from
d10724b to
dbdc8fd
Compare
|
@uberbrady thanks for the quick review and great feedback! I do believe this implementation already handles your concerns about For For I chose not to collect all of the IDs for all parents of the selected parent since that would introduce extra queries when it can be determined early in a longer chain. For example Another approach I had in an early implementation included building a tree for every location in the system with their child and parent relationships in a static memo-ized method on the Location class. However I felt it was beyond the scope of just handling this validation. Another thought is that an invalid Location should not even be a select-able option. They should either be hidden, or ideally visible but disabled. Happy to continue working on this until it's where it needs to be. 👍 |
There was a problem hiding this comment.
Ok, I tested this and it definitely worked; does what it says on the tin. I'll check with @snipe to see if she's OK with the wording (my concern being that "circular reference" isn't very intuitive to non-computer-science people) but if she signs off on that, we're good!
…parent_id - rebased from #8253 (#8927) * Fixed #8252 - circular references in location parents * Remove non-translated translation changes * Fix typo * Add loop limit to avoid unforseen infinite loops * Remove check against parent_id in location controllers * Remove the Location->id=null piece (no longer needed) * Fix some formatting and whitespace * Re-introduce accidentally merged-out language file Co-authored-by: Travis Miller <[email protected]>
|
This one was resolved with another PR, thank you! |
This adds and uses a custom validator to prevent circular references in Locations.
Issue raised in #8252.