Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[WebProfilerBundle] Remove application name#28939
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| {%blockpanel %} | ||
| {%ifcollector.applicationname %} | ||
| {# this application is not the Symfony framework#} |
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.
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.
I think we can remove both application name and application version. It was used by Silex, but not needed anymore. Let's simplify.
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.
ok. it's ripped out fully now :) should the constructor trigger a deprecation if passed?
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.
@fabpot actually it was used also by our apps to show app's name and version (from tag or from branch's name). Too bad it was removed.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
javiereguiluz left a comment
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.
This is ready, except for the minor change asked by Kévin. Thanks Roland!
ro0NL left a comment
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.
should be good now
src/Symfony/Component/HttpKernel/DataCollector/ConfigDataCollector.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
nicolas-grekas commentedOct 23, 2018
Thank you@ro0NL. |
This PR was squashed before being merged into the 4.2-dev branch (closes#28939).Discussion----------[WebProfilerBundle] Remove application name| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | yes| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | yes| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | symfony/symfony-docs#... <!-- required for new features -->This fixes a regression in master, i think after#28809 (or something related)There's still a "application name" concept out there today, and it's `ConfigDataCollector::getApplicationName()` 🤔In my case the service resolved to```php$e = new \Symfony\Component\HttpKernel\DataCollector\ConfigDataCollector(\basename('/app'));```Causing the app name to be, well, `app` (again :P). It was really confusing.Before:(the "a" is from "app" here)After:orI think we need to deprecate the argument for real?cc@fabpotCommits-------f5c355e [WebProfilerBundle] Remove application name
Wirone commentedNov 4, 2018
We've been using this option for displaying app's name and version (based on tag, passed in CI to built Docker image), even wrote custom compiler pass for it in order to pass those nonconfigurable arguments... :/ This is weird, IMO it should be changed in totally opposite way, to allow setting name and version through configuration. 👎 |
ro0NL commentedNov 4, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
IMHO this is not a feature for SF to maintain, and causes to bypass all core information which your app still relies on. In general i dont see the value of custom branding sort of speak, thru a custom appname/version in the toolbar as a core feature, unless a custom data collector does that. |
Wirone commentedNov 4, 2018
Why should I write custom data collector just for displaying app's name and version if there was a perfect and simple built-in feature for handling it? Of course, those arguments were orphaned and not configurable on framework level, but IMO it's so common task to display such data so it should be part of the framework and configuration options should be added instead of removing it. This is really simple to maintain. From users' (and developers' too) point of view version of the app is more important than version of the framework used for building the app (not to mention that Symfony version is displayed in the collector's expanded content, not on the toolbar, so I don't know what "bypass" you're talking about). We have CD process for deploying DEV instance from Maybe month ago I asked on the Slack how to customize toolbar to show such info, people were rather confused. I am sure that it's because lack of documentation on this topic, not lack of need of such usage. |
ro0NL commentedNov 5, 2018
Not perfect :)
Personally i disagree.
Not sure, apparently it was totally forgotten in core :)
Personally i disagree. At least both are equally important then IMHO.
The detailed SF information in the web profiler. You're right thouh the information is still available on hover in the toolbar. In general, to put GIT info in the toolbar/webprofiler i suggest i.e.https://medium.com/@smaine.milianni/display-git-informations-in-the-symfony-webprofiler-521e4d7595de or something likehttps://github.com/kendrick-k/symfony-debug-toolbar-git maybe. |
Wirone commentedNov 5, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Trust me, our Product Owner and people from business who are our customers don't care about Symfony version. It's important only for developers and it was there, on hover. Still, it's low level information, even for developers less important than app's version. As I said, we build Docker images in CI, and populate version/revision (from CI variables) to the image as env variables. We don't have Git there, so these solutions are useless for us. Anyway, I just wanted to signal that there are people using this removed feature.@fabpot decided to remove it, I think it's bad idea and I wrote it. Can't do more. |
This PR was merged into the 5.0-dev branch.Discussion----------[HttpKernel] Cleanup legacy in ConfigDataCollector| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | symfony/symfony-docs#... <!-- required for new features -->See#28939Commits-------09ca33c [HttpKernel] Cleanup legacy in ConfigDataCollector

This fixes a regression in master, i think after#28809 (or something related)
There's still a "application name" concept out there today, and it's
ConfigDataCollector::getApplicationName()🤔In my case the service resolved to
Causing the app name to be, well,
app(again :P). It was really confusing.Before:
(the "a" is from "app" here)
After:
or
I think we need to deprecate the argument for real?
cc@fabpot