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

Routing: Implementing JComponentRouterAdvanced #5444

Closed
wants to merge 25 commits into from

Conversation

Hackwar
Copy link
Member

@Hackwar Hackwar commented Dec 16, 2014

This PR implements a new base class for component routers, which can be used to create rules based routers that are dynamically extendable.

The idea

Components that stick to the Joomla MVC style should be able to create a fully working router with as little work as possible. At the same time, component routers should only handle views, since those are the only ones that you should be able to link to. If you have something that is not a view, it should execute and then redirect to a view again. This means that we only have to create SEF URLs for views. Views are pretty well defined, too.

How it works

A router extends from this class and in its constructor, it registers all its views with a bunch of its data and the hierarchy between them. The idea is, that views form a tree hierarchy, where traversing along the tree towards the leafs gets you higher and higher in the hierarchy. In case of com_content, the articles and its article view would represent the leafs of the tree, the categories with their category view represent the nodes of the tree and the categories view, which displays all categories, would represent the root of the tree.
So we already have a kind of hierarchy in our views and we register them accordingly in our router. We start with the categories view. That has no id, has no parent view and is not nestable. Then we have the category view, which has the ID "id" in the query, has the categories view as a parent view and that view is nestable. This means that while traversing along the above described tree, we can have a node that is a category view and its child-nodes are also category views.
Based on this, we can then find the right Itemid for example (by starting from the node given by our query, for example the article view and its ID and traversing along the tree to its root, all the while comparing the menu items that we have with the current node that we are in) and/or add the category hierarchy from that menu item to the URL and last but not least add the article slug to it.
Since we are not registering "we have categories and content items" but a flexible hierarchy, you could in theory go as far as having categories that contain tags which again contain content items and those contain something else again. So the hierarchy is not limited to the category/content-item system that we have in Joomla right now, but allows for a lot more.

A specific example could look like this:

    public function __construct()
    {
        $categories = new JComponentRouterViewconfiguration('categories');
        $this->registerView($categories);
        $category = new JComponentRouterViewconfiguration('category');
        $category->setKey('id')->setParent($categories)->setNestable()->addLayout('blog');
        $this->registerView($category);
        $article = new JComponentRouterViewconfiguration('article');
        $article->setKey('id')->setParent($category, 'catid');
        $this->registerView($article);
        $this->registerView(new JComponentRouterViewconfiguration('archive'));
        $this->registerView(new JComponentRouterViewconfiguration('featured'));
        $this->registerView(new JComponentRouterViewconfiguration('form'));

        /** Insert adding rules to process the URLs here **/
    }

The rules

Based on the hierarchy that you have to define above, we can then write rules that process the input query to transform it to the SEF URL that we want. You can see a rule that finds the right Itemid for a query in #5446. This means, that we don't need a component specific code for this in the ContentHelperRoute classes.
Similarly the current routers behavior can be combined into a generalized rule that works of of the hierarchy that was described above. That means, that we can summarize all the code in our core routers in those rules and instead of having 11 component routers, we would only have one rule that implements this specific logic. You can see this in #5533.
I have to admit, those rules get pretty complex and difficult to maintain, but it would mean that no component developer needs to write his own router anymore, but can use the core rules instead.

The component router

A specific component router could simply consist of the code that I posted above. Adding a plugin event to it and some further logic, and we would specifically only have this:

/**
 * Content Router
 */
class ContentRouter extends JComponentRouterAdvanced
{
    function __construct()
    {
        $categories = new JComponentRouterViewconfiguration('categories');
        $this->registerView($categories);
        $category = new JComponentRouterViewconfiguration('category');
        $category->setKey('id')->setParent($categories)->setNestable()->addLayout('blog');
        $this->registerView($category);
        $article = new JComponentRouterViewconfiguration('article');
        $article->setKey('id')->setParent($category, 'catid');
        $this->registerView($article);
        $this->registerView(new JComponentRouterViewconfiguration('archive'));
        $this->registerView(new JComponentRouterViewconfiguration('featured'));
        $this->registerView(new JComponentRouterViewconfiguration('form'));
        parent::__construct();
    }
}

