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

RFC: Use Symfony Console Component for Tasks and better cli support #5542

Closed
axyr opened this issue May 16, 2016 · 36 comments
Closed

RFC: Use Symfony Console Component for Tasks and better cli support #5542

axyr opened this issue May 16, 2016 · 36 comments

Comments

@axyr
Copy link
Contributor

axyr commented May 16, 2016

Author: @axyr
Status: Draft
Version: 0.1

Introduction

Currently we can run Tasks or call (cli)Controllers on the command line with sake dev/build or calling php framework/cli-script.php dev/build.

While when we are on the command line, we can do much more to control and develop our website.

Symfony provides a very nice component to interact with our application on the command line.

http://symfony.com/doc/current/components/console/introduction.html

We can use the command line for much more then build tasks, and the Commands can be called from code as well.

We can still use dev/tasks from a browser to call the newly create TaskCommand and maintain backwards compatibility.

It becomes more easy to run Tasks/Commands from code and reuse pieces of functionality.

With commands we can create code scaffolders like make:page MyCustomPage, run cron tasks.

We can display more help information and options for complex tasks on the screen, instead of only buried in the documentation.

It will be easier for developers to write small maintenance or development commands.

Organizing code in Commands can clean up code and improve reusabilty.

Purpose and outcome

  • Symfony Console component becomes a Framework requirement.
  • Tasks will be transformed to Commands
  • The command line will come in first when it comes to manage and craft our application.
  • Browser comes second as a ‘poor mans’ alternative for basic stuff.

Motivation

When writing tests, I find myself more time spending on the command line, in favor of refreshing my browser all the time. That makes me want having more tools on the command line that work and look good.

And since more and more people are familiar with git(hub) and composer for bootstrapping applications, the command line is more widely used and almost a requirement to start developing. Every module tells you to run composer require some/package.

The browser is only needed to flush and rebuild the cache (for now...?).

I did a lot of fiddling and find it very easy to extend and work with.

See for some examples. I use the Make Commands very extensive and it speeds up my boilerplating stage.

https://github.com/axyr/silverstripe-console

Now I will not say that all this is not possible with the current codebase of Silverstripe, but I think it can be done better.

Proposal

  1. All Task code will be moved to a Command.
  2. All Browser callable Tasks will call the newly created Command to preserve backwards compatibilty

Alternatives

I could not find a better console package then Symfony provides.

Impact

Sake will call the Commands instead of Controllers.
The syntax on the command line will change from :

sake dev/build flush=all to something like sake dev:build --flush=all

On the cli we can use enter sake to list all available commands. Do we want to have the same in the browser?

We might have Commands implement BrowserCallable to make them explicitly available for browser usage and not by default.
We need to find out how to refresh caches for browser usage from the command line, since this is not always (never?) the case.

The formatting of messages like in DB::alteration needs to be refactored.

We might want to think of a nice color scheme for our info, error and warning messages in a blue-ish coloring scheme?

We can get rid of a lot of Directory::is_cli(); calls that are used for formatting and permission checks. A lot of that logic can be handled by the commands.

This is a nice way to reorganize some code when we gradually move pieces of functionality to Single Responsibility Classes that can be called by a command (and current use cases where the code is used) and in other places if we need them.

Our live will become easier and we gain more power and flexibility.

@assertchris
Copy link
Contributor

I think it would be better for the RFC to remove the sake dev:build -f=a example from the RFC text. Shorthand options are a neat side-effect, but they may confuse the issue a little.

@axyr
Copy link
Contributor Author

axyr commented May 16, 2016

changed
sake dev/build flush=all to something like sake dev:build -f=a or sake dev:build --flush=all
to
sake dev/build flush=all to something like sake dev:build --flush=all

:) thanks

@sminnee
Copy link
Member

sminnee commented May 16, 2016

I like the idea of using the Symfony console tools. However I wonder if our work in this space should be done as a composer plugin that provides custom commands?

That way, users don't need to learn a new tool.

@axyr
Copy link
Contributor Author

axyr commented May 16, 2016

We could provide a wrapper to convert
dev/tasks/MyTask option1=2&option2=3
to
task:mytask --option1=2 --option3=4

So commandline and browser work the the same as it does now, with the option and power to use the new syntax.

Using the Symfony console syntax is more alike with 'normal' terminal commands that are also used for git and composer.

I think as soon as devs know they can use latter, a little cli experience is enough to recognize the syntax.

And people that are coming from symfony, yi, laravel or drupal, but want to use Silverstripe because they need a good CMS, will feel at home immediately, and don't need to learn a new tool?

@sminnee
Copy link
Member

sminnee commented May 16, 2016

