Skip to content

[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

Merged

Conversation

dem3trio
Copy link
Contributor

@dem3trio dem3trio commented Dec 8, 2018

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

@nicolas-grekas nicolas-grekas added this to the next milestone Dec 8, 2018
@dem3trio dem3trio changed the title [HackDay][DebugBundle] Added html_dumper_theme option to change the color of dump() when ren… [HackDay][DebugBundle] Added 'theme' option to change the color of dump() when ren… Dec 8, 2018
@nicolas-grekas nicolas-grekas changed the title [HackDay][DebugBundle] Added 'theme' option to change the color of dump() when ren… [DebugBundle] Added 'theme' option to change the color of dump() when rendered inside templates Dec 13, 2018
Copy link
Member

@nicolas-grekas nicolas-grekas left a 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')) {
Copy link
Member

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')) {
Copy link
Member

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')) {

Copy link
Contributor Author

@dem3trio dem3trio Dec 24, 2018

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.

Copy link
Member

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!

Copy link
Member

@fabpot fabpot left a 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"')
Copy link
Member

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').

@fabpot
Copy link
Member

fabpot commented Jan 1, 2019

Something went bad when merging, can you re-push your work here? Sorry about that.

@dem3trio dem3trio force-pushed the add_html_dumper_theme_config_option branch from c0bb3a7 to feb6f34 Compare January 1, 2019 23:15
@dem3trio
Copy link
Contributor Author

dem3trio commented Jan 1, 2019

I've made a git push --force it's that enough? If you need anything else, tell me.

@fabpot
Copy link
Member

fabpot commented Jan 2, 2019

It's not enough. You need to execute git rebase origin/master as this will remove the merge commits.

@dem3trio dem3trio force-pushed the add_html_dumper_theme_config_option branch from feb6f34 to a0db35a Compare January 2, 2019 22:33
@dem3trio
Copy link
Contributor Author

dem3trio commented Jan 2, 2019

created new origin "sf" (symfony/symfony)
git pull sf master
git push origin master
git checkout add_html_dumper_theme_config_option
git rebase origin/master
and git push --force

I hope i've done well this time

@fabpot fabpot force-pushed the add_html_dumper_theme_config_option branch from a0db35a to 91e8057 Compare January 3, 2019 03:26
@fabpot
Copy link
Member

fabpot commented Jan 3, 2019

Thank you @dem3trio.

@fabpot fabpot merged commit 91e8057 into symfony:master Jan 3, 2019
fabpot added a commit that referenced this pull request Jan 3, 2019
… 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
@ro0NL ro0NL mentioned this pull request Jan 3, 2019
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019
@fabpot fabpot mentioned this pull request May 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants