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

[make:*] improve output messages for Symfony CLI users #1238

Merged
merged 12 commits into from
Nov 28, 2022
3 changes: 2 additions & 1 deletion src/Maker/MakeEntity.php
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
use Symfony\Bundle\MakerBundle\Str;
use Symfony\Bundle\MakerBundle\Util\ClassDetails;
use Symfony\Bundle\MakerBundle\Util\ClassSourceManipulator;
use Symfony\Bundle\MakerBundle\Util\CliOutputHelper;
use Symfony\Bundle\MakerBundle\Util\PhpCompatUtil;
use Symfony\Bundle\MakerBundle\Validator;
use Symfony\Component\Console\Command\Command;
Expand Down Expand Up @@ -310,7 +311,7 @@ public function generate(InputInterface $input, ConsoleStyle $io, Generator $gen

$this->writeSuccessMessage($io);
$io->text([
'Next: When you\'re ready, create a migration with <info>php bin/console make:migration</info>',
sprintf('Next: When you\'re ready, create a migration with <info>%s make:migration</info>', CliOutputHelper::getCommandPrefix()),
'',
]);
}
Expand Down
3 changes: 2 additions & 1 deletion src/Maker/MakeMigration.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
use Symfony\Bundle\MakerBundle\DependencyBuilder;
use Symfony\Bundle\MakerBundle\Generator;
use Symfony\Bundle\MakerBundle\InputConfiguration;
use Symfony\Bundle\MakerBundle\Util\CliOutputHelper;
use Symfony\Component\Console\Application;
use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Input\ArgvInput;
Expand Down Expand Up @@ -121,7 +122,7 @@ public function generate(InputInterface $input, ConsoleStyle $io, Generator $gen

$io->text([
sprintf('Next: Review the new migration <info>%s</info>', $migrationName),
'Then: Run the migration with <info>php bin/console doctrine:migrations:migrate</info>',
sprintf('Then: Run the migration with <info>%s doctrine:migrations:migrate</info>', CliOutputHelper::getCommandPrefix()),
'See <fg=yellow>https://symfony.com/doc/current/bundles/DoctrineMigrationsBundle/index.html</>',
]);
}
Expand Down
3 changes: 2 additions & 1 deletion src/Maker/MakeRegistrationForm.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
use Symfony\Bundle\MakerBundle\Util\ClassDetails;
use Symfony\Bundle\MakerBundle\Util\ClassNameDetails;
use Symfony\Bundle\MakerBundle\Util\ClassSourceManipulator;
use Symfony\Bundle\MakerBundle\Util\CliOutputHelper;
use Symfony\Bundle\MakerBundle\Util\UseStatementGenerator;
use Symfony\Bundle\MakerBundle\Util\YamlSourceManipulator;
use Symfony\Bundle\MakerBundle\Validator;
Expand Down Expand Up @@ -434,7 +435,7 @@ private function successMessage(ConsoleStyle $io, bool $emailVerification, strin
$closing[] = ' * Customize the last <fg=yellow>redirectToRoute()</> after a successful email verification.';
$closing[] = ' * Make sure you\'re rendering <fg=yellow>success</> flash messages or change the <fg=yellow>$this->addFlash()</> line.';
$closing[] = sprintf('%d) Review and customize the form, controller, and templates as needed.', $index++);
$closing[] = sprintf('%d) Run <fg=yellow>"php bin/console make:migration"</> to generate a migration for the newly added <fg=yellow>%s::isVerified</> property.', $index++, $userClass);
$closing[] = sprintf('%d) Run <fg=yellow>"%s make:migration"</> to generate a migration for the newly added <fg=yellow>%s::isVerified</> property.', $index++, CliOutputHelper::getCommandPrefix(), $userClass);
}