This is what I'm talking about: https://getcomposer.org/doc/articles/plugins.md#command-provider

Then you could support

  • composer ss-task:<taskname>
  • composer ss-build

Without needing a tool other than composer.

I agree on the need for a wrapper - otherwise people will be stuck with non-converted tasks and be forced to pick between 2 tools.

@axyr
Copy link
Contributor Author

axyr commented May 16, 2016

Ah that way.

I will inspect the composer providers a little bit more! I was not aware of that.

But it seems just a wrapper around Symfony Console.

It feels a bit confusing to type composer to interact with silverstripe instead of composer.

And it requires to 'namespace' all commands with ss?

@axyr
Copy link
Contributor Author

axyr commented May 16, 2016

And it will require composer on live servers.

@stevie-mayhew
Copy link
Contributor

I don't like the thought of composer on live environments.

I am a big fan of the Symfony console component and think that it would be a great replacement for the sake command. It makes it a lot easier for us to input other commands into sake as modules as well which could be handy.

@axyr
Copy link
Contributor Author

axyr commented May 19, 2016

Me neither... it was an argument against it in addition to my previous comment ;)

@dhensby
Copy link
Contributor

dhensby commented May 19, 2016

This is a fantastic idea that I'm sure many of us have thought about.

I'd like to see the framework CLI tools be a package/module that would easily allow the definition of new commands by modules.

What you've got so far in your module looks like excellent ground work!

@tractorcow
Copy link
Contributor

I would be in favour of a native non-composer task runner built on Symfony. I've used it for cow and I find it an excellent framework.

You would need to find a solution to abstracting running of tasks via the web, however. Perhaps the symfony console component could be one interface, where the web is a separate one.

@sminnee
Copy link
Member

sminnee commented Jun 29, 2017

The beta1 windows is closing. However if we retained the existing sake command for posterity and introduced this as a new tool, this can be done as a minor change.

@sminnee
Copy link
Member

sminnee commented Nov 15, 2018

Also, it's quite likely that not all actions we might want to bolt into the CLI tool could be wrapped into this abstracted build tasks, but that most would be, and that where it's possible it has the benefit of running them in different context such as a job-queue, which is especially helpful for production environments and cronjobs.

@tractorcow
Copy link
Contributor

We already integrate with a lot more symfony (whereas we used to do so with Zend). I wouldn't mind if we just used their IO abstraction directly for tasks. I have looked at their interfaces before, and I believe that they would well suit our use cases (e.g. cronjobs, manual tasks with CLI, other interactive / non-interactive environments).

I would look at something similar to queuedjobs interface, but with more care given to how tasks are persisted. :) E.g. use core __wake / __sleep methods instead of custom jobData(), implement serializable for storage, and a main entry point public function run(InputInterface, OutputInterface) to replace the old public function run($request)

We would also separate the concerns of the nature of the tasks themselves, versus how the tasks are created, invoked, or stored. That would be left up to third party modules, and we would simply provide a basic runner replacing sake.

@robbieaverill
Copy link
Contributor

But the core issue here is that tasks aren't necessarily HTTP, and CLI definitely isn't. How is moving from 1 HTTP interface to another HTTP interface helping anything?

Yeah true, haha

+1 from me for using the Symfony IO interfaces.

I do feel like there's a place for a logger in tasks, but I'm not sure where I sit in its placement as the primary output interface

@ScopeyNZ
Copy link
Contributor

I don't think we should use a log interface as output. Output should be a fair amount more dynamic than logging.

My vote is to use Symfony interfaces. Don't use any HTTP interface - we're making CLI tasks.

The only challenge is running tasks over HTTP. We can probably write something that takes __GET parameters and runs the command with --no-interaction and just forwards the output back into a streamed response?

Eg:
dev/build?flush -> sake dev:build --flush
dev/tasks/my/task?param=1&flag -> sake tasks:my:task --param="1" --flag

We could also redesign the output of these tasks when formatted as HTTP - I'm thinking some basic layout with a CLI-like themed <pre> where the content is streamed - similar to what we see on Travis builds.

@robbieaverill
Copy link
Contributor

