New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implemented naturalPrev function in Hierarchy class #3789
Conversation
} | ||
|
||
// if this is not an instance of the root class or has the root id, search the parent | ||
if(!(is_numeric($root) && $root == $this->owner->ID || $root == $this->owner->class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need some more brackets here to group the conditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this enough?
// if this is not an instance of the root class or has the root id, search the parent
if($root != $this->owner->ID && $root != $this->owner->class && ($parent = $this->owner->Parent())) {
// ....
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
&& $parent->exists()
I think this needs to go against 3 branch |
This also needs tests |
This actually needs to go against master as it's a breaking API change... even though it was never implemented :( |
if(!(is_numeric($root) && $root == $this->owner->ID || $root == $this->owner->class) | ||
&& ($parent = $this->owner->Parent())) { | ||
|
||
return $parent->naturalPrev( $className, $root, $this->owner ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spacing inside parenthesis shouldn't be there
@tractorcow is there any value in someone picking this up? I don't even know why there's a function stub for this |
We should just remove the stub in 4.x. |
This has been marked as "todo" since forever. I just need it for a project.
naturalNext
function doesn't have unit tests, so they both need unit tests at some stage.Only tested with parent->children relationship so far, rather than deep nesting.