Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged
nicolas-grekas merged 1 commit intosymfony:masterfromro0NL:appname
Oct 23, 2018
Merged

[WebProfilerBundle] Remove application name#28939

nicolas-grekas merged 1 commit intosymfony:masterfromro0NL:appname
Oct 23, 2018

Conversation

@ro0NL
Copy link
Contributor

QA
Branch?master
Bug fix?yes
New feature?yes
BC breaks?no
Deprecations?yes
Tests pass?yes
Fixed tickets#...
LicenseMIT
Doc PRsymfony/symfony-docs#...

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'sConfigDataCollector::getApplicationName() 🤔

In my case the service resolved to

$e =new \Symfony\Component\HttpKernel\DataCollector\ConfigDataCollector(\basename('/app'));

Causing the app name to be, well,app (again :P). It was really confusing.

Before:

image

(the "a" is from "app" here)

After:

image

or

image

I think we need to deprecate the argument for real?

cc@fabpot

Wirone reacted with thumbs down emoji
@ro0NLro0NL changed the titlea[WebProfilerBundle] Remove application name[WebProfilerBundle] Remove application nameOct 21, 2018

{%blockpanel %}
{%ifcollector.applicationname %}
{# this application is not the Symfony framework#}
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

i actually saw

image

:|

sstok reacted with laugh emoji
Copy link
Member

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.

Copy link
ContributorAuthor

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?

Copy link
Contributor

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.

@nicolas-grekasnicolas-grekas added this to the4.2 milestoneOct 22, 2018
Copy link
Member

@javiereguiluzjaviereguiluz left a 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!

Copy link
ContributorAuthor

@ro0NLro0NL left a 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

@nicolas-grekas
Copy link
Member

Thank you@ro0NL.

@nicolas-grekasnicolas-grekas merged commitf5c355e intosymfony:masterOct 23, 2018
nicolas-grekas added a commit that referenced this pull requestOct 23, 2018
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:![image](https://user-images.githubusercontent.com/1047696/47264761-fcb01980-d51c-11e8-9a3b-14a43d18b179.png)(the "a" is from "app" here)After:![image](https://user-images.githubusercontent.com/1047696/47264767-16516100-d51d-11e8-8a5d-4c20d4911861.png)or![image](https://user-images.githubusercontent.com/1047696/47264774-28cb9a80-d51d-11e8-8fe6-de0f46b06a1d.png)I think we need to deprecate the argument for real?cc@fabpotCommits-------f5c355e [WebProfilerBundle] Remove application name
@ro0NLro0NL deleted the appname branchOctober 23, 2018 12:48
This was referencedNov 3, 2018
@Wirone
Copy link
Contributor

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
Copy link
ContributorAuthor

ro0NL commentedNov 4, 2018
edited
Loading

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
Copy link
Contributor

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 fromdevelop branch in automatic way, some projects deploy feature branches too, other ones have additional job in CI that can be triggered manually so feature branch can be deployed to common DEV instance. It's important to see which version is deployed (DEV branches display branch's name as version, git commit's hash is appended there if specified).

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
Copy link
ContributorAuthor

there was a perfect and simple built-in feature for handling it?

Not perfect :)

should be part of the framework and configuration options should be added

Personally i disagree.

This is really simple to maintain.

Not sure, apparently it was totally forgotten in core :)

From users' (and developers' too) point of view version of the app is more important than version of the framework

Personally i disagree. At least both are equally important then IMHO.

I don't know what "bypass" you're talking about

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
Copy link
Contributor

Wirone commentedNov 5, 2018
edited
Loading

Personally i disagree. At least both are equally important then IMHO

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.

ro0NL reacted with thumbs up emoji

nicolas-grekas added a commit that referenced this pull requestMay 29, 2019
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
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@dunglasdunglasdunglas left review comments

@javiereguiluzjaviereguiluzjaviereguiluz approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

+1 more reviewer

@WironeWironeWirone left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.2

Development

Successfully merging this pull request may close these issues.

7 participants

@ro0NL@nicolas-grekas@Wirone@fabpot@dunglas@javiereguiluz@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp