-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[FrameworkBundle] Remove env var table from AboutCommand #34768
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
[FrameworkBundle] Remove env var table from AboutCommand #34768
Conversation
it's considerable to use the new Console dumper (#28898) for env var values, that would improve display as well. The CliDumper has something like diff --git a/Command/AboutCommand.php b/Command/AboutCommand.php
index b9fbe67..feee4e6 100644
--- a/Command/AboutCommand.php
+++ b/Command/AboutCommand.php
@@ -12,6 +12,7 @@
namespace Symfony\Bundle\FrameworkBundle\Command;
use Symfony\Component\Console\Command\Command;
+use Symfony\Component\Console\Helper\Dumper;
use Symfony\Component\Console\Helper\Helper;
use Symfony\Component\Console\Helper\TableSeparator;
use Symfony\Component\Console\Input\InputInterface;
@@ -90,12 +91,13 @@ EOT
];
if ($dotenv = self::getDotenvVars()) {
+ $dumper = new Dumper($io);
$rows = array_merge($rows, [
new TableSeparator(),
['<info>Environment (.env)</>'],
new TableSeparator(),
- ], array_map(function ($value, $name) {
- return [$name, $value];
+ ], array_map(function ($value, $name) use ($dumper) {
+ return [$name, $dumper($value, $maxLength)];
}, $dotenv, array_keys($dotenv)));
} |
@ro0NL I'm not sure. If so, If so, I could take a look into this. |
yep :)
IMHO yes, but perhaps post a screenshot to see what others think. Maybe it's too much indeed ... i was mostly triggered for being able to reuse |
@ro0NL I decided against using So I rewrote it a bit and it looks quite nice. I added a pic Could you please review my code? |
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.
decided against using Dumper since we would rely on CliDumper::setMaxStringWidth() and it is part of VarDumper which is mostly a require-dev package
good point, i forgot we need to account for the fallback implementation as well.
LGTM in general 👍
@ro0NL fixed all |
Not sure about this: while it improves the display, what if I do want to get the full value, eg to copy/paste it somewhere else? |
from experience, i agree the original intend was always "debugging"; give a hint about the actual values being used. Truncating does not defeat this purpose IMHO.
|
Wouldn't it make more sense to just remove this table with env vars? |
hmpf... yes. There's a subtle difference.. (envs managed by .env vs envs used in DI), but i agree it's not really significant. Alternatively, what about displaying |
i tend to agree we should remove the Environment table in The current table was a solution, prior to the later added Also we imply this are the actual values in .env, which is false :) the feature replacement is looking at .env really 😅 |
@ro0NL @nicolas-grekas ok, I see. I pushed a new commit which removes env var table completely. |
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.
probably best to update the PR title
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.
Arthur thanks for this contribution (and congrats for being your first Symfony contribution!). And thanks for your patience during the review process.
Thank you @tuqqu. |
… (tuqqu) This PR was squashed before being merged into the 5.1-dev branch. Discussion ---------- [FrameworkBundle] Remove env var table from AboutCommand | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | - | Deprecations? | - | Tickets | - | License | MIT | Doc PR | - Fixed AboutCommand output by shortening environment variable value (replacing with `...`) to fit the screen width if the value is too long. Right now all output is a mess if it does not fit in the terminal window. Commits ------- 6962da9 [FrameworkBundle] Remove env var table from AboutCommand
Change introduced in 5.1 symfony#34768 ec945f1
Change introduced in 5.1 symfony#34768 ec945f1
…ection was removed (GromNaN) This PR was merged into the 5.3 branch. Discussion ---------- [Framework] Clean "about" command help after Environment section was removed In Symfony 5.1 the "Environment" section of "about" command was removed (see #34768). The help text needs to be updated as a consequence. The sentence "It will not be shown if no variables were found." is particularly puzzling since the section is never shown. | Q | A | ------------- | --- | Branch? | 5.3 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #34768 | License | MIT Commits ------- a4306d2 Clean about command description after Environment section was removed
Fixed AboutCommand output by shortening environment variable value (replacing with
...
) to fit the screen width if the value is too long.Right now all output is a mess if it does not fit in the terminal window.