-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[DebugBundle] Added 'theme' option to change the color of dump() when rendered inside templates #29528
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
[DebugBundle] Added 'theme' option to change the color of dump() when rendered inside templates #29528
Conversation
src/Symfony/Bundle/DebugBundle/DependencyInjection/Configuration.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/DebugBundle/DependencyInjection/DebugExtension.php
Outdated
Show resolved
Hide resolved
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.
LGTM with some minor comments.
@@ -53,6 +54,19 @@ public function getConfigTreeBuilder() | |||
->end() | |||
; | |||
|
|||
if (class_exists(HtmlDumper::class) && method_exists(HtmlDumper::class, 'setTheme')) { |
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.
can be simplified:
if (method_exists(HtmlDumper::class, 'setTheme')) {
@@ -42,6 +43,11 @@ public function load(array $configs, ContainerBuilder $container) | |||
->addMethodCall('setMinDepth', array($config['min_depth'])) | |||
->addMethodCall('setMaxString', array($config['max_string_length'])); | |||
|
|||
if (class_exists(HtmlDumper::class) && method_exists(HtmlDumper::class, 'setTheme')) { |
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.
if ('dark' !== $config['theme'] && method_exists(HtmlDumper::class, 'setTheme')) {
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.
The 'dark' !== $config['theme']
will not launch an undefinex index notice with var dumper 4.1?
In that case, the Configuration class will not initialize the $config['theme'] index because 'setTheme' doesn't exists.
it wouldn't be better to set something like this?:
if (isset($config['theme'] && 'dark' !== $config['theme'] && method_exists(HtmlDumper::class, 'setTheme')) {
Or maybe leave it just as:
if (method_exists(HtmlDumper::class, 'setTheme')) {
as it appears in the Configuration 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.
Correct. if (method_exists(HtmlDumper::class, 'setTheme') && 'dark' !== $config['theme']) {
then!
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.
with a minor comment
->children() | ||
->enumNode('theme') | ||
->info('Changes the color of the dump() output when rendered directly on the templating. "dark" (default) or "light"') | ||
->example('"dark"') |
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.
The other example
calls we have do not wrap strings, should probably be ->example('dark')
.
feb6f34
to
c0bb3a7
Compare
Something went bad when merging, can you re-push your work here? Sorry about that. |
c0bb3a7
to
feb6f34
Compare
I've made a git push --force it's that enough? If you need anything else, tell me. |
It's not enough. You need to execute |
feb6f34
to
a0db35a
Compare
created new origin "sf" (symfony/symfony) I hope i've done well this time |
… rendered inside templates
a0db35a
to
91e8057
Compare
Thank you @dem3trio. |
… of dump() when rendered inside templates (dem3trio) This PR was squashed before being merged into the 4.3-dev branch (closes #29528). Discussion ---------- [DebugBundle] Added 'theme' option to change the color of dump() when rendered inside templates | Q | A | ------------- | --- | Branch? | master for features | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT Added a html_dumper_theme option config to use the new HtmlDumper light theme when using dump() inside templates #SymfonyConHackday2018 Commits ------- 91e8057 [DebugBundle] Added 'theme' option to change the color of dump() when rendered inside templates
Added a html_dumper_theme option config to use the new HtmlDumper light theme when using dump() inside templates
#SymfonyConHackday2018