$io->text($closing);
Expand Down
3 changes: 2 additions & 1 deletion src/Maker/MakeResetPassword.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
use Symfony\Bundle\MakerBundle\Security\InteractiveSecurityHelper;
use Symfony\Bundle\MakerBundle\Util\ClassNameDetails;
use Symfony\Bundle\MakerBundle\Util\ClassSourceManipulator;
use Symfony\Bundle\MakerBundle\Util\CliOutputHelper;
use Symfony\Bundle\MakerBundle\Util\UseStatementGenerator;
use Symfony\Bundle\MakerBundle\Util\YamlSourceManipulator;
use Symfony\Bundle\MakerBundle\Validator;
Expand Down Expand Up @@ -384,7 +385,7 @@ private function setBundleConfig(ConsoleStyle $io, Generator $generator, string
private function successMessage(InputInterface $input, ConsoleStyle $io, string $requestClassName): void
{
$closing[] = 'Next:';
$closing[] = sprintf(' 1) Run <fg=yellow>"php bin/console make:migration"</> to generate a migration for the new <fg=yellow>"%s"</> entity.', $requestClassName);
$closing[] = sprintf(' 1) Run <fg=yellow>"%s make:migration"</> to generate a migration for the new <fg=yellow>"%s"</> entity.', CliOutputHelper::getCommandPrefix(), $requestClassName);
$closing[] = ' 2) Review forms in <fg=yellow>"src/Form"</> to customize validation and labels.';
$closing[] = ' 3) Review and customize the templates in <fg=yellow>`templates/reset_password`</>.';
$closing[] = ' 4) Make sure your <fg=yellow>MAILER_DSN</> env var has the correct settings.';
Expand Down
23 changes: 12 additions & 11 deletions src/Test/MakerTestEnvironment.php
Original file line number Diff line number Diff line change
Expand Up @@ -170,15 +170,16 @@ public function runCommand(string $command): MakerTestProcess
return MakerTestProcess::create($command, $this->path)->run();
}

public function runMaker(array $inputs, string $argumentsString = '', bool $allowedToFail = false): MakerTestProcess
public function runMaker(array $inputs, string $argumentsString = '', bool $allowedToFail = false, array $envVars = []): MakerTestProcess
{
// Let's remove cache
$this->fs->remove($this->path.'/var/cache');

$testProcess = $this->createInteractiveCommandProcess(
$this->testDetails->getMaker()::getCommandName(),
$inputs,
$argumentsString
commandName: $this->testDetails->getMaker()::getCommandName(),
userInputs: $inputs,
argumentsString: $argumentsString,
envVars: $envVars,
);

$this->runnedMakerProcess = $testProcess->run($allowedToFail);
Expand Down Expand Up @@ -329,16 +330,16 @@ public function processReplacement(string $rootDir, string $filename, string $fi
file_put_contents($path, str_replace($find, $replace, $contents));
}

public function createInteractiveCommandProcess(string $commandName, array $userInputs, string $argumentsString = ''): MakerTestProcess
public function createInteractiveCommandProcess(string $commandName, array $userInputs, string $argumentsString = '', array $envVars = []): MakerTestProcess
{
$envVars = array_merge(['SHELL_INTERACTIVE' => '1'], $envVars);

// We don't need ansi coloring in tests!
$process = MakerTestProcess::create(
sprintf('php bin/console %s %s --no-ansi', $commandName, $argumentsString),
$this->path,
[
'SHELL_INTERACTIVE' => '1',
],
10
commandLine: sprintf('php bin/console %s %s --no-ansi', $commandName, $argumentsString),
cwd: $this->path,
envVars: $envVars,
timeout: 10
);

if ($userInputs) {
Expand Down
4 changes: 2 additions & 2 deletions src/Test/MakerTestRunner.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,9 @@ public function __construct(
$this->filesystem = new Filesystem();
}

public function runMaker(array $inputs, string $argumentsString = '', bool $allowedToFail = false): string
public function runMaker(array $inputs, string $argumentsString = '', bool $allowedToFail = false, array $envVars = []): string
{
$this->executedMakerProcess = $this->environment->runMaker($inputs, $argumentsString, $allowedToFail);
$this->executedMakerProcess = $this->environment->runMaker($inputs, $argumentsString, $allowedToFail, $envVars);

return $this->executedMakerProcess->getOutput();
}
Expand Down
47 changes: 47 additions & 0 deletions src/Util/CliOutputHelper.php
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
Copy link
Contributor

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 ?

Copy link
Collaborator Author

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?

Copy link
Collaborator Author

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?

Copy link
Member

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 checking SYMFONY_CLI_BINARY_NAME when replacing the placeholders in help texts.

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?

Copy link
Member

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 with symfony 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)

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 ;)

Copy link
Member

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:

  • an env variable exposing the wrapper command (to be decided whether we expose symfony console or just console and expect the consumer to combine it with the existing env var for the binary name)
  • an env variable exposing whether the wrapper command is only wrapping PHP itself or also the PHP script (to decide whether it replaces the script name or only prefix it in symfony/console)

{
/**
* 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;
}
}
23 changes: 23 additions & 0 deletions tests/Maker/MakeMigrationTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use Symfony\Bundle\MakerBundle\Test\MakerTestCase;
use Symfony\Bundle\MakerBundle\Test\MakerTestDetails;
use Symfony\Bundle\MakerBundle\Test\MakerTestRunner;
use Symfony\Bundle\MakerBundle\Util\CliOutputHelper;
use Symfony\Component\Finder\Finder;

class MakeMigrationTest extends MakerTestCase
Expand Down Expand Up @@ -65,6 +66,28 @@ public function getTestDetails(): \Generator
}),
];

yield 'it_detects_symfony_cli_usage' => [$this->createMakeMigrationTest()
->run(function (MakerTestRunner $runner) {
$output = $runner->runMaker(
inputs: [],
envVars: [CliOutputHelper::ENV_VERSION => '0.0.0', CliOutputHelper::ENV_BIN_NAME => 'symfony']
);

$this->assertStringContainsString('symfony console doctrine:migrations:migrate', $output);
}),
];

yield 'it_detects_symfony_cli_is_not_used' => [$this->createMakeMigrationTest()
->run(function (MakerTestRunner $runner) {
$output = $runner->runMaker(
inputs: [],
envVars: []
);

$this->assertStringContainsString('php bin/console doctrine:migrations:migrate', $output);
}),
];

yield 'it_generates_migration_with_no_changes' => [$this->createMakeMigrationTest()
->run(function (MakerTestRunner $runner) {
// sync so there are no changes
Expand Down
37 changes: 37 additions & 0 deletions tests/Util/CliOutputHelperTest.php
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());
}
Copy link
Member

Choose a reason for hiding this comment

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

Nice 👍

}