Refactor sake around Symfony console

[emteknetnz]

sake is basic though functional way to run cli commands

It's very non-standard, and it's likely that we could make the interface significantly better by changing it to use symfony/console instead. There may also be some downstream benefits of making it easier to project to extend/customise if they have a need to

Related

• RFC: Use Symfony Console Component for Tasks and better cli support #5542
• dev/tasks on command line are very hard to read #8998
• RFC: Avoid HTTPApplication for CLI #7509

Acceptance criteria

• First determine if this is something that's viable without breaking much in terms of backward compatibility i.e. user should still call sake, though possibly some of the commands are different, though preferably not
• Target CMS 6
• Replace as much of sake with symfony/console as possible

CMS 5 PRs

• Various deprecations and a few features #11365
• API Deprecate API that will be removed silverstripe-graphql#603
• API Deprecate API that will be removed silverstripe-ldap#91
• API Deprecate API that will be removed silverstripe-subsites#599
• API Deprecate API that will be removed silverstripe-externallinks#147
• API Deprecate API that will be removed silverstripe-realme#162
• API Deprecate API that will be removed silverstripe-staticpublishqueue#203
• API Deprecate API that will be removed silverstripe-contentreview#257
• API Deprecate API that will be removed silverstripe-crontask#94
• API Deprecate API that will be removed silverstripe-graphql-devtools#60
• API Deprecate API that will be removed tractorcow-farm/silverstripe-fluent#887
• API Deprecate API that will be removed bringyourownideas/silverstripe-maintenance#240
• API Deprecate API that will be removed symbiote/silverstripe-queuedjobs#447
• DOCS Document deprecated API and a few new features developer-docs#580
• Merge branch '7' into 8 tractorcow-farm/silverstripe-fluent#890

CMS 6 PRs

kitchen sink run
CI failures are pre-existing

• NEW Refactor CLI interaction with Silverstripe app #11353
• API Update API to reflect changes to CLI interaction silverstripe-testsession#93
• API Update API to reflect changes to CLI interaction silverstripe-graphql#600
• API Update API to reflect changes to CLI interaction silverstripe-session-manager#213
• API Update API to reflect changes to CLI interaction silverstripe-frameworktest#203
• API Update API to reflect changes to CLI interaction silverstripe-assets#635
• API Update API to reflect changes to CLI interaction silverstripe-ldap#89
• API Update API to reflect changes to CLI interaction silverstripe-linkfield#329
• API Update API to reflect changes to CLI interaction silverstripe-subsites#597
• API Update API to reflect changes to CLI interaction silverstripe-externallinks#145
• API Update API to reflect changes to CLI interaction silverstripe-realme#160
• API Update API to reflect changes to CLI interaction silverstripe-staticpublishqueue#201
• API Update API to reflect changes to CLI interaction silverstripe-userforms#1325
• API Update API to reflect changes to CLI interaction silverstripe-contentreview#255
• API Update API to reflect changes to CLI interaction silverstripe-sharedraftcontent#259
• API Update API to reflect changes to CLI interaction silverstripe-graphql-devtools#58
• API Update API to reflect changes to CLI interaction silverstripe-elemental#1252
• API Update API to reflect changes in CLI interaction silverstripe-crontask#93
• API Update API to reflect changes to CLI interaction symbiote/silverstripe-advancedworkflow#552
• API Update API to reflect changes to CLI interaction symbiote/silverstripe-queuedjobs#445
• API Update API to reflect changes to CLI interaction tractorcow-farm/silverstripe-fluent#884
• API Update API to reflect changes to CLI interaction bringyourownideas/silverstripe-maintenance#239
• API Update API to reflect changes to CLI interaction bringyourownideas/silverstripe-composer-update-checker#97
• ENH Update config to reflect changes in CLI interaction silverstripe-environmentcheck#112
• ENH Use standardised BackURL instead of non-standard returnURL silverstripe-cms#2999
• DOC Update docs to reflect new sake commands silverstripe-tagfield#307
• DOC Update docs to reflect changes in CLI interaction silverstripe-behat-extension#282
• DOC Update docs to reflect changes in CLI interaction silverstripe-taxonomy#123
• DOC Update docs to reflect changes in CLI interaction silverstripe-registry#107
• DOC Update docs to reflect changes in CLI interaction silverstripe-auditor#92
• DOC Update docs to reflect changes in CLI interaction silverstripe-siteconfig#176
• DOC Update docs to reflect changes in CLI interaction silverstripe-spamprotection#127
• DOC Update docs to reflect changes in CLI interaction silverstripe-hybridsessions#113
• DEP Update dep for graphql-devtools recipe-kitchen-sink#72
• DOC Document changes to CLI interaction developer-docs#571

[GuySartorelli]

I've gotten pretty far with this for everything included in installer. I'm nearly at the point where I can clean up and start writing tests (and then move onto the rest of kitchen sink).

One thing I'd like some opinions on is how the dev/ commands should be arranged.

Right now the PRs have these just swapping dev/ to dev:, i.e. you can run the commands like so:

HTTP route	CLI command	CLI alias (for backwards compat)
dev/tasks/my-task	dev:tasks:my-task	dev/tasks/my-task
dev/config	dev:config	dev/config
dev/build	dev:build	dev/build

