Support for tools in packages

[ChrisHougard]

No description provided.

[aembler]

Ergh...do we want to do this? I only ask because tools are deprecated in favor of Routing (although I say that fully aware that we have them implemented in the application/ directory.)

[Mnkras]

I personally am not a fan of them, I always saw them as a security risk.

[ChrisHougard]

I tried using tools because of #1736. I didn't want to create a dedicated controller for what I was doing.

I'm ok with closing this and not allowing packages to use tools, although we should remove all of the references to tools in packages that exist in the core currently such as the URL builder so it is clear they're not provided.

On the other hand, I feel that since tools in the application folder will work, having support for tools in some areas and not in others could be confusing.

[jamesshannon]

+1

I'm consider lack of tools package support to be a blocker to releasing migrated add-ons.

Deprecated is nice and all (and ironic), but current support for tools-like functionality in a package seems to be pretty weak. ("Just treat them like a normal page, create a URL scheme that won't conflict with other packages, manually register a route for them, set up a controller function, and then create a view... or figure out how to return JSON.)

In the interim -- for package migration -- having an easy(ier) upgrade path is valuable.

[goutnet]

I personally vote for eradicating tools once and for all (and from everywhere).

[joe-meyer]

I think @EC-Chris makes a good point. The fact that it's supported in some places and not others gives an inconsistent feel to the application. I personally would like to see these disappear completely.

[Remo]

I'd definitely vote for all or nothing. Having them in the application directory and not anywhere else feels half baked. Personally I'd remove them as quickly as possible, otherwise we'll keep on having the old stuff around forever. Better teach devs how to do it right from the start.

[goutnet]

Well, now that I look at the code, there still is quite a lot of tools around (file manager etc…), shouldn't we do a bit of refactoring here ?

[jamesshannon]

I vote for all or nothing, too. Largely because if it's "all" then there'll be some best practices and helpers built around it for a standardized approach.

From what I've seen, creating a controller/view from a tool isn't trivial (or else the core would probably have migrated all of theirs), so I don't expect the "all" approach to be taken in the medium term. Considering that, I don't think it makes much sense to say "we're not going to support (or fix) this thing that was recommended in the previous version, which we now consider deprecated, but that we still support (and use) for core (and possibly site) purposes". Especially, as I said above, that there is no best practice / recommendations around tool replacements.

[jamesshannon]

FYI, The tools route is broken in that it no longer allows -'s in the filename: https://www.concrete5.org/index.php?cID=693585

[goutnet]

Well, that's one more reason to remove tools then.

BTW, along the way of #537 I am already refactoring a bit of the file manager to remove a few tools. Maybe one day we will see a complete 'clean' version with no tools anymore :)

[KorvinSzanto]

I'm with every one else other than maybe jshannon. Tools should be removed in favor of explicit route declaration.
@jamesshannon setting up a stateless procedural "tool" is as easy as:

\Route::register(
    '/some/unique/path/to/my/tool', 
    function() {
        $foo = new Foo;
        $foo->bar();
        $response = new JsonResponse($foo);
        $response->send();
    });

You can see a reasonable example of a modern package here: https://github.com/Buttress/addon_composer_sample

[jamesshannon]

@KorvinSzanto Thanks for the code. I ended up using the more typical register('/path/', 'Namespace\Class::function'); approach, and dealing with creating a response object and setting the json headers manually (because there's no json c5 class).

If you're going to cut out legacy tools (which seems a bit unfair considering that the core doesn't), then I still think there needs to be some best practices around this since "tools" will continue to be a standard use case. E.g.:

1. What's /some/unique/path? I ended up using /57tools/package_name. But since there's no standard around this, I'm sure some people will set up /my/tool.
2. No equivalent to getToolsPath(). If you make up your own manual path (above), then not a big deal, but not super DRYy.
3. It'd be nice if there was a Src\JsonResponse. The current JsonResponse isn't an instance of the src\Response so you can't simply return it.

[goutnet]

Well there is kind of something like that, get a look at the "Likes this block" package from the core :)

[KorvinSzanto]

@jamesshannon

1. It's in the hands of the content creator, so who cares?
2. You don't need to the tools path, because it's no longer implicit.
3. This is more an issue of the router not handling valid objects since we want to be able to set cookies from our \Cookie instance within a response. There is an open issue for this.