Skip to content
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

[4.0] Adding a node object interface #23364

Merged
merged 7 commits into from
Jan 4, 2019
Merged

Conversation

Hackwar
Copy link
Member

@Hackwar Hackwar commented Dec 28, 2018

This adds an interface for node objects. For now this only adds the interface, but these are supposed to be applied to CategoriesNode, MenuItem and a class for tags in the future. And of course wherever else they would fit.

@rdeutz
Copy link
Contributor

rdeutz commented Dec 29, 2018

looks good me, maybe a getRoot function could make sense

@Hackwar
Copy link
Member Author

Hackwar commented Dec 29, 2018

Adding a getRoot method would mean that the objects need to know about the whole tree and not just their direct children and their parent. I'd like to keep it as simple as possible and each node by default would only need to know the bare minimum.

@rdeutz
Copy link
Contributor

rdeutz commented Dec 29, 2018

ok, then you have to use getParent() so often till you not get a parent. Is also a way to go.

@mbabker
Copy link
Contributor

mbabker commented Dec 29, 2018

public function getRoot(): object
{
    $root = $this->getParent();

    if (!$root) {
        return $this;
    }

    while ($root->getParent()) {
        $root = $root->getParent();
    }

    return $root;
}

@Hackwar
Copy link
Member Author

Hackwar commented Dec 29, 2018

So, is the consensus that you want such a getRoot method? I personally would want to keep it rather simple and leave such methods out which can be created with the other methods of the interface. But in any case, just say which way to go and I'll adapt it. It would just be nice to move forward with this.

@mbabker
Copy link
Contributor

mbabker commented Dec 29, 2018

Personally I'd say add it, getRoot() is a rather simple thing and a similar thing does exist in most other tree oriented packages I work with.

@Hackwar
Copy link
Member Author

Hackwar commented Dec 30, 2018

getRoot has been added. Can we simply merge this or do we need "testers"?

@rdeutz rdeutz merged commit eeade14 into joomla:4.0-dev Jan 4, 2019
@zero-24 zero-24 added this to the Joomla 4.0 milestone Jan 4, 2019
@Hackwar Hackwar deleted the j4nodeinterface branch April 27, 2019 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants