Controller Routing in Vanilla 2.5

Shadowdare

Currently working on updating Articles to support Vanilla 2.5 (see GitHub repo for latest dev code). Ran into issues with routing. Is anyone else having this issue with Vanilla 2.5?

If you're familiar with Vanilla's routing system, you already know you can get to a method via /appName/controllerName/methodName/(...), with or without appName in the link (and without methodName if it's the index). In this case, we have the ArticlesController within an app called Articles.

• This link shouldn't show Vanilla's All Categories page, but it does: http://example.com/articles/categories
• This link doesn't work too: http://example.com/articles/category/general. It seems like the router thinks you want: http://example.com/category/general (same as /vanilla/category/general).
• Instead, if you prefix the app name (key) and go to http://example.com/articles/articles/categories and http://example.com/articles/articles/category/general, these links work.

I don't know what would happen if I rename ArticlesController to something else and try to go to the same Articles category page (this would indicate a conflict between the app key and controller name).

My guess is that there's a conflict between app keys and method names that is being handled differently in Vanilla 2.5 than older versions. Is there a difference in how 2.5 handles routing?

If you don't see this issue, then it could be an issue in my dev environment's nginx configuration, but I did try different settings to no avail.

R_J

1. I have tested in my server and the error is the same => no server setup problem
2. I have tested example.com/discussions/categories and example.com/plugin/categories and got "regular" errors => I would say you have done something wrong in your application, since this only happens to the articles controller

Shadowdare

@R_J, thank you for testing this out! I wish I could click both Insightful and Awesome for your comment!

All the routes worked fine with Vanilla 2.3. Anyway, I've been staring at my nginx config for hours today, so I better take a break and come back to ArticlesController later

R_J

The dispatcher uses the following method:

    private function findController($parts) {
        // Look for the old-school application name as the first part of the path.
        if (in_array(val(0, $parts), $this->getEnabledApplicationFolders())) {
            $application = array_shift($parts);
        } else {
            $application = '';
        }
        $controller = ucfirst(reset($parts));

        // This is a kludge until we can refactor- settings controllers better.
        if ($controller === 'Settings' && $application !== 'dashboard') {
            $controller = ucfirst($application).$controller;
        }

        $controllerName = $controller.'Controller';

        // If the lookup succeeded, good to go
        if (class_exists($controllerName, true)) {
            array_shift($parts);
            return [$controllerName, $parts];
        } elseif (!empty($application) && class_exists($application.'Controller', true)) {
            // There is a controller with the same name as the application so use it.
            return [ucfirst($application).'Controller', $parts];
        } else {
            return ['', $parts];
        }
    }

The first part splits the application from the url and therefore the behaviour is different for /someApplication/someMethod and /rubbish/someMethod. Therefore my second assumption above is wrong. It is not your controller which is causing the problems (at least not the code in the controller)

Since the first part is an application, the second must be a controller, in this logic, and that's why the code searches for a class called CategoryController. That certainly exists as part of Vanilla and that is what is presented to you.

If you call example.com/articles/articles/categories instead, the first part is the application, the second the class and the third the method and everything is working as expected. Looking at it from that perspective I would say that this is the correct and expected behavior. Maybe you either call that ArticlesController differently or you add an internal route from /articles/* to /articles/articles

Linc

I asked a developer to review this and this was the resulting issue: https://github.com/vanilla/vanilla/issues/6378

Shadowdare

I'll be following that issue and will be interested to hear if the new dispatcher logic refactoring needs a fix or is working the way the team intended it to. If the latter, I'm all for implementing workarounds.

Is this related to the reason Vanilla's SettingsController was renamed to VanillaSettingsController?

Linc

@Shadowdare said:
Is this related to the reason Vanilla's SettingsController was renamed to VanillaSettingsController?

You can no longer use the same controller name in two different addons. That was definitely intentional; it was a performance drag to let multiple addons share. I'm unclear if this other change was intentional.

R_J

@Linc said:

@Shadowdare said:
Is this related to the reason Vanilla's SettingsController was renamed to VanillaSettingsController?

You can no longer use the same controller name in two different addons. That was definitely intentional; it was a performance drag to let multiple addons share. I'm unclear if this other change was intentional.

Isn't that a bad idea? If Shadowdare creates an articles applicaion which would allow users to think in the known pattern of categories and x00 creates a marketplace with categories and someone will write a calendar application with categories and someone else would write a gallery with categories, that would end up in the following routes:

example.com/vanilla/categories
example.com/articles/articlescategories
example.com/marketplace/marketplacecategories
example.com/events/eventscategories
example.com/gallery/gallerycategories

Simply naming a controller after its function wouldn't be enough because you would always risk incompatibilities. So a plugin author would be forced to prepend something more to a controller name.

That looks really ugly. On the source code layer that problem is solved by namespaces. There is no reason not to have numerous CategoriesControllers all over Vanilla - that's what namespaces are made for.

The restriction now purely comes from the routing side. Pitifully I am no developer and whenever I try my hands on namespaces I fail miserably, although I'm convinced it is a super easy principle. But shouldn't it be possible to take namespaces into account when routing?

And if that's not possible, what about forcing a meaningful pattern like
application/controller/method/param/param/param/...?

That would certainly speed up routing (and take away pains from "/profile/(username|controller)")

Combine that with some "forbidden" application names because they are used for internal routes (most of the simple routes that are available right now):
"discussion" -> "vanilla/discussion",
"discussions" -> "vanilla/discussions",
"categories" -> "vanilla/categories"
"profile" -> "dashboard/profile"
...

---

I remember that some time ago I have read about dynamic routing as something which should be not used. I haven't wasted much brain power on that but simply thought to myself that I love the easy way I can create "new routes" in Vanilla by simply providing an end point.

Now I see where a static routing table has advantages. That would be much faster and the problems above not have happened.

Is that something which has been considered when the routing flow has been talked about?

Linc

The performance issue is very real. The situation where everyone wants "categories" in non-forum contexts is very hypothetical.

Is it worth a bunch of developer time to augment the dispatcher in a way that allows applications to overlap controller names? What value does that deliver to us? What cost is there in all the maintenance of all the extra code required to do that? Why would we want people to name things "categories" that have nothing to do with the platform's feature "categories"? How is having a duplicate controller better than extending the existing one?

Is "don't re-use core controller names" that onerous of a thing that we should build around it? Aren't there many other "gotchas" that we should spend time addressing rather than this one?

In the abstract, sure, that'd be great to resolve all those issues and let any developer use whatever URL namespace they want. In reality, the money required to spend the time to do those things with good performance is very high, and the return is nearly non-existent.

x00

personally I favour explicit routing over implicit any day.

although the implicit system vanilla uses is kind of of neat, frameworks with explicit routing are more flexible/adaptable and the routing can be two way, due to having a mapping you can plug them into your url functions and change url scheme with instant effect. Also less performance issues or conflicts.

there are ways to have you cake an eat it implicit and explicit.

Shadowdare

Implicit routing seems to work better for the extensible nature of Vanilla, although explicit routing could work as well. A good example of how explicit routing has been implemented would be Microsoft's ASP.NET MVC framework and its Areas and Routing middleware.

Namespaces would be good to have for extensible frameworks such as ours; not just for controllers, but for all classes. A developer who is working on an app totally unrelated to other addons wouldn't have to worry about ones that may use the same name for a class.

In general, a lot of routing issues could be solved if every link in Vanilla were prefixed with the app name in the URL in addition to having classes organized by namespaces. However, I like how it's not required in Vanilla as the URLs look cleaner that way, especially since the Vanilla app provides the main forum functionality. Rewriting the entire dispatcher and routing system would likely break a lot of links and be expensive to do, so we best work with how Vanilla does it all, which isn't exactly the wrong way to do it IMO.

Regarding the aforementioned issues, /articles/category/general shouldn't take you to /category/general as having "articles" in the path should be recognized as the ArticlesController, unless the dispatcher thinks it's the app name since it has the same name and falls back to finding the only existing CategoryController. However, this may just be the preferred behavior in this case, depending on how the dispatcher and routing system is designed to work.

Perhaps we can add internal route rules in Vanilla to workaround some of these issues like R_J mentioned earlier since having /articles/articles/categories or /articles/articlescategories would be ugly. For back-end pages, it doesn't matter what the URL looks like, but it should look good for front-end pages.

Also, a more extreme solution could be to have articles be a new discussion type in Vanilla as has been discussed in the past, but this way only covers the CategoriesController issue and has limitations of its own. It makes sense to try to integrate custom apps into Vanilla as much as we can, but apps should be able to run independently of the Vanilla app even if it's enabled simultaneously.

Since the names of classes are used for implicit routing, I agree that addons should not have the same names for controllers as the core code. The developer can prevent this by using a synonyms or restructuring their methods within other controllers. Consequently, we break consistency from Vanilla's design once we introduce synonyms. For example, one addon may have "sections" that serves the same purpose of "categories," but other developers working with the code may get confused.

This doesn't take into account other third-party addons that may end up using the same names as well, so finding a good solution for these issues is quite complicated.

x00

there are plenty of extensible framework with explicit routing. It is simply a lot more flexible an adaptable.

in fact dependency injection is also often done with explicit mapping, precisely for extensibility purposes. e.g symphony

x00

Like i said it is not impossible to force a change of route, done it a few time, just not particularly intuitive.

charrondev

I’ll say that internally we had routing issues with Gdn_Dispatcher that were only partially resolved with Garden\Web\Dispatcher in APIv2. We’ve been looking into using the Symphony 4 router or other routers for an upcoming release, so we should have a better story for this soon.

Shadowdare

@charrondev (https://github.com/vanilla/vanilla/issues/6378#issuecomment-469517894):

I'm going to sum this up as don't use Gdn_Controller & Gdn_Dispatcher any more. If you do follow the rule specified by @linc here.

New solutions are Garden\Dispatcher & Vanilla\Web\Page.

Are there any examples of Garden\Dispatcher or Vanilla\Web\Page in action for non-API endpoints currently?