-
-
Notifications
You must be signed in to change notification settings - Fork 416
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
[make:*] improve output messages for Symfony CLI users #1238
Merged
Merged
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
72f0ff0
[make:*] improve output messages for Symfony CLI users
jrushlow 2aa5179
use Symfony CLI vars to determine correct command prefix
jrushlow d1c39d2
testcases for cli tools
jrushlow 0a9aee3
no trailing space
jrushlow 2552c06
guess prompt in existing makers
jrushlow 589e8a0
ensure correct command shown in cmd output
jrushlow 70fac1a
cleanup
jrushlow 37ebedd
add testcases for make migration
jrushlow db8bbd8
use name value for prompt
jrushlow 9e98c25
simplify tests
jrushlow 1349524
better method name
jrushlow a8ec5d7
why didnt i think of this
jrushlow File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
<?php | ||
|
||
/* | ||
* This file is part of the Symfony MakerBundle package. | ||
* | ||
* (c) Fabien Potencier <[email protected]> | ||
* | ||
* For the full copyright and license information, please view the LICENSE | ||
* file that was distributed with this source code. | ||
*/ | ||
|
||
namespace Symfony\Bundle\MakerBundle\Util; | ||
|
||
/** | ||
* Tools used to enhance maker command output. | ||
* | ||
* For additional context with Symfony CLI EnvVars, see | ||
* https://github.com/symfony-cli/symfony-cli/pull/231 | ||
* | ||
* @author Jesse Rushlow <[email protected]> | ||
* | ||
* @internal | ||
*/ | ||
final class CliOutputHelper | ||
{ | ||
/** | ||
* EnvVars exposed by Symfony's CLI. | ||
*/ | ||
public const ENV_VERSION = 'SYMFONY_CLI_VERSION'; // Current CLI Version | ||
public const ENV_BIN_NAME = 'SYMFONY_CLI_BINARY_NAME'; // Name of the binary e.g. "symfony" | ||
|
||
/** | ||
* Get the correct command prefix based on Symfony CLI usage. | ||
*/ | ||
public static function getCommandPrefix(): string | ||
{ | ||
$prompt = 'php bin/console'; | ||
|
||
$binaryNameEnvVar = getenv(self::ENV_BIN_NAME); | ||
|
||
if (false !== $binaryNameEnvVar && false !== getenv(self::ENV_VERSION)) { | ||
$prompt = sprintf('%s console', $binaryNameEnvVar); | ||
} | ||
|
||
return $prompt; | ||
} | ||
} |
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
<?php | ||
|
||
/* | ||
* This file is part of the Symfony MakerBundle package. | ||
* | ||
* (c) Fabien Potencier <[email protected]> | ||
* | ||
* For the full copyright and license information, please view the LICENSE | ||
* file that was distributed with this source code. | ||
*/ | ||
|
||
namespace Symfony\Bundle\MakerBundle\Tests\Util; | ||
|
||
use PHPUnit\Framework\TestCase; | ||
use Symfony\Bundle\MakerBundle\Util\CliOutputHelper; | ||
|
||
/** | ||
* @author Jesse Rushlow <[email protected]> | ||
*/ | ||
class CliOutputHelperTest extends TestCase | ||
{ | ||
protected function tearDown(): void | ||
{ | ||
putenv('SYMFONY_CLI_BINARY_NAME'); | ||
putenv('SYMFONY_CLI_VERSION'); | ||
} | ||
|
||
public function testCorrectCommandPrefixReturnedWhenUsingSymfonyBinary(): void | ||
{ | ||
self::assertSame('php bin/console', CliOutputHelper::getCommandPrefix()); | ||
|
||
putenv('SYMFONY_CLI_BINARY_NAME=symfony'); | ||
putenv('SYMFONY_CLI_VERSION=0.0.0'); | ||
|
||
self::assertSame('symfony console', CliOutputHelper::getCommandPrefix()); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice 👍 |
||
} |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be integrated directly into symfony/console ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why not. We would need to add getters for the version & binary name (if not now, in the future) should the need arise where we need to conditionally change messaging in maker for some reason..
I can get a PR going in
symfony/console
to get this in... Should it be done as helper like we have here or integrated directly into another part of an existing class?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note - do we have time to get it in for 6.2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not put this directly in symfony/console. We currently have nothing exposing just this info in the component and
php bin/console
is still specific to the Flex skeleton.However,
symfony/console
has an internal logic that would benefit from checkingSYMFONY_CLI_BINARY_NAME
when replacing the placeholders in help texts.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hacked something about this yesterday: https://github.com/symfony/symfony/compare/4.4...tucksaun:symfony:console/symfony-cli-full-command?expand=1
But I didn't submit it because I was afraid of any side effects for third party projects using symfony/console.
Do you want me to open the PR and discuss this there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is exactly the patch I had in mind. But ideally, we would have an env variable covering the whole symfony-cli command instead of hard-coding
console
in symfony/console, as this would cause issues withsymfony composer
for instance.and I'm wondering of the effect when running
symfony php
as the script itself is not wrapped there. so this requires more discussion before being readme (and it won't be for 4.4 as it reached EOM)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exactly the uses case I had in mind and lead me to not submit the patch yet :D
yeah I only targeted 4.4 by lazyness ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say that
symfony-cli
should expose 2 env variables to solve that:symfony console
or justconsole
and expect the consumer to combine it with the existing env var for the binary name)