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

DOC Document changes to CLI interaction #571

Merged
merged 1 commit into from
Sep 26, 2024

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli marked this pull request as draft August 28, 2024 05:20
@GuySartorelli GuySartorelli force-pushed the pulls/6/symfony-sake branch 4 times, most recently from 4a1da0d to 20ebfd6 Compare September 4, 2024 08:34
@GuySartorelli GuySartorelli force-pushed the pulls/6/symfony-sake branch 2 times, most recently from af215b9 to 70249e8 Compare September 16, 2024 22:40
@GuySartorelli GuySartorelli marked this pull request as ready for review September 17, 2024 02:28
---
SilverStripe\Control\Director:
rules:
my-command: 'App\Cli\Command\MyPolyCommand'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this now necessary? Or does protected static string $commandName provide a segment for dev/tasks the way that private static $segment used to?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an optional way that custom poly commands can be routed for access via an HTTP request.
This is not necessary for BuildTask or DevCommand.

Comment on lines 257 to 262
You would now access the `/dev/my-action` action via an HTTP request only. The `/dev/my-other-action` action can be access via an HTTP request, or by using `sake app:my-other-action` on the command line.

The `some-var` option can be used in a query string when running the action via an HTTP request, or as a flag (e.g. `sake app:my-other-action --some-var`) in CLI.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
You would now access the `/dev/my-action` action via an HTTP request only. The `/dev/my-other-action` action can be access via an HTTP request, or by using `sake app:my-other-action` on the command line.
The `some-var` option can be used in a query string when running the action via an HTTP request, or as a flag (e.g. `sake app:my-other-action --some-var`) in CLI.
You would now access the `/dev/my-http-only-action` action via an HTTP request only. The `/dev/my-http-and-cli-action` action can be access via an HTTP request, or by using `sake app:my-http-and-cli-action` on the command line.
The `some-var` option can be used in a query string when running the action via an HTTP request, or as a flag (e.g. `sake app:my-http-and-cli-action --some-var`) in CLI.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 200 to 205
+ protected static string $commandName = 'app:my-other-action';
+
+ protected static string $description = 'Perform my custom action in dev/my-other-action or via sake app:my-other-action';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
+ protected static string $commandName = 'app:my-other-action';
+
+ protected static string $description = 'Perform my custom action in dev/my-other-action or via sake app:my-other-action';
+ protected static string $commandName = 'app:my-http-and-cli-action';
+
+ protected static string $description = 'Perform my custom action in dev/my-http-and-cli-action or via sake app:my-http-and-cli-action';

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

+use Symfony\Component\Console\Input\InputOption;

-class MyOtherActionController extends Controller implements PermissionProvider
+class MyOtherActionController extends DevCommand implements PermissionProvider
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
+class MyOtherActionController extends DevCommand implements PermissionProvider
+class MyHttpAndCliActionController extends DevCommand implements PermissionProvider

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done - also updated MyActionController to MyHttpOnlyActionController

> [!CAUTION]
> You should run `sake` with the same system user that runs your web server. Otherwise you will have a separate filesystem cache for CLI and you won't be able to flush or warm your webserver cache using Sake.

Sake doesn't use your project's routing and controllers for normal execution, but if you do specifically need to access an HTTP route in your application from the CLI, you can use the `sake navigate` command.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Sake doesn't use your project's routing and controllers for normal execution, but if you do specifically need to access an HTTP route in your application from the CLI, you can use the `sake navigate` command.
Sake doesn't use your project's routing and controllers for normal execution. However if you do specifically need to access an HTTP route in your application from the CLI, you can use the `sake navigate` command.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very subjective and you've not given any reasoning for it? I'll make the change to avoid unnecessary ping pong but might be good to say why you want subjective changes like this in future.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -0,0 +1,172 @@
---
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename file to 02_Poly_Commands.md

Copy link
Member Author

@GuySartorelli GuySartorelli Sep 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to 02_PolyCommand.md which is what all the links to this page assume the name is.