Further idea (from #7992): have a default output showing task completed with the execution time. Note that this may come by default with symfony/console with the verbose flag, otherwise we could make it something that only shows with verbosity enabled.

@sminnee
Copy link
Member

sminnee commented Nov 27, 2018

I don't think we should use a log interface as output. Output should be a fair amount more dynamic than logging.

I think that we're conflating two things here:

  • Build tasks
  • Sake plugins

For pieces of code that are intended to be used in the CLI tool (whether it's called sake or something else), I agree.

However, build tasks are typically run in a variety of contexts:

  • CLI execution
  • Web-based execution
  • Cron-jobs
  • Added to the QueuedJobs queue and run in the background with their output fetched back into the QJ admin

Given this breadth of execution it's going to be important to clearly state what the limits of input and output are. My view is that PSR3 describes the intent of a build task's output very well: to provide a log of what happened and why.

If anyone disagrees with this, I would ask "what form of out output is inappropriately fed through a log that is also useable in the 4 different use-cases I've listed above?"

Output should be a fair amount more dynamic than logging.

I could see a use case for interactive / animated CLI tools with lots of colour (beyond the simple colour coding of error-levels with PSR3 would allow for). However, such code is unlikely to be very useful outside of CLI execution.

As such I think we should not use BuildTask or its replacement as the placeholder of such code, and think of that as something more like a ConsolePlugin.

@robbieaverill
Copy link
Contributor

Ok fair enough! I think we need to review #8044 as well, which would allow a task with a logger to use all of the PSR-3 levels without having them being chewed up in the SilverStripe core log handling

@chillu
Copy link
Member

chillu commented Dec 7, 2018

I'm in favour of this as well, but coming from another angle: There's no CSRF protection around web-based administrative commands. This can lead to DoS targeted at a single CMS author or admin user (not really DDoS). Depending on the dev tasks installed, it could also be destructive. Rather than adding CSRF protection, I'd propose that we focus our energies on making CLI usage great, deprecate calling dev/* and via web, and disable it by default in production mode (allowing devs to opt in for covering their edge cases).

Laravel allows you to define routes to call CLI commands through the web, but you have to opt-into that - which is a good balance I think.

I've collected related tickets:

@sminnee
Copy link
Member

sminnee commented Dec 7, 2018

I'd propose that we focus our energies on making CLI usage great, deprecate calling dev/* and via web, and disable it by default in production mode (allowing devs to opt in for covering their edge cases).

Just as long as build tasks can still be enqueued as is currently the case if you install the queuedjobs module. This approach is an important use-case and should be our first recommendation for production servers where is SSH access is either unavailable or discouraged.

@sminnee
Copy link
Member

sminnee commented Dec 7, 2018

To be clear: since queuedjobs 3.1, if you install queuedjobs, the behaviour of the dev/tasks web UI is replaced with one that provides links to enqueue jobs instead of running them directly.

@sminnee
Copy link
Member

sminnee commented Dec 7, 2018

...this probably means that we should add CSRF protection.

@chillu
Copy link
Member

chillu commented Dec 7, 2018

...this probably means that we should add CSRF protection.

Discussed as SS-2018-022

@robbieaverill
Copy link
Contributor

I'd love to see the dev/* routes gone from the browser by default at least. I understand the need for some support since not everyone has CLI access, so queued jobs seem like a good alternative to me. In a perfect world I think I'd rather see the management of that happening in the admin interface rather than in the dev/* space though

@lerni
Copy link
Contributor

lerni commented Dec 7, 2018

I like the separation of concerns of developer & content-author. Moving dev-tasks to the admin-interface goes against that and requires the admin-interface to be functional to run dev-tasks per HTTP. CLI for sure is the way to go but if it has to runs per HTTP, it has to be as simple as possible, which the admin interface isn't.

@ScopeyNZ
Copy link
Contributor

I'd love to see the dev/* routes gone from the browser by default at least. I understand the need for some support since not everyone has CLI access, so queued jobs seem like a good alternative to me. In a perfect world I think I'd rather see the management of that happening in the admin interface rather than in the dev/* space though

If we're going down this path I think it makes a lot of sense to get the conversation started sooner rather than later. Some RFC to remove the HTTP interfaces for dev/build/ and dev/tasks (what about ?flush)? I don't really think I'm for this yet btw, I'd need some convincing.

@dnsl48
Copy link
Contributor

dnsl48 commented Feb 4, 2019

Symfony Console component becomes a Framework requirement

I don't see why it cannot live as a decoupled module. The only requirement would be for the framework to provide necessary APIs.

@chillu
Copy link
Member

chillu commented Apr 15, 2019

There's a simple "ansi to html" wrapper for showing coloured Symfony console output via a web browser: https://symfony.com/doc/current/console/command_in_controller.html

@GuySartorelli
Copy link
Member

GuySartorelli commented Sep 3, 2024

Closing. The work is being done in #11341.

Much of the discussion here has directly influenced the implementation.
We're not using a PSR3 logger for BuildTask output - but care has been taken to ensure output can be made suitable for all of the scenarios outlined in #5542 (comment) with the exception that output from CRON will be ignored unless errors occur or the cronjob somehow captures the output.

The output class we're using is injectable, so developers are free to add their own PSR3 logging if they want to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests