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

Refactor sake around Symfony console #11341

Open
3 tasks done
emteknetnz opened this issue Aug 18, 2024 · 8 comments
Open
3 tasks done

Refactor sake around Symfony console #11341

emteknetnz opened this issue Aug 18, 2024 · 8 comments

Comments

@emteknetnz
Copy link
Member

emteknetnz commented Aug 18, 2024

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

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

CMS 6 PRs

kitchen sink run

@GuySartorelli
Copy link
Member

GuySartorelli commented Aug 31, 2024

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

Example of the existing command setup

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

Example of the proposed command setup

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
Copy link
Member

wilr commented Sep 1, 2024

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

@emteknetnz
Copy link
Member Author

Looks good, happy to get rid of the dev prefix

@kinglozzer
Copy link
Member

+1 for getting rid of the dev prefix

@michalkleiner
Copy link
Contributor

All good to drop the dev: prefix.

@emteknetnz
Copy link
Member Author

@GuySartorelli Will need to rebase the PRs that have phpunit/phpunit in require rather than require-dev. behat-extension is one of them, there may be others.

I'm unable to install of the PR's at the same time locally due to conflicts between phpunit 11 which was just merged and phpunit 9

@GuySartorelli
Copy link
Member

GuySartorelli commented Sep 18, 2024

Yup there's rebasing to be done, just needed that CMS 5 pr merged first 👍

@emteknetnz
Copy link
Member Author

@GuySartorelli A few things I've noticed from doing some quick testing locally, which I'll not here rather than on individual PRs where they'll need to be fixed:

  • 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. The command names and docs will need to be updated.
  • 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
  • There's some text which doesn't make sense in a CLI context, e.g. sake tasks:delete-queued-jobs will prompt Really delete 3 jobs? Please add ?confirm=1 to the URL to confirm. - should be prompting use to add --confirm when in CLI instead

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

6 participants