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

Conversation

jrushlow
Copy link
Collaborator

@jrushlow jrushlow commented Nov 21, 2022

To help with the situations described in symfony/symfony#46705 && #1235

If the user is using the Symfony binary, we check if SYMFONY_CLI_BINARY_NAME && SYMFONY_CLI_VERSION are set, then return the correct command prefix to in the output.

E.g.

// Using the Symfony Binary
$> make:migration

...
Run `symfony console doctrine:migrations:migrate`
...
// Without
$> make:migration

...
Run `php bin/console doctrine:migrations:migrate`
...

We are currently on checking that both of the envVars are set. In the future as we need to add/remove functionality based on the CLI version, we can check for explicit values of the env's if needed.

@jrushlow jrushlow added Feature New Feature Status: Needs Work Additional work is needed labels Nov 21, 2022
Copy link
Contributor

@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

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

I am wondering if there is a better approach and find out if the user is using it actually?

@jrushlow
Copy link
Collaborator Author

jrushlow commented Nov 21, 2022

I am wondering if there is a better approach and find out if the user is using it actually?

I was chewing on that too, but the symfony cli ultimately calls bin/console doctrine:migrations:migrate. The only way that I could think of is if the cli passed an additional flag like --from-symfony-cli to any make:* commands.. But I'm not sure that is something even worth doing...

Definitely open to idea's - It would be nice to be able conditionally show the correct message...

@OskarStark
Copy link
Contributor

friendly ping @tucksaun

@tucksaun
Copy link

IIRC we didn't hide the original PHP binary path to prevent any compatibility issues.
So we can't make this transparent. But we might expose this to the Console component or to Maker Bundler via an environment variable?

@weaverryan
Copy link
Member

But we might expose this to the Console component or to Maker Bundler via an environment variable?

That... WOULD be cool yes :). Is that a fairly simple thing?

@tucksaun
Copy link

The CLI part is easy to do: agreeing on the environment variable name(s) and content is the most complicated thing.
Then there's the PHP part where it depends how far you want to push things.

@jrushlow
Copy link
Collaborator Author

@tucksaun If you can expose an env like SYMFONY_CLI_VERSION: 1.0.0 I think that would do the trick. In MakerBundle we can check if that env exists then display the appropriate message. Using the version would also "future proof" things on our side - e.g. if we need to conditionally offer a feature based on the cli version..

@tucksaun
Copy link

but Symfony CLI binary can have another name that symfony, so we might want to expose both version and current binary name.

@jrushlow
Copy link
Collaborator Author

jrushlow commented Nov 22, 2022

That would work too.. So long as whatever env name we use is consistent across releases so we can determine if the user is using a binary.. Or perhaps setting 2 env's. e.g. SYMFONY_CLI_NAME: some-binary-name && SYMFONY_CLI_VERSION: 1.0.0

The actual names of the env vars don't really matter.. I think whatever works best on your side of the fence will work for us...

tucksaun added a commit to tucksaun/symfonycli that referenced this pull request Nov 24, 2022
…and `composer` wrappers

Exposing those info via environment variables will allow better Symfony Maker experience, see symfony/maker-bundle#1238
tucksaun added a commit to tucksaun/symfonycli that referenced this pull request Nov 24, 2022
…and `composer` wrappers

Exposing that info via environment variables will allow a better Symfony Maker experience, see symfony/maker-bundle#1238
@tucksaun
Copy link

see symfony-cli/symfony-cli#231

let me know if that would work for you this way.

tucksaun added a commit to tucksaun/symfonycli that referenced this pull request Nov 24, 2022
…and `composer` wrappers

Exposing that info via environment variables will allow a better Symfony Maker experience, see symfony/maker-bundle#1238
@tucksaun
Copy link

CLI's PR has been merged so you can now rely on the presence of SYMFONY_CLI_BINARY_NAME as an envidronment variable

@OskarStark
Copy link
Contributor

OskarStark commented Nov 25, 2022

That will bring a much better DX 😃👍🏻 thanks