This would be all that is needed for our core routers. There are however some things that I would propose for the future and that would allow to make the system more robust and a bit more flexible.
In order for the system to be able to retrieve the right data, I think it would be good to introduce router specific getters for things like the article ID/slug. So a com_content router would have a method getArticle() that either returns the ID of the article that the slug and the category ID belongs to or that returns the ID with the slug attached, based on the input. I'm not sure about the correct interface here yet. The getCategory() method is a generalized version for all category related views. The system that I have in mind expects an interface like that from JCategoryNode for all nestable views. Since I don't like the JCategoryNode implementation very much anymore, since its too category specific, I started a discussion on a node interface in #5227. Feel free to join in there for this part.

getPath()

The getPath() method in this class returns a path of views from the node/leaf that is given by the query to the root of the tree-hierarchy. This allows us to simply check if each entry for example corresponds to a menu item and thus set the right Itemid or to add a segment to our url from that matching menu-path item to our original node/leaf defined by the query. It's a helper method that I think a lot of rules will make use of.

Current issues

Since there is no code yet in this PR to test this with, I have to point you to #5446 and #5533. I'd also like to ask for a code-review by the maintainers and merging this based on that code review.

This was made possible through the generous donation of the people mentioned in the following link via an Indiegogo campaign: http://joomlager.de/crowdfunding/5-contributors

$r = null;
if (!preg_match('/(.*)Router/i', get_class($this), $r))
{
JError::raiseError(500, 'JLIB_APPLICATION_ERROR_ROUTER_GET_NAME');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use Exceptions rather than JError in the new code please :) I presume a 500 error throws the error page. So we can use JErrorPage::render() if we need output the error page with exceptions

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will change that.

@johanjanssens
Copy link
Contributor

@Hackwar Thanks, read the proposal and added a first set of comments on the code as proposed. One first question though.Why did you choose the proposed implementation using a path based solution for routing over a regex based routing approach ?

@Hackwar
Copy link
Member Author

Hackwar commented Dec 17, 2014

First of all, I want this advanced router to be optional. If a component does not want to use this router or does not fit into the schema proposed here, they can still use their own implementation. This also means that the behavior up until the component router is called, has to be the same as now. Always remember the backwards compatibility that we have to adhere to in 3.x 😉 So we already have to find the right menu item when parsing the URL.

Then the goal also is to keep the same URLs for the system right now, so I can't start with a clean slate here and drop the menu path or category path completely.

Last but not least, I honestly wouldn't know how to write a regex for the system that we have. As far as I can see, you would need a different regex depending on the menu item that is associated with this query and that would make it rather ... difficult. Or we could have a regex that represents the hierarchy that you have to set up in your component controllers constructor, but that would only be a different representation of the same thing and considering MY issues with regex, I would opt for the simpler code solution in PHP for our developers.
To clarify: I looked at implementations like in AngularJS and Play and those are good for a complete application, but we are talking about components and not the whole application and I think that is where a regex solution would fall flat.

Last but not least, this approach with registerView allows us to extend a router dynamically. You could add your own view to a component (there are different ways to do that) and simply add that view into the component router via a plugin and the router would automatically adapt and take that new view into account.

*
* @since 3.4
*/
public function registerView($name, $view, $id = false, $parent = false, $parent_id = false, $nestable = false, $layout = 'default')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would store the views in a specialized class JComponentRouterView here instead of stdClass. This way you can pass around the objects easily and you can also provide default getters for some values. For example $name and $view will almost always be the same.
In fact, you could move most of the logic here to the class constructor after that.
Then for the method signature you could do array|JComponentRouterView. A typical call would look like this:

registerView(array( 
    'view'   => 'article',
    'id'     => 'id', // can be made default 
    'parent' => 'category',
    'parent_id' => 'catid'
))

Another thing is I think id and parent_id should be renamed to key and parent_key. We don't use ID columns for routing in our components for example and id => alias would look confusing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the JComponentRouterView: I decided against another class here. My main reason would be the balance between proper abstraction and over-engineering. Trying to keep it simple here. That is also why I would not want to use the array()-parameter approach, since afaik it prevents the hinting in IDEs to work.

Regarding the id/key argument: I agree. I will rename that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Hackwar I agree with Ercan.

Having 6 parameters in a function is a bit of a code smell. Ideally functions have 2 max 3 arguments. If more arguments are needed a better approach would be to use a object to express the various arguments as Ercan suggests. This also makes the API cleaner and more declarative.

Give it some more thought :)

@johanjanssens
Copy link
Contributor

Hannes, thanks for the feedback. I agree with you that for Joomla a none regex based routing approach is better. Just wanted to hear if you did research the regex and what you decided.

