Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FrameworkBundle] Add service and alias deprecation message to debug:container <name> output #46814

Merged
merged 1 commit into from Jul 21, 2022

Conversation

94noni
Copy link
Contributor

@94noni 94noni commented Jun 30, 2022

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

Hi, the debug:container is great to understand container's services in the app

Having this deprecated warning can be valuable in such debug context, that is why I propose to add this on the output


For ex with such definition:

        ->set('url_helper', UrlHelper::class)
            ->args([
                service('request_stack'),
                service('router.request_context')->ignoreOnInvalid(),
            ])
            ->deprecate('symfony/framework-bundle', '6.2', 'The service "%service_id% is deprecated. Use "FooBarService" instead.')
        ->alias(UrlHelper::class, 'url_helper')
            ->deprecate('symfony/framework-bundle', '6.2', 'The alias "%alias_id% is deprecated. Use "FooBarAlias" instead.')

Capture d’écran 2022-07-05 à 10 39 03

Capture d’écran 2022-07-05 à 10 39 16

@94noni

This comment was marked as outdated.

@ogizanagi
Copy link
Member

What about showing the deprecation message as well?
Regarding the txt output, this could even be shown in a warning block.

@94noni
Copy link
Contributor Author

94noni commented Jul 3, 2022

@ogizanagi it could indeed be a nice idea!

But as there are lot of test files to update, I would prefer asking first a consensus before doing the proposed change if you dont mind :)

Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

That's a very useful information. I like the idea of colored warning in case of depreciation.

@94noni
Copy link
Contributor Author

94noni commented Jul 4, 2022

@GromNaN @ogizanagi oki, I will try adding the deprecated message, and a colored output for when applicable

@94noni 94noni force-pushed the debug-container-deprecated branch 4 times, most recently from f4ac1a4 to 32562a8 Compare July 4, 2022 08:18
@94noni
Copy link
Contributor Author

94noni commented Jul 4, 2022

With new code changes, I can get such results (with faking a service deprecation)

Capture d’écran 2022-07-04 à 10 19 35

@ogizanagi
Copy link
Member

ogizanagi commented Jul 4, 2022

@94noni : Sorry to make you change this again, I should have been clearer:
what about removing the info from the table output to show the deprecation message if the service is deprecated, using $io->warning() at the end of the command output?

@94noni
Copy link
Contributor Author

94noni commented Jul 4, 2022

@ogizanagi hey, do you mean like in my screenshot and written above the ! [Note] but as warning?

if yes I can rework as requested :)

@ogizanagi
Copy link
Member

@94noni : exactly what I meant :)

@94noni 94noni force-pushed the debug-container-deprecated branch 2 times, most recently from 8e2d3cd to 6a30629 Compare July 5, 2022 08:43
@94noni
Copy link
Contributor Author

94noni commented Jul 5, 2022

@ogizanagi @GromNaN PR reworked and description updated based on addressed comments :)

@chalasr
Copy link
Member

chalasr commented Jul 5, 2022

Looks good to me. Can you please go ahead with the missing test(s)?

@94noni
Copy link
Contributor Author

94noni commented Jul 5, 2022

@chalasr The only failed test I can see in the above checks is about 1) Symfony\Component\VarDumper\Tests\Server\ConnectionTest::testDump seem unrelated isn't it?

@chalasr
Copy link
Member

chalasr commented Jul 5, 2022

What I mean is that this feature should be covered by a test if possible

deprecated:
class: Symfony\Bundle\FrameworkBundle\Tests\Fixtures\DeprecatedClass
deprecated: The "%service_id%" service is deprecated since foo/bar 1.9 and will be removed in 2.0. Use "public" service instead.
deprecated_alias:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cf doc

@@ -11,6 +11,15 @@ services:
private_alias:
alias: public
public: false
deprecated:
Copy link
Contributor Author

@94noni 94noni Jul 6, 2022

Choose a reason for hiding this comment

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

cf doc + PR

@94noni 94noni force-pushed the debug-container-deprecated branch from cf6e55d to e9a5d5c Compare July 6, 2022 15:21
@94noni 94noni force-pushed the debug-container-deprecated branch 6 times, most recently from 4bb7984 to 0f69408 Compare July 8, 2022 08:00
@94noni
Copy link
Contributor Author

94noni commented Jul 8, 2022

status: needs review

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Jul 8, 2022
This PR was submitted for the 6.0 branch but it was merged into the 5.4 branch instead.

Discussion
----------

Fix doc service deprecation

Hi,
Based on [this PR](symfony/symfony#46814), I've followed [the doc](https://symfony.com/doc/current/service_container/alias_private.html#deprecating-services)

But when tests executed, I got [this error](symfony/symfony#46814 (comment))

So I propose to fix the doc accordingly here

Thank you

Commits
-------

ffdb7e6 Fix documentation on declaration of service deprecation in YAML format
@94noni
Copy link
Contributor Author

94noni commented Jul 18, 2022

With #38903 being recently merged there are lot of conflicts
May I ask from reviewers another round of reviews on the core code, before taking time to fix all tests conflicts?
thanks :)

@fabpot
Copy link
Member

fabpot commented Jul 20, 2022

@94noni Can you rebase this PR to get rid of the merge commit (that prevents me from merging this PR)?

@94noni
Copy link
Contributor Author

94noni commented Jul 20, 2022

@fabpot I'll handle this very soon and ping back

@94noni 94noni force-pushed the debug-container-deprecated branch 2 times, most recently from eb6b62e to 5a93272 Compare July 21, 2022 06:58
@94noni 94noni changed the title [FrameworkBundle] Add service deprecation on debug:container command output [FrameworkBundle] Add service and alias deprecation message to debug:container <name> output Jul 21, 2022
@94noni 94noni force-pushed the debug-container-deprecated branch from 5a93272 to 82054bc Compare July 21, 2022 07:34
@fabpot
Copy link
Member

fabpot commented Jul 21, 2022

Thank you @94noni.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants