NotificationsYou must be signed in to change notification settings
Fork9.6k
Star30.4k

[FrameworkBundle] Expose dotenv in bin/console about#24144

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:3.4fromro0NL:about-env

Oct 3, 2017

Merged

[FrameworkBundle] Expose dotenv in bin/console about#24144

fabpot merged 1 commit intosymfony:3.4fromro0NL:about-env

Oct 3, 2017

Conversation

Copy link

Contributor

ro0NL commentedSep 10, 2017•
edited
Loading

Q	A
Branch?	3.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes/no
Fixed tickets	#...
License	MIT
Doc PR	symfony/symfony-docs#...

For completeness in CLI, shown in web under "Server parameters".

(newDotenv())->populate(['APP_ENV' =>'prod','APP_DEBUG' =>'1']);

carsonbot added Status: Needs Review FrameworkBundle Feature labels

Sep 10, 2017

ro0NL mentioned this pull request

Sep 10, 2017

[WebProfilerBundle] Add a pane with loaded envs#23951

Closed

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

Dotenv doesn't mean much unless you know what is Dotenv. Could we rename this toEnvironment variables loaded by Symfony or something like that? Thanks.

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

Fair point. Let me think of better name, yet i could live with "Dotenv" :) more suggestions welcome.

Copy link

ContributorAuthor

ro0NLSep 11, 2017•
edited
Loading

Choose a reason for hiding this comment

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

What aboutEnvironment variables (dotenv)?

It's enabled by usingDotenv directly, so should be clear.

Copy link

Member

Choose a reason for hiding this comment

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

I'd useEnvironment variables (.env) which seems more "correct". The user defined the vars in a.env file and they don't care about how they are loaded.

Copy link

ContributorAuthor

ro0NLSep 11, 2017•
edited
Loading

Choose a reason for hiding this comment

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

Managed environment variables could do

Copy link

Member

javiereguiluz commentedSep 12, 2017

I'm still not convinced by the naming of this option. Here we say "Environment variables (.env)" but in#23951 we say "Loaded variables".

The naming must be perfectly precise: these are only the env vars loaded by Symfony from your local .env file. They are not all the env vars defined or available in your app.

Copy link

Member

Choose a reason for hiding this comment

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

this will break whengetenv returnsfalse

Copy link

Contributor

Choose a reason for hiding this comment

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

@stof, explode returns an array with one empty string:

>>> explode(',', false)=> [     "",   ]>>>

Copy link

ContributorAuthor

ro0NL commentedSep 12, 2017•
edited
Loading

Here we say "Environment variables (.env)" but in#23951 we say "Loaded variables".

I think it should follow withEnvironment vars (dotenv) or whatever we decide here (or there :)). They should be the same, yes.

Fromsymfony/recipes#176 (comment)

What we could do instead is having a CLI tool that list defined env vars and tell the dev where they are coming from (env var, .env, $_SERVER, ...).

Im worried for a big list of envs, cluttering the output. IMHOabout only cares about the SF ones. System-wide just dophp -r "var_dump($_ENV, $_SERVER);" no?

Same applies for web. System-wide you can check phpinfo or so (available via the profiler already). Yet we decided to list all server parameters (thus including those loaded by dotenv). Space is less an issue here.

To mimic "loaded by dotenv" behavior in web, i suggest to highlight those in the current server parameters table and avoid extra panels etc.. See#23951

And technically these variables are not loaded/set by SF persee, only managed.

Copy link

ContributorAuthor

ro0NL commentedSep 12, 2017•
edited
Loading

@fabpot @javiereguiluz i just realized.. there isbin/console about --help also. Great place for discovery/explanation. That could justify usingDotenv again by explaining what that means with --help.

I like the simplicity it brings forjust describing the feature we're implementing; expose 1) Symfony 2) Dotenv 3) Variables, a.k.a.1)SYMFONY_2)DOTENV_3)VARS.

For this PR that is

1 =about
2 = Dotenv table header
3 = the table is clearly a key/value table

We shouldnt hide this is aboutDotenv IMHO.

Copy link

ContributorAuthor

ro0NL commentedSep 12, 2017

Updated. Latest version;

$ bin/console about -------------------- ---------------------------------   Environment (.env)                                     -------------------- ---------------------------------   APP_ENV              prod                               APP_DEBUG            1                                 -------------------- --------------------------------- $ bin/console about --helpHelp:  The about command displays information about the current Symfony project.    The PHP section displays configuration that might differ between web and CLI.    The Environment section displays the current environment variables managed by Symfony Dotenv. It will not  be shown if no variables were found.

Works for me :)

ro0NL mentioned this pull request

Sep 22, 2017

Fix kernel namesymfony/recipes#186

Closed

yceruto reviewed

Sep 22, 2017

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Command/AboutCommand.php Outdated

		@@ -57,14 +71,12 @@ protected function execute(InputInterface $input, OutputInterface $output)
		new TableSeparator(),
		array('<info>Kernel</>'),
		new TableSeparator(),
		array('Type', get_class($kernel)),
		array('Name', $kernel->getName()),

