NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[FrameworkBundle]`message_bus` alias public#28216

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

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:4.1fromsroze:message-bus-alias-public

Aug 19, 2018

Merged

[FrameworkBundle]`message_bus` alias public#28216

fabpot merged 1 commit intosymfony:4.1fromsroze:message-bus-alias-public

Aug 19, 2018

Conversation

Copy link

Contributor

sroze commentedAug 17, 2018

Q	A
Branch?	4.1
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#28215
License	MIT
Doc PR	ø

Because it is used in theControllerTrait withget('message_bus')... same than forsecurity.csrf.token_manager and cie, it should be public.

Make themessage_bus alias public

51b6e9e

carsonbot added Status: Needs Review FrameworkBundle Bug labels

Aug 17, 2018

sroze added the Messenger label

Aug 17, 2018

Copy link

Member

chalasr commentedAug 17, 2018•
edited
Loading

ControllerTrait deals with a service locator, services provided by a service locator do not need to be public (and should not).
I see no good reason for making the command bus public, container aware stuff make no sense anymore (really since 4.1).

Copy link

Member

chalasr commentedAug 17, 2018

hmm, ControllerTrait is used by the FrameworkBundle'sController class...

nicolas-grekas added this to the4.1 milestone

Aug 17, 2018

Copy link

Member

nicolas-grekas commentedAug 17, 2018

It's time to deprecateController :)

Copy link

ContributorAuthor

sroze commentedAug 17, 2018

In the meantime... this is still a bug to fix 🙃

xabbuh approved these changes

Aug 17, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Aug 17, 2018

xabbuh reviewed

Aug 17, 2018

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php


		if ($busId ===$config['default_bus']) {
		$container->setAlias('message_bus',$busId);
		$container->setAlias('message_bus',$busId)->setPublic(true);

Copy link

Member

xabbuhAug 17, 2018

Choose a reason for hiding this comment

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

or$container->setAlias('message_bus', new Alias($busId, true));

Copy link

Member

nicolas-grekasAug 17, 2018

Choose a reason for hiding this comment

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

Yep. using setPublic is more explicit to me :)

nicolas-grekas approved these changes

Aug 17, 2018

View reviewed changes

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Sadly :)

Copy link

Contributor

MatTheCat commentedAug 17, 2018

What would be the consequence of removingController ? Would impacted controllers have to useControllerTrait ?

Copy link

Member

nicolas-grekas commentedAug 17, 2018

ControllerTrait is internal. The consequence would be to ask people to use AbstractController instead.

chalasr approved these changes

Aug 18, 2018

View reviewed changes

fabpot approved these changes

Aug 19, 2018

View reviewed changes

Copy link

Member

fabpot commentedAug 19, 2018

Thank you@sroze.

fabpot merged commit51b6e9e intosymfony:4.1

Aug 19, 2018

fabpot added a commit that referenced this pull request

Aug 19, 2018

bug#28216[FrameworkBundle]message_busalias public (sroze)

a09809b

This PR was merged into the 4.1 branch.Discussion----------[FrameworkBundle] `message_bus` alias public| Q             | A| ------------- | ---| Branch?       | 4.1| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#28215| License       | MIT| Doc PR        | øBecause it is used in the `ControllerTrait` with `get('message_bus')`... same than for `security.csrf.token_manager` and cie, it should be public.Commits-------51b6e9e Make the `message_bus` alias public

Copy link

Contributor

MatTheCat commentedAug 21, 2018

Oh okay I better useAbstractController right now then?

Copy link

ContributorAuthor

sroze commentedAug 21, 2018 via email

Yep. I'll send a PR to deprecate it :)

…

On Tue, Aug 21, 2018 at 11:14 AM Mathieu ***@***.***> wrote: Oh okay I better use AbstractController right now then? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#28216 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAxHEXgVazULdjG3QsEcAyV1ntUeGhEiks5uS8-NgaJpZM4WBdQE> .