@Hackwar
Copy link
Member Author

Hackwar commented Dec 29, 2014

I've added a JComponentRouterViewconfiguration class in regards to @johanjanssens and @ercanozkaya comments. I'm looking for some feedback from you guys, before I finalise this with proper comments and codestyle.

@Hackwar
Copy link
Member Author

Hackwar commented Dec 31, 2014

I've written unittests for all classes, rewritten a few of them again and removed the ability to have one view under several names in the system. I was hoping that I would be able to make this work, but it is logically impossible to solve this. It also has the benefit that this makes the whole implementation a bit simpler and removes a layer of indirection.

With that said, I'd ask you to go over this one more time, @johanjanssens, @mbabker, @wilsonge

I will update the PRs depending on this one asap.

@@ -7,8 +7,6 @@
* @license GNU General Public License version 2 or later; see LICENSE
*/

defined('JPATH_PLATFORM') or die;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we have this back please :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we have these in all joomla library files to prevent additional entry points.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that is simply a waste of bits and bytes. If you open this file in a browser, you get no content returned. If someone creates a second entry file and includes this file, he will in 99.9% of the cases also have added that constant... I mean, these are as usefull in just-classes files as the index.html files that we had...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those lines are a reminiscence of old days, when we included scripts instead of doing OOP. It is one of the characteristics of PHP legacy applications as listed in "Modernizing Legacy Applications In PHP" by Paul Jones, page 1, https://leanpub.com/mlaphp . Also see for instance kohana/core#508

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you wanna do that then we do it across joomla as a whole and you can do a PR for that. Until then leave them in please - let's not be sketchy across the CMS

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wilsonge I disagree. This is an old practice as @Hackwar and @HermanPeeren state. We can proactively remove this by making sure to not add it to new code. The lines that are still in can be considered deprecated and can be removed at any time.

@Hackwar
Copy link
Member Author

Hackwar commented Jan 4, 2015

As I've written before, this advanced router is an offering to developers. It should make their lives a lot easier, but they also will have to stick to the implicit structure that is required by this router. This means, that you have to use the views and nothing else. If you want to route by something else, you either have to write your own router like those that we have right now or write your own router rules. But that is the reason why it is called "advanced router". If that is not your style, you can still write a simple router, as long as it adheres to JComponentRouterInterface or as long as it is the 1.5 to 3.2-style function based routers.

@Bakual
Copy link
Contributor

Bakual commented Jan 4, 2015

I don't think I like this restriction myself, but I will likely need to do my own router anyway.

What about having multiple trees? Like for example I have in my component three different types of content types (sermons, series and speakers) where each of them has categories. But there is no shared root for them. So I would need to build three trees. I guess that's not possible with that router and I have to extend it or build an own myself?

@Hackwar
Copy link
Member Author

Hackwar commented Jan 4, 2015

You can have as many trees as you want. As long as the category view is not named the same for all 3, its okay.

@Bakual
Copy link
Contributor

Bakual commented Jan 4, 2015

Thanks, that's great!

@johanjanssens
Copy link
Contributor

@Bakual @wilsonge Routing in Joomla has always been 'view' centric, this is by design and for very good reasons. The views represents a resource.

The routing in Joomla is called resource routing, in this routing scheme the router matches the url to a resource. Ideally only the HTTP methods are used. Joomla 1.5 didn't do this yet, however it was build to move towards this.

None resource routing is possible with Joomla, but not implemented in the core. In this routing scheme a url would be matched to an action on a controller. None resource routing should be avoided as much as possible.

For more info on both also read the Rails routing documentation which covers both forms of routing in details. In Rails like in Joomla resource routing is the default.

@Bakual An additional note about &task=file.download you mentioned.

This is one of the bigger architectural mistakes that have crept in Joomla after 1.5. This change devaluated Joomla back to the swamp of POX, or level 0 following the Richardson Rest maturity model

The task variable includes not only the action to be performed it also includes the controller to call the action on. You cannot go much worse then that.

Whoever did this, obviously had no clue how the web works and set a very bad example for extension developers. I see many extensions to day who implement tasks like this. As @HermanPeeren correctly points out the change is one of the main reasons why Joomla still doesn't have a proper REST interface.

In short

Joomla 1.5 was build specifically to be able to work at level 1 of the RMM model; being the level of resources. A 'view' represents a resource, and the router is responsible to find the correct resource/component pair. Routers are 'view' centric, not controller centric, from the view the controller is found, not the other way around.

