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

[DebugBundle] Added 'theme' option to change the color of dump() when rendered inside templates #29528

Merged

Conversation

Projects
None yet
4 participants
@dem3trio
Copy link
Contributor

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

@nicolas-grekas
Copy link
Member

left a comment

LGTM with some minor comments.

@@ -53,6 +54,19 @@ public function getConfigTreeBuilder()
->end()
;
if (class_exists(HtmlDumper::class) && method_exists(HtmlDumper::class, 'setTheme')) {

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Dec 24, 2018

Member

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

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Dec 24, 2018

Member

if ('dark' !== $config['theme'] && method_exists(HtmlDumper::class, 'setTheme')) {

This comment has been minimized.

Copy link
@dem3trio

dem3trio Dec 24, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Dec 24, 2018

Member

Correct. if (method_exists(HtmlDumper::class, 'setTheme') && 'dark' !== $config['theme']) { then!

@fabpot

fabpot approved these changes Dec 26, 2018

Copy link
Member

left a comment

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

This comment has been minimized.

Copy link
@fabpot

fabpot Dec 26, 2018

Member

The other example calls we have do not wrap strings, should probably be ->example('dark').

@fabpot fabpot force-pushed the dem3trio:add_html_dumper_theme_config_option branch from feb6f34 to c0bb3a7 Jan 1, 2019

@dem3trio dem3trio requested review from dunglas, lyrixx, sroze and xabbuh as code owners Jan 1, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Jan 1, 2019

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

@dem3trio dem3trio force-pushed the dem3trio:add_html_dumper_theme_config_option branch from c0bb3a7 to feb6f34 Jan 1, 2019

@dem3trio

This comment has been minimized.

Copy link
Contributor Author

commented Jan 1, 2019

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

@fabpot

This comment has been minimized.

Copy link
Member

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 dem3trio:add_html_dumper_theme_config_option branch from feb6f34 to a0db35a Jan 2, 2019

@dem3trio

This comment has been minimized.

Copy link
Contributor Author

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 dem3trio:add_html_dumper_theme_config_option branch from a0db35a to 91e8057 Jan 3, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

Thank you @dem3trio.

@fabpot fabpot merged commit 91e8057 into symfony:master Jan 3, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Jan 3, 2019

feature #29528 [DebugBundle] Added 'theme' option to change the color…
… 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 referenced this pull request Jan 3, 2019

Closed

Disable var-dumper search #29748

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.