NEW Refactor CLI interaction with Silverstripe app #11353
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3 tasks
GuySartorelli
force-pushed
the
pulls/6/symfony-sake
branch
3 times, most recently
from
db57250
to
942a93f
Compare
This was referenced Aug 28, 2024
Merged
Merged
GuySartorelli
commented
Aug 28, 2024
GuySartorelli
force-pushed
the
pulls/6/symfony-sake
branch
from
942a93f
to
dd5e39b
Compare
This was referenced Aug 28, 2024
GuySartorelli
force-pushed
the
pulls/6/symfony-sake
branch
from
dd5e39b
to
dff8ca3
Compare
GuySartorelli
force-pushed
the
pulls/6/symfony-sake
branch
15 times, most recently
from
225a8f9
to
d3cb726
Compare
GuySartorelli
force-pushed
the
pulls/6/symfony-sake
branch
3 times, most recently
from
d56edb4
to
9a08b2e
Compare
emteknetnz
requested changes
Sep 12, 2024
GuySartorelli
force-pushed
the
pulls/6/symfony-sake
branch
2 times, most recently
from
78b06c0
to
c95d6dc
Compare
GuySartorelli
commented
Sep 17, 2024
GuySartorelli
force-pushed
the
pulls/6/symfony-sake
branch
7 times, most recently
from
b4c359e
to
7cb613d
Compare
emteknetnz
requested changes
Sep 19, 2024
Comment on lines
+62
to
+69
| // These lines commented out from the parent class implementation. | ||
| // We don't want this opinionated default colouring - it doesn't appear in the ANSI format so it doesn't belong in the output. | ||
| // if ($this->inlineStyles) { | ||
| // $html = sprintf('<span style="background-color: %s; color: %s">%s</span>', $this->inlineColors['black'], $this->inlineColors['white'], $html); | ||
| // } else { | ||
| // $html = sprintf('<span class="ansi_color_bg_black ansi_color_fg_white">%s</span>', $html); | ||
| // } | ||
| // We do need an opening and closing span though, or the HTML markup is broken |
There was a problem hiding this comment.
Makes sense to keep in I think to understand why this exists
emteknetnz
requested changes
Sep 23, 2024
GuySartorelli
force-pushed
the
pulls/6/symfony-sake
branch
6 times, most recently
from
4b1e49d
to
88a1f03
Compare
emteknetnz
requested changes
Sep 25, 2024
src/Cli/LegacyParamArgvInput.php
Outdated
|
|
||
| public function __construct(?array $argv = null, ?InputDefinition $definition = null) | ||
| { | ||
| Deprecation::withNoReplacement( |
There was a problem hiding this comment.
Should rebase it in then so we don't have things break later
src/Cli/LegacyParamArgvInput.php
Outdated
| $tokens = ArrayLib::insertBefore($tokens, $convertedFlags, '--', true, true); | ||
| } | ||
| if ($hadLegacyParams) { | ||
| // We only want the warning once regardless of how many params there are. |
There was a problem hiding this comment.
It's an E_USER_DEPRECATED? Which is neither. Since we say 'notice' everywhere we should just refer to it as a notice rather than a warning.
- Turn sake into a symfony/console app - Avoid using HTTPRequest for CLI interaction - Implement abstract hybrid execution path
GuySartorelli
force-pushed
the
pulls/6/symfony-sake
branch
from
88a1f03
to
bb27aec
Compare
emteknetnz
approved these changes
Sep 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The idea is that you can define any combination of these in a module or project:
dev/*route) e.g./dev/graphql/idewhich simply doesn't work in a CLI contextDirector::is_cli()/dev/*especiallyBuildTasksubclasses will use thisNew sake uses
tasks:mytaskformat in CLI (this is standard for symfony console apps and means we can take advantage of grouping functionality), but the olddev/tasks/mytasksyntax will still work for at least the lifetime of CMS 6 so people don't have to immediately rewrite all their scripts. Thedev/aliases are deprecated to be removed in the future.Flags can also still be passed as arguments (e.g.
--flushcan still be passed asflush=1) but this is deprecated.The HTTP
/dev/*routes (e.g./dev/build) will still work exactly the same way they used to, from a user perspective. The only difference is where the code lives, how it's registered, and how theDevelopmentAdminclass calls it - and the fact that additional help info including available parameters are shown in the list views.I'm pretty sure this is a low-traffic area in terms of customisation but the upgrade path is very straight forward in any case.
Existing
BuildTaskimplementations whichechoout their output directly should still work effectively as-is (i.e. just update the method and property signatures but don't update the method body) if people don't want to update to use the newPolyOutput.The main thing that will change is that you won't have an
HTTPRequestanymore - instead get vars and CLI flags will be given via symfony'sInputInterface. Should be a very straight forward upgrade.I've implemented a
navigatecommand (e.g.sake navigate about-us) in case people have weird edge cases where they do explicitly want to use the old HTTPRequest-over-CLI style. But its use will be discouraged in favour of implementing aPolyCommand.Issue