[WebProfilerBundle] Remove application name #28939
Conversation
| @@ -126,74 +108,53 @@ | |||
| {% endblock %} | |||
|
|
|||
| {% block panel %} | |||
| {% if collector.applicationname %} | |||
| {# this application is not the Symfony framework #} | |||
There was a problem hiding this comment.
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.
ok. it's ripped out fully now :) should the constructor trigger a deprecation if passed?
There was a problem hiding this comment.
@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.
src/Symfony/Component/HttpKernel/DataCollector/ConfigDataCollector.php
Outdated
Show resolved
Hide resolved
|
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 <!-- see https://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:  or  I think we need to deprecate the argument for real? cc @fabpot Commits ------- f5c355e [WebProfilerBundle] Remove application name
|
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. 👎 |
|
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. |
|
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. |
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 like https://github.com/kendrick-k/symfony-debug-toolbar-git maybe. |
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 <!-- see https://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 #28939 Commits ------- 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