Skip to content

[VarDumper] Expand nested properties by default. Reverts #35959 #39523

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[VarDumper] Expand nested properties by default. Reverts #35959 #39523

wants to merge 1 commit into from

Conversation

rodrigopedra
Copy link

Q A
Branch? 5.2
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #39394
License MIT
Doc PR N/A

This PR reverts changes made by PR #35959

PR #35959 changed the VarDumper HTML output to be collapsed by default instead of expanded by default.

This prevents developers to debug HTML response on a browser's developer tools' network tab preview pane.

Some developers relied on the output to be expanded by default when debugging AJAX responses. As the preview pane on the network tab doesn't run JavaScript, after PR #35959 one could not see the expanded properties anymore.

Just for the record, PR #35959 addresses issue #35800, where a user asked for changing the behavior to be collapsed by default as in their machine there was some flickering as the collapse was done by JavaScript after rendering.

This PR just reverts to the previous behavior, it does not attempt to fix or workaround the request from issue #35800, so maybe that could be reopened for further assessment.

More info can be found on the discussion on ticket #39394

@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 5.x branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@rodrigopedra rodrigopedra mentioned this pull request Dec 16, 2020
Closed
@nicolas-grekas
Copy link
Member

/cc @ln-dim FYI, this proposes reverting your PR, and the arguments make sense. WDYT?

@nicolas-grekas
Copy link
Member

A middle ground could be to use JS to activate the CSS rule that hides the collapsed nodes.

@rodrigopedra rodrigopedra changed the title [VarDumper] Expand nested properties by defaulr revert #35959 [VarDumper] Expand nested properties by default revert #35959 Dec 16, 2020
@rodrigopedra rodrigopedra changed the title [VarDumper] Expand nested properties by default revert #35959 [VarDumper] Expand nested properties by default. Reverts #35959 Dec 16, 2020
@nicolas-grekas nicolas-grekas added this to the 5.2 milestone Dec 16, 2020
@nicolas-grekas nicolas-grekas mentioned this pull request Dec 16, 2020
Merged
@nicolas-grekas
Copy link
Member

I gave my last proposal a try, here it is: #39525
Thanks for having a look and finding the root cause!

@ln-dim
Copy link
Contributor

ln-dim commented Dec 16, 2020

I didn't think about using dump()/dd() that way.
Let's see if #39525 solves the problem.
If not, I will try again.

nicolas-grekas added a commit that referenced this pull request Dec 17, 2020
@nicolas-grekas
bug #39525 [VarDumper] dont hide any nodes until JS is proven to work…
3741b06 
… (nicolas-grekas)

This PR was merged into the 5.2 branch.

Discussion
----------

[VarDumper] dont hide any nodes until JS is proven to work

| Q             | A
| ------------- | ---
| Branch?       | 5.2
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #39394
| License       | MIT
| Doc PR        | -

Replaces #39523

Commits
-------

42ad1ec [VarDumper] dont hide any nodes until JS is proven to work
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug Status: Needs Review VarDumper
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.

The function dd() is not working in laravel
4 participants
@rodrigopedra @carsonbot @nicolas-grekas @ln-dim