In future to move towards level 2 and 3 of RMM and to become fully Hypermedia aware Joomla should get rid of the 'task' URL query parameter and adopt the HTTP methods as the default actions across all of it's components. This will allow components to become RESTful out of the box.

In your example of &task=file.download the correct request would be GET /downloads/myfile.doc HTTP/1.1which is level 2 in RMM.

Example

I often see developers claim that using only the HTTP methods doesn't work for their extension. I disagree.

In Nooku we have adopted a BREAD (browse, read, edit, add, delete) paradigm for all controller actions. Nooku's http dispatcher maps HTTP methods to controller actions on the fly. We have found that we can cover any scenario's with only those 5 actions.

Notes

New methods are also being proposed and added to HTTP

In those very few scenario's where the default HTTP methods are insufficient WebDav could offer a solution.

@hannes Good work on the routing changes.

/**
* Views of the component
*
* @var array
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For improved typehinting, you can make this @var JComponentRouterViewconfiguration[] which will tell IDEs and tools like phpDocumentor that the container is an array of JComponentRouterViewconfiguration objects.

@Hackwar
Copy link
Member Author

Hackwar commented Jan 21, 2015

I've implemented the feedback from @johanjanssens and @mbabker into this last commit. Please review this. If this is accepted, I'm going to update the other PRs.

@johanjanssens
Copy link
Contributor

@Hackwar Good work. I appreciate the name change to JComponentRouterView. No immediate and urgent remarks from me on the PR. Will do one final pass tonight.

@wilsonge wilsonge added this to the Joomla! 3.5.0 milestone Jan 21, 2015
*
* @since 3.4
*/
class JComponentRouterViewconfiguration
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Hackwar I noticed that all of the class properties are still public ? Is that by design ? You have getters and setters for some. Shouldn't those be at least protected ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not want to make this class more complex than necessary, so I didn't want to add 10+ getter-online-functions. I know that it might not be by the book, but I would weigh the gain of a little bit of added code-security against the overhead of having all those getters, both in LoCs and in function calls.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but you cannot do both. Have a specialised setters and allow public access to the property. For those properties where you have specialised setters you need to ensure the property cannot be set directly, otherwise you create ambiguity in how the class can be used.

So I would still advice to make those properties that have setters private or protected and add getters for them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having worked with Symfony for a bit, their approach of "most restrictive access" first has worked really well for me. If these are properties which you need to guarantee are a certain object type or certain values, I'd restrict access and use get/set methods. If the properties are OK being freely modified and doing so won't cause massive issues, they can be public, but there's always a risk in doing so of a value being overridden incorrectly. It might cause a bloated class with extra 1-5 line methods, but the benefits outweigh the issues IMO.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I don't wanna... 😜

Ok, I will implement getters for this. I've got a lot to do until end of next week, but I will tend to that then.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Hannes! Good call.

@johanjanssens
Copy link
Contributor

@Hackwar Added 2 more points of feedback. Things are shaping up very nicely.

@Hackwar
Copy link
Member Author

Hackwar commented Mar 22, 2015

@phproberto I updated the branch to latest staging and added the "covers" notation to the unittests. Can we merge this now? 😄

@phproberto
Copy link
Contributor

Merged into 3.5-dev branch. Thanks!

@phproberto phproberto closed this Mar 26, 2015
@Hackwar
Copy link
Member Author

Hackwar commented Mar 26, 2015

Thank you!!

roland-d pushed a commit that referenced this pull request Aug 6, 2015
Call-time pass-by-reference has been removed

Removing JError, using Exception instead

protecting $name and renaming register() to registerView()

Adding removeRule, getRules and renamed $id to $key in register method

Making method names consistent

Implementing JComponentRouterViewconfiguration for configuration of views in JComponentRouterAdvanced

Codestyle, smaller improvements, unittests for all component router classes except for JComponentRouterAdvanced

Removing ability to have one view with different names and implementing unittests for JComponentRouterAdvanced

Adding get<View>Slug() and get<View>Key() methods to JComponentRouterAdvanced

Updating unittest

Small fixes

Adding back in platform check

Adding back in platform check

Adding back in platform check

Adding back in platform check

Implementing feedback so far

Adding "covers" notation for unittests
@Hackwar Hackwar deleted the componentrouteradvanced branch January 6, 2016 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants