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

When using winzouStateMachineBundle 0.4, the old named services are aliases, so need to be marked public as well #178

Merged
merged 2 commits into from
Oct 14, 2020

Conversation

mbabker
Copy link
Contributor

@mbabker mbabker commented Sep 1, 2020

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Related tickets N/A
License MIT

In winzouStateMachineBundle 0.4, the sm.factory, sm.callback_factory, and sm.callback.cascade_transition IDs are aliases and not real services. So, the aliases need to be made public as well otherwise they won't be available through the container once it's compiled.

@mbabker mbabker requested a review from a team as a code owner September 1, 2020 00:41
@@ -39,9 +38,12 @@ public function process(ContainerBuilder $container): void
$container->setAlias('sm.callback.cascade_transition', CascadeTransitionCallback::class);
Copy link
Contributor

@stloyd stloyd Sep 2, 2020

Choose a reason for hiding this comment

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

IMHO this should be changed to just like(tested the current proposal & it didn't work for me):

Suggested change
$container->setAlias('sm.callback.cascade_transition', CascadeTransitionCallback::class);
$container->setAlias('sm.factory', FactoryInterface::class)->setPublic(true);
$container->setAlias('sm.callback_factory', CallbackFactoryInterface::class)->setPublic(true);
$container->setAlias('sm.callback.cascade_transition', CascadeTransitionCallback::class)->setPublic(true);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR seems fine as is for me, this is coming out of my local Sylius clone with my WIP upgrade PR applied:

Michaels-MacBook-Pro-2:Sylius mbabker$ bin/console debug:container sm.factory

 // This service is a public alias for the service SM\Factory\Factory

Compared to the current state of affairs:

Michaels-MacBook-Pro-2:Sylius mbabker$ bin/console debug:container sm.factory

 // This service is a private alias for the service SM\Factory\Factory                                                  

... snip ...

 ! [NOTE] The "sm.factory" service or alias has been removed or inlined when the container was compiled.

@lchrusciel
Copy link
Member

We have to fix the main build before we can go anywhere further.

@lchrusciel lchrusciel linked an issue Sep 9, 2020 that may be closed by this pull request
@pamil
Copy link
Contributor

pamil commented Oct 14, 2020

Rebased & added a functional test for these services accessibility.

@pamil pamil merged commit 5e94583 into Sylius:master Oct 14, 2020
@pamil
Copy link
Contributor

pamil commented Oct 14, 2020

Thank you, Michael! 🥇

@mbabker mbabker deleted the sm-bc-tweak branch October 14, 2020 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Symfony 5 support
4 participants