@jrushlow jrushlow marked this pull request as ready for review November 28, 2022 12:18
@jrushlow jrushlow added Status: Needs Review Needs to be reviewed and removed Status: Needs Work Additional work is needed labels Nov 28, 2022
@jrushlow
Copy link
Collaborator Author

Thank you @tucksaun for making this possible in the binary!

src/Util/CliOutputHelper.php Outdated Show resolved Hide resolved
$this->assertStringContainsString('Success', $output);
$this->assertStringContainsString('php bin/console make:migration', $output);
}),
];
Copy link
Member

Choose a reason for hiding this comment

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

Let's just add a test for the correct command to ONE of these makers (not all of them) just as a smoke test. It's too much added test code (and test running time) for such a minor detail.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done - Using make:migration for the smoke test

tests/Util/CliOutputHelperTest.php Outdated Show resolved Hide resolved
$envs = [
'version' => getenv(self::ENV_VERSION),
'name' => getenv(self::ENV_BIN_NAME),
];
Copy link
Member

Choose a reason for hiding this comment

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

Why not?

$binaryNameEnvVar = getenv(self::ENV_BIN_NAME);

if (false !== getenv(self::ENV_VERSION) && false !== $binaryNameEnvVar) {

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 👍

@jrushlow jrushlow added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Nov 28, 2022
@jrushlow jrushlow merged commit 03e3f83 into symfony:main Nov 28, 2022
@jrushlow jrushlow deleted the feature/better-whats-next branch November 28, 2022 16:20
*
* @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)

@jrushlow jrushlow mentioned this pull request May 23, 2023
weaverryan added a commit that referenced this pull request Jun 7, 2023
This PR was merged into the 1.0-dev branch.

Discussion
----------

[git-released] v1.49.0

# Changelog

## [v1.49.0](https://github.com/symfony/maker-bundle/releases/tag/v1.49.0)

*Jun 7th, 2023*

### Feature

- [#1321](#1321) - Changing make:stimulus-controller to require StimulusBundle - *`@weaverryan`*
- [#1309](#1309) - Apply `get_class_to_class_keyword` PHP-CS-Fixer rule - *`@seb`-jean*
- [#1276](#1276) - [make:migration] Change message when required package for migration doesn't exist. - *`@bdaler`*
- [#1261](#1261) - [make:registration-form] use UniqueEntity attribute instead of annotation - *`@jrushlow`*
- [#1253](#1253) - [make:migration] Add link to new migration files - *`@nicolas`-grekas*
- [#1251](#1251) - [make:*] use php-cs-fixer to style/lint all generated php templates - *`@jrushlow`*
- [#1244](#1244) - [make:security:form-login] new maker to use built in FormLogin - *`@jrushlow`*
- [#1242](#1242) - [make:*] use static return type instead of self for setters - *`@jrushlow`*
- [#1239](#1239) - Improve error messages to show PHP & XML configurations are not supported - *`@ThomasLandauer`*
- [#1238](#1238) - [make:*] improve output messages for Symfony CLI users - *`@jrushlow`*
- [#1237](#1237) - [make:registration-form] Print registration form errors - *`@comxd`*
### Bug

- [#1307](#1307) - [make:twig-component] handle upstream changes to how live components are rendered - *`@jrushlow`*
- [#1270](#1270) - [make:authenticator] Core\Security or SecurityBundle\Security - Avoid deprecations in 6.2 - *`@nacorp`*
- [#1265](#1265) - [make:crud] Make sensio/framework-extra-bundle an optional dependency - *`@acrobat`*
- [#1264](#1264) - [make:controller] doctrine/annotations is not needed - *`@jrushlow`*
- [#1262](#1262) - [make:reset-password] doctrine/annotations are not needed - *`@jrushlow`*

_Generated by Git Released_

Commits
-------

a7bb5c6 Updating date, adding 1 more PR
cb06c33 [release] v1.49.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Feature Status: Reviewed Has been reviewed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants