Skip to content

[WebProfilerBundle] Try to display the most useful panel by default #33783

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
Merged
merged 1 commit into from
Oct 2, 2019
Merged

[WebProfilerBundle] Try to display the most useful panel by default #33783

merged 1 commit into from
Oct 2, 2019

Conversation

fancyweb
Copy link
Contributor

@fancyweb fancyweb commented Oct 1, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? yes
Deprecations? yes
Tickets -
License MIT
Doc PR -

Alternative to #32491, the goal stays the same.

I think reserving a data collector name is fine, isn'it ? It's not likely that end users use this name (that's why I added an underscore) + shouldn't be hard for them to just rename it.

I don't think adding a configuration option to toggle the "_best" behavior is useful, should be by default for DX IMHO.

Not adding an extension point for now (for end users to set their panel as the "best"), maybe later if someone request it?

@fancyweb
Copy link
Contributor Author

fancyweb commented Oct 1, 2019

Personally I still prefer #32491 btw because the profile link is directly the right one, there is no special magic panel name.

However, I recognize the implementation here is more straightforward and easier to understand / maintain. It was such a great alternative thinking idea!

@stof stof mentioned this pull request Oct 1, 2019
Closed
@stof
Copy link
Member

stof commented Oct 1, 2019

Question here: should we reserve a _best name, or should we perform the guessing when no panel is specified in the query string (instead of defaulting to request in that case) ?

@fancyweb fancyweb force-pushed the web-profiler-bundle-panel-auto branch 3 times, most recently from c0c34d6 to 66c5c12 Compare October 1, 2019 09:44
@javiereguiluz
Copy link
Member

If technically possible, my preference would be to guess when no param is passed. Otherwise, I'd prefer to rename _best to _auto. Thanks.

@stof
Copy link
Member

stof commented Oct 1, 2019

@javiereguiluz it is totally possible, by dropping the default panel when getting the panel from the request, and then performing guessing when $panel is null (and using request as the fallback for guessing, as already done here by reusing $defaultPanel)

@fancyweb fancyweb force-pushed the web-profiler-bundle-panel-auto branch from 66c5c12 to 9690405 Compare October 1, 2019 10:23
@fancyweb
Copy link
Contributor Author

fancyweb commented Oct 1, 2019

@stof Your idea looks good to me, I just updated the code. And at least there is no debate on the name 😁

@fancyweb fancyweb force-pushed the web-profiler-bundle-panel-auto branch 2 times, most recently from 6776a43 to 777110e Compare October 1, 2019 10:26
@fancyweb fancyweb changed the title [WebProfilerBundle] Add special "_best" panel [WebProfilerBundle] Try to display the most useful panel by default Oct 1, 2019
yceruto
yceruto approved these changes Oct 1, 2019
View reviewed changes
Copy link
Member

@yceruto yceruto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this approach, thank you!

@yceruto
Copy link
Member

yceruto commented Oct 1, 2019

There are some pending warnings https://travis-ci.org/symfony/symfony/jobs/591950102#L4498

@fancyweb fancyweb force-pushed the web-profiler-bundle-panel-auto branch 2 times, most recently from 3b1a977 to 7d94e29 Compare October 1, 2019 13:42
@fancyweb
Copy link
Contributor Author

fancyweb commented Oct 1, 2019

Tests will need #33792 to pass.

@nicolas-grekas nicolas-grekas added this to the next milestone Oct 2, 2019
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Oct 2, 2019 •
edited
Loading

rebase unlocked, please apply the changes on src/Symfony/Bundle/WebProfilerBundle/Tests/Profiler/TemplateManagerTest.php as I didn't merge them from 3.4

@fancyweb fancyweb force-pushed the web-profiler-bundle-panel-auto branch 2 times, most recently from 2e1af14 to a0079a4 Compare October 2, 2019 09:01
@fancyweb fancyweb force-pushed the web-profiler-bundle-panel-auto branch from a0079a4 to a45dd98 Compare October 2, 2019 09:17
@fancyweb
Copy link
Contributor Author

fancyweb commented Oct 2, 2019

Done, failures are unrelated.

@nicolas-grekas
Copy link
Member

Thank you @fancyweb.

nicolas-grekas added a commit that referenced this pull request Oct 2, 2019
@nicolas-grekas
feature #33783 [WebProfilerBundle] Try to display the most useful pan…
4d6fe5c 
…el by default (fancyweb)

This PR was merged into the 4.4 branch.

Discussion
----------

[WebProfilerBundle] Try to display the most useful panel by default

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | yes
| Tickets       | -
| License       | MIT
| Doc PR        | -

Alternative to #32491, the goal stays the same.

I think reserving a data collector name is fine, isn'it ? It's not likely that end users use this name (that's why I added an underscore) + shouldn't be hard for them to just rename it.

I don't think adding a configuration option to toggle the "_best" behavior is useful, should be by default for DX IMHO.

Not adding an extension point for now (for end users to set their panel as the "best"), maybe later if someone request it?

Commits
-------

a45dd98 [WebProfilerBundle] Try to display the most useful panel by default
@nicolas-grekas nicolas-grekas merged commit a45dd98 into symfony:4.4 Oct 2, 2019
@fancyweb fancyweb deleted the web-profiler-bundle-panel-auto branch October 2, 2019 12:26
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
This was referenced Nov 12, 2019
Merged
Merged
@jsfgreen
Copy link

jsfgreen commented Dec 11, 2019 •
edited
Loading

Is there something that was changed here that would cause deprecation notices to stop getting displayed in the toolbar? I don't see any deprecation notices anywhere for anything since I upgraded an old project to symfony 4.4 and moved my configs over to the SF4 style.

Or is there some hidden configuration setting somewhere that would tell symfony to silence deprecation notices? I rely heavily on those even internally when we deprecate our own features.

@fancyweb
Copy link
Contributor Author

fancyweb commented Dec 11, 2019 •
edited
Loading

Nope, this PR only changed the default displayed panel. This is not related to deprecation notices nor to the toolbar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof approved these changes

@yceruto yceruto yceruto approved these changes

Assignees
No one assigned
Labels
Deprecation Feature Status: Reviewed WebProfilerBundle
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
7 participants
@fancyweb @stof @javiereguiluz @yceruto @nicolas-grekas @jsfgreen @carsonbot