Copy link

Member

ycerutoSep 22, 2017•
edited
Loading

Choose a reason for hiding this comment

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

Please, don't remove the kernel name info, it is important forName-based Virtual Kernel 4.0 applications :(

edit: I means, it is the way I can to know which app is running according to the kernel name.

Copy link

Contributor

robfrawleySep 22, 2017

Choose a reason for hiding this comment

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

Agreed: please don't remove this output. Regardless, even if there was merit to the change, this seems outside the scope of this PR. We should avoid surprises that aren't directly related to the PR itself; otherwise, people will have difficulty even finding the discussion around the change.

Copy link

ContributorAuthor

ro0NLSep 22, 2017

Choose a reason for hiding this comment

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

Well if we get rid ofKernel::getName() then we're good anyway :)

Maybe i can look into that soonish;symfony/recipes#186 (comment)

In general i agree; if we keep it, it should be displayed here. Tend to remove Kernel type either way, you probably "know" this.

Copy link

ContributorAuthor

ro0NLSep 22, 2017

Choose a reason for hiding this comment

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

Reverted for now :) should be discussed when deprecating getName.

Copy link

Member

fabpotSep 22, 2017

Choose a reason for hiding this comment

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

Let's not introduce something we know will be deprecated. If you really want to display something here, we could use the Kernel class name. That would help differentiate multi-kernel apps.

Copy link

Contributor

robfrawleySep 22, 2017

Choose a reason for hiding this comment

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

We already show the kernel class name, no? Regardless, the issue isn't withintroducing something, but that this PRremoved something unrelated to the stated purpose of this PR.

IMHO, the call togetName() here should be discussed (and possibly removed) when a PR to remove theKernel::getName() method is introduced, not in a PR adding Dotenv output. Right?

Copy link

ContributorAuthor

ro0NL commentedSep 26, 2017

Perhaps we should consider a special command for this as well. Tend to likedebug:env.

See#22406 (comment)

But, the current proposed feature, works for me 👍 although not used in real life projects with many env e.g.

stof approved these changes

Sep 27, 2017

View reviewed changes

fabpot reviewed

Sep 30, 2017

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Command/AboutCommand.php Outdated

		array('Log directory', self::formatPath($kernel->getLogDir(), $kernel->getProjectDir()).' (<comment>'.self::formatFileSize($kernel->getLogDir()).'</>)'),
		array('Root directory', self::formatPath($container->getParameter('kernel.root_dir'), $projectDir)),
		array('Cache directory', self::formatPath($cacheDir = $container->getParameter('kernel.cache_dir'), $projectDir).' (<comment>'.self::formatFileSize($cacheDir).'</>)'),
		array('Log directory', self::formatPath($logDir = $container->getParameter('kernel.logs_dir'), $projectDir).' (<comment>'.self::formatFileSize($logDir).'</>)'),

Copy link

Member

fabpotSep 30, 2017

Choose a reason for hiding this comment

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

Why is it better to use parameters?

Copy link

ContributorAuthor

ro0NLOct 2, 2017•
edited
Loading

Choose a reason for hiding this comment

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

nothing really, it's just thatgetProjectDir is not part ofKernelInterface, and$kernel->getContainer()->getParameter('kernel.some_path') works for all paths / kernel instances.

See also#23624 (comment) where we favoredgetParam over injection.

Copy link

Member

fabpotOct 2, 2017

Choose a reason for hiding this comment

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

I don't like it. I don't buy the "it is not part of KernelInterface". Using a string sounds much worse than using a method.

Copy link

ContributorAuthor

ro0NLOct 3, 2017

Choose a reason for hiding this comment

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

Reverted.

[FrameworkBundle] Expose dotenv in bin/console about

9011f47

fabpot approved these changes

Oct 3, 2017

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Oct 3, 2017

Copy link

Member

fabpot commentedOct 3, 2017

Thank you@ro0NL.

fabpot merged commit9011f47 intosymfony:3.4

Oct 3, 2017

fabpot added a commit that referenced this pull request

Oct 3, 2017

feature#24144[FrameworkBundle] Expose dotenv in bin/console about (…

dcca180

…ro0NL)This PR was merged into the 3.4 branch.Discussion----------[FrameworkBundle] Expose dotenv in bin/console about| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes/no| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License       | MIT| Doc PR        | symfony/symfony-docs#... <!--highly recommended for new features-->For completeness in CLI, shown in web under "Server parameters".```php(new Dotenv())->populate(['APP_ENV' => 'prod', 'APP_DEBUG' => '1']);```![image](https://user-images.githubusercontent.com/1047696/30293203-fe360d0a-9738-11e7-80ed-94901cea8898.png)Commits-------9011f47 [FrameworkBundle] Expose dotenv in bin/console about

ro0NL deleted the about-env branch

October 3, 2017 18:31

This was referencedOct 18, 2017

Release v3.4.0-BETA1#24609

Merged

Release v4.0.0-BETA1#24612

Merged

ro0NL mentioned this pull request

Nov 26, 2017

[WebProfilerBundle] Expose dotenv variables#25166

Closed