Skip to content

[4.0] Adding a node object interface#23364

Merged
rdeutz merged 7 commits intojoomla:4.0-devfrom
Hackwar:j4nodeinterface
Jan 4, 2019
Merged

[4.0] Adding a node object interface#23364
rdeutz merged 7 commits intojoomla:4.0-devfrom
Hackwar:j4nodeinterface

Conversation

@Hackwar
Copy link
Copy Markdown
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
Copy Markdown
Contributor

rdeutz commented Dec 29, 2018

looks good me, maybe a getRoot function could make sense

@Hackwar
Copy link
Copy Markdown
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
Copy Markdown
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
mbabker reviewed Dec 29, 2018
View reviewed changes
@mbabker
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 mentioned this pull request Jan 4, 2019
Merged
@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

Reviewers

1 more reviewer

@mbabker mbabker mbabker left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Documentation Required

Projects

None yet

Milestone

Joomla 4.0

Development

Successfully merging this pull request may close these issues.

6 participants

@Hackwar @rdeutz @mbabker @wilsonge @zero-24 @joomla-cms-bot