@@ -130,5 +130,5 @@ Silverstripe core environment variables are listed here, though you're free to d
| `SS_MANIFESTCACHE` | The manifest cache to use (defaults to file based caching). Must be a `Psr\Cache\CacheItemPoolInterface`, `Psr\SimpleCache\CacheInterface`, or `SilverStripe\Core\Cache\CacheFactory` class implementation. |
| `SS_IGNORE_DOT_ENV` | If set, the `.env` file will be ignored. This is good for live to mitigate any performance implications of loading the `.env` file. |
| `SS_BASE_URL` | The URL to use when it isn't determinable by other means (for example for CLI commands). Should either start with a protocol (e.g. `https://www.example.com`) or with a double forward slash (e.g. `//www.example.com`). |
| `SS_FLUSH_ON_DEPLOY` | Try to detect deployments through file system modifications and flush on the first request after every deploy. Does not run "dev/build" - only "flush". Possible values are `true` (check for a framework PHP file modification time), `false` (no checks, skip deploy detection) or a path to a specific file or folder to be checked. See [DeployFlushDiscoverer](api:SilverStripe\Core\Startup\DeployFlushDiscoverer) for more details.<br /><br />False by default. |
| `SS_FLUSH_ON_DEPLOY` | Try to detect deployments through file system modifications and flushes the cache on the first request after every deploy. Note this does not trigger buildin the database. Possible values are `true` (check for a framework PHP file modification time), `false` (no checks, skip deploy detection) or a path to a specific file or folder to be checked. See [DeployFlushDiscoverer](api:SilverStripe\Core\Startup\DeployFlushDiscoverer) for more details.<br /><br />False by default. |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| `SS_FLUSH_ON_DEPLOY` | Try to detect deployments through file system modifications and flushes the cache on the first request after every deploy. Note this does not trigger buildin the database. Possible values are `true` (check for a framework PHP file modification time), `false` (no checks, skip deploy detection) or a path to a specific file or folder to be checked. See [DeployFlushDiscoverer](api:SilverStripe\Core\Startup\DeployFlushDiscoverer) for more details.<br /><br />False by default. |
| `SS_FLUSH_ON_DEPLOY` | Try to detect deployments through file system modifications and flushes the cache on the first request after every deploy. Note this does not trigger building the database. Possible values are `true` (check for a framework PHP file modification time), `false` (no checks, skip deploy detection) or a path to a specific file or folder to be checked. See [DeployFlushDiscoverer](api:SilverStripe\Core\Startup\DeployFlushDiscoverer) for more details.<br /><br />False by default. |

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 174 to 181
+ my-action:
+ class: 'App\Dev\MyActionController'
+ description: 'Perform my custom action in dev/my-action'
+ commands:
+ my-other-action: 'App\Dev\MyOtherActionCommand'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
+ my-action:
+ class: 'App\Dev\MyActionController'
+ description: 'Perform my custom action in dev/my-action'
+ commands:
+ my-other-action: 'App\Dev\MyOtherActionCommand'
+ my-http-only-action:
+ class: 'App\Dev\MyHttpOnlyActionController'
+ description: 'Perform my custom action in dev/my-http-only-action'
+ commands:
+ my-http-and-cli-action: 'App\Dev\MyHttpAndCliActionCommand'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 76 to 78
- [`can_run_in_cli`](api:SilverStripe\PolyExecution\PolyCommand->can_run_in_cli): Whether the command can be run in CLI
- [`can_run_in_browser`](api:SilverStripe\PolyExecution\PolyCommand->can_run_in_browser): Whether the command can be run in HTTP requests
- [`permissions_for_browser_execution`](api:SilverStripe\PolyExecution\PolyCommand->permissions_for_browser_execution): An array of permissions a user must have to run the command in an HTTP request
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- [`can_run_in_cli`](api:SilverStripe\PolyExecution\PolyCommand->can_run_in_cli): Whether the command can be run in CLI
- [`can_run_in_browser`](api:SilverStripe\PolyExecution\PolyCommand->can_run_in_browser): Whether the command can be run in HTTP requests
- [`permissions_for_browser_execution`](api:SilverStripe\PolyExecution\PolyCommand->permissions_for_browser_execution): An array of permissions a user must have to run the command in an HTTP request
- [`can_run_in_cli`](api:SilverStripe\PolyExecution\PolyCommand->can_run_in_cli): Whether the command can be run in CLI
- [`can_run_in_browser`](api:SilverStripe\PolyExecution\PolyCommand->can_run_in_browser): Whether the command can be run in HTTP requests
- [`permissions_for_browser_execution`](api:SilverStripe\PolyExecution\PolyCommand->permissions_for_browser_execution): An array of permissions of which the user must have at least one have to run the command in an HTTP request

By default Permission::check() will use any not all

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarified in a different way (based on the comment in the main issue I assumed there would be no suggestion here so I made the change before I saw this)

