-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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] Introduce category factory #20693
Conversation
I need to get this one merged to finish the service conversion of com_contact, com_newsfeeds and com_categories. |
I have tested this item ✅ successfully on 582fa3e This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20693. |
I don't really want to name this CategoriesInterface. The whole category system is a tree implementation, which is why I implemented a node interface here: #21327 I don't know how to name it in the tree-object-sense, but maybe use something that more aligns with tree terminology, instead of focusing heavily on the categories stuff. In an ideal world, JMenu would implement the same interface and menu items become a NodeInterface object, too. |
I agree for the tree, but the object which returns the tree is category related. So it makes sense to name it |
*/ | ||
public function get($id = 'root', $forceload = false) | ||
public function get($id = 'root', $forceload = false): CategoryNode |
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.
Technically this is a b/c break for people who are subclassing JCategories and overriding this method.
Having said that I doubt people are overriding this method so it's probably ok. any thoughts?
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.
Agree, lets revert it then.
* | ||
* @since __DEPLOY_VERSION__ | ||
*/ | ||
public function setOptions(array $options); |
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.
Do we really want this in the interface? I think the options setting is specific to the implementation?
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.
In the router you get a Category Injected and then you want to fetch the children with access check and without, so we need it there, or we inject there then the factory.
Can we get this merged @wilsonge so I can finish the service provider story? |
c4f66c1
to
bda3a8b
Compare
I know it's annoyingly retrospective. But this looks good to me. |
As discussed on skype, moves the category instance creation code into a factory.
I'v also extended it with a new
CategoryInterface
class so we do not have to deal with the implementation class directly.