This however keeps all of the commands clumped together in an unhelpful way, and means you have to type dev: before every command, which is completely unnecessary in the CLI. The only reason they had that historically is because sake was just a CLI wrapper for the HTTP controllers, so the routes had to be identical. Now they don't.

I would like to change the commands to be more meaningful in CLI, while keeping their HTTP routes as they currently are. That would look more like this:

HTTP route	CLI command	CLI alias (for backwards compat)
dev/tasks/my-task	tasks:my-task	dev/tasks/my-task
dev/config	config:dump	dev/config
dev/build	db:build	dev/build

This gives us a more sensible grouping for the CLI commands, allows for new commands to be more easily added to those groupings in a meaningful way, reduces unnecessary key-strokes, and I think just looks way tidier.

(I'm planning on hiding the tasks from this initial list (at least if there are more than some configurable number of them) and only showing them when you run sake tasks (or sake dev:tasks if this proposal isn't accepted) so don't get too caught up on how those are filling the output)

@silverstripe/core-team Does anyone have any thoughts on this?

[wilr]

@GuySartorelli 100% agree. Coming from a laravel view that makes a lot more sense than prefixing everything dev. 👍

[emteknetnz]

Looks good, happy to get rid of the dev prefix

[kinglozzer]

+1 for getting rid of the dev prefix

[michalkleiner]

All good to drop the dev: prefix.

[GuySartorelli]

Running sake tasks lists a bunch of tasks in a mixture of casing, e.g. tasks:WorkflowReminderTask and tasks:content-review-emails. We should only have a single style, which to me seems like it could be the WorkflowReminderTask style as that matches the class name which people are familiar with

I think we should aim for lowercase task names for newly named tasks, because that's easier to type and is more consistent with basically everything else you'll ever do in CLI (at least for *nix).
Note that these names are also used for the URL segment, where lowercase with hyphens is also more standard than using capital letters.

I would prefer not to rename existing tasks that already have short names so that we can preserve backwards compatibility for them (I gave an explicit name to some tasks that were falling back to FQCN because the FQCN is really long and looks awful in the new sake tasks output).

Only exceptions would be for the two tasks in fluent which already has $segments in CMS 5 which didn't match the ClassName, they should say as is

Based on this comment it seems you agree that we should avoid renaming the tasks which already have short names defined. So this is only about tasks we're giving a command name where the legacy segment wasn't already defined

[GuySartorelli]

sake dev/tasks and dev/graphql and simply sake dev don't work like they used to, though dev/build and dev/config do work which is inconsistent

They can't work like they used to, because sake doesn't use the routed controllers to get or list info anymore. Lists of commands are handled by symfony/console internals.

sake tasks is a slightly special case because I've implemented logic to hide tasks from the main list if you have too many, so I made an explicit command for listing them - but that still just fires off the equivalent of sake list tasks. It doesn't use the old logic to list the tasks, nor should it.

Given the intention behind adding backwards compatibility is to allow scripts to continue working, I don't see any value in adding backwards compatibility for the commands that only listed what commands are available, given that doing so would be adding tech debt (immediately deprecated code).

[GuySartorelli]

There's some text which doesn't make sense in a CLI context

I'll resolve the one you've listed, but the way you said that implies there are more cases? Please point them all out, since you saw them and I didn't it would be hard for me to fix them without them being pointed out.

[GuySartorelli]

I'm also finding the new permissions_for_browser_execution private static confusing for a few reasons:

• It (probably correctly) uses an any check vs the LeftAndMain subclasses required_permission_codes's all

It's essentially doing the job of the now-deprecated init_permissions configuration property on the old controllers, e.g

silverstripe-framework/src/Dev/DevConfigController.php

Lines 40 to 44 in 6287b6e

	private static $init_permissions = [
	'ADMIN',
	'ALL_DEV_ADMIN',
	'CAN_DEV_CONFIG',
	];

• In 02_Hybrid_commands.php (rename to 02_Poly_commands.php) it says "An array of permissions a user must have to run the command in an HTTP request" which implies it's an 'all', however the fact that ADMIN is in all of these arrays implies they're all ADMIN only, which isn't the case.

Please point out things like this in the relevant PR - it's a lot easier for me to keep track of changes I need to make to a specific PR if the comment is in that PR.
I'll try to update the wording in a way that doesn't imply "all".

• Also the anyone_with_dev_admin_permissions and anyone_with_task_permissions keys really confused me, I thought they were actually some sort of new functionality for a while until I realised they're just labels to try and be helpful. They're actually unhelpful so the keys should just be be removed

Those permissions already existed. I'm keeping the same permissions in order to preserve backwards compatibility.
Although if I've misunderstood what you're meaning it may pay to comment in context of the PR so I can see what you're talking about.

[emteknetnz]

Running sake tasks lists a bunch of tasks in a mixture of casing ... Note that these names are also used for the URL segment

Yeah I'll assume that most most tasks are being run via the browser rather than CLI, simply because it's generally easier to login as an admin rather than SSH on to a production server, so the lower-case-with-dashes which $segment currently uses is probably the better standard.

There's some text which doesn't make sense in a CLI context ... Please point them all out

I had look through all the tasks, I could find any others like this one I already pointed out

sake dev/tasks and dev/graphql ... They can't work like they used to

All good

[GuySartorelli]

All PRs merged. CI will be cleaned up in silverstripe/.github#313

[madmatt]

Thank you to everyone who has worked on this, very glad to see this happening <3