en/08_Changelogs/6.0.0.md Show resolved Hide resolved

class MyCustomTask extends BuildTask
{
protected static string $commandName = 'my-custom-task';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
protected static string $commandName = 'my-custom-task';
protected static string $commandName = 'MyCustomTask';

I've recommended we use a $commandName that matches the short classname for all BuildTasks, so we should probably recommend the same convention in our docs

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussion about this is ongoing in the issue.

Comment on lines 144 to 147
For example, the following will run `MyTask` every minute.

```bash
* * * * * /your/site/folder/vendor/bin/sake tasks:my-task
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
For example, the following will run `MyTask` every minute.
```bash
* * * * * /your/site/folder/vendor/bin/sake tasks:my-task
For example, the following will run `MyCustomTask` every minute.
```bash
* * * * * /your/site/folder/vendor/bin/sake tasks:MyCustomTask

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussion about this is ongoing in the issue.

en/08_Changelogs/6.0.0.md Show resolved Hide resolved

For actions that should only be accessible in the browser, you only need to change how these are registered. Move them from `DevelopmentAdmin.registered_controllers` to the new [`DevelopmentAdmin.controllers`](api:SilverStripe\Dev\DevelopmentAdmin->controllers) configuration property.

Controllers added to `DevelopmentAdmin.registered_controllers` can only be accessed via HTTP requests, so you can remove any logic around CLI usage.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Controllers added to `DevelopmentAdmin.registered_controllers` can only be accessed via HTTP requests, so you can remove any logic around CLI usage.
Controllers added to `DevelopmentAdmin.controllers` can only be accessed via HTTP requests, so you can remove any logic around CLI usage.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 153 to 157
- `/dev/my-action`: intended for use in the browser only, but you'd have to add custom logic in `init()` to disallow its use in CLI until now
- `/dev/my-other-action`: intended for use both in CLI and in the browser.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- `/dev/my-action`: intended for use in the browser only, but you'd have to add custom logic in `init()` to disallow its use in CLI until now
- `/dev/my-other-action`: intended for use both in CLI and in the browser.
- `/dev/my-http-only-action`: intended for use in the browser only, but you'd have to add custom logic in `init()` to disallow its use in CLI until now
- `/dev/my-http-and-cli-action`: intended for use both in CLI and in the browser.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


For actions that should only be accessible in the browser, you only need to change how these are registered. Move them from `DevelopmentAdmin.registered_controllers` to the new [`DevelopmentAdmin.controllers`](api:SilverStripe\Dev\DevelopmentAdmin->controllers) configuration property.

Controllers added to `DevelopmentAdmin.registered_controllers` can only be accessed via HTTP requests, so you can remove any logic around CLI usage.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Controllers added to `DevelopmentAdmin.registered_controllers` can only be accessed via HTTP requests, so you can remove any logic around CLI usage.
Controllers added to `DevelopmentAdmin.controllers` can only be accessed via HTTP requests, so you can remove any logic around CLI usage.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate of #571 (comment)?

}
```

You would now access the `/dev/my-action` action via an HTTP request only. The `/dev/my-other-action` action can be access via an HTTP request, or by using `sake app:my-other-action` on the command line.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
You would now access the `/dev/my-action` action via an HTTP request only. The `/dev/my-other-action` action can be access via an HTTP request, or by using `sake app:my-other-action` on the command line.
You would now access the `/dev/my-http-only-action` action via an HTTP request only. The `/dev/my-http-and-cli-action` action can be access via an HTTP request, or by using `sake app:my-http-and-cli-action` on the command line.

Copy link
Member Author

@GuySartorelli GuySartorelli Sep 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.... I think this is a duplicate suggestion? Or else you suggested other related changes way earlier and now it's hard to tell if this is a duplicate or just related.
Done in any case.

Sake used to have functionality to make daemon processes for your application. This functionality was managed with `sake -start my-process` and `sake -stop my-process`.

We've removed this functionality. Please use an appropriate daemon tool such as `systemctl` to manage these instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to mention dont_populate being renamed to no-populate

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not related to sake -start and sake -stop, but I've added a note about it to the relevant place.

@emteknetnz emteknetnz merged commit 2cc7091 into silverstripe:6 Sep 26, 2024
3 checks passed
@emteknetnz emteknetnz deleted the pulls/6/symfony-sake branch September 26, 2024 05:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants