Make ErroneousMessagesSelector service public again #458
Conversation
| @@ -1,7 +1,7 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
| <container xmlns="http://symfony.com/schema/dic/services" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd"> | |||
| <services> | |||
| <service id="sonata.notification.erroneous_messages_selector" class="Sonata\NotificationBundle\Selector\ErroneousMessagesSelector"> | |||
| <service id="sonata.notification.erroneous_messages_selector" class="Sonata\NotificationBundle\Selector\ErroneousMessagesSelector" public="true"> | |||
There was a problem hiding this comment.
If this service is not always required, I think we could declare it as an alias of the private service Sonata\NotificationBundle\Selector\ErroneousMessagesSelector in order to improve the container performance.
There was a problem hiding this comment.
Take a look at this thread @phansys sonata-project/SonataAdminBundle#6210 (comment)
what do you mean by improving performance using a public alias?
There was a problem hiding this comment.
Take a look at this thread @phansys sonata-project/SonataAdminBundle#6210 (comment)
No problem. My suggestion had the intention to keep the service private (its id really doesn't matter for what I was trying to achieve).
what do you mean by improving performance using a public alias?
What I mean is, based on the docs, there seems to be an implicit optimization when a private service keeps private, using a public alias to access it from DIC if required.
I'm not sure if my supposal is right or not, these are the relevant paragraphs:
Private services are special because they allow the container to optimize whether and how they are instantiated. This increases the container’s performance. It also gives you better errors: if you try to reference a non-existent service, you will get a clear error when you refresh any page, even if the problematic code would not have run on that page.
However, if a service has been marked as private, you can still alias it (see below) to access this service (via the alias).
If I'm wrong about this, please forget about my comment.
| @@ -1,7 +1,7 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
| <container xmlns="http://symfony.com/schema/dic/services" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd"> | |||
| <services> | |||
| <service id="sonata.notification.erroneous_messages_selector" class="Sonata\NotificationBundle\Selector\ErroneousMessagesSelector"> | |||
| <service id="sonata.notification.erroneous_messages_selector" class="Sonata\NotificationBundle\Selector\ErroneousMessagesSelector" public="true"> | |||
There was a problem hiding this comment.
What about adding a TODO: remove public in next major if not used anymore by ... ?
There was a problem hiding this comment.
I would say: NEXT_MAJOR: Remove public and make use of dependency injection instead of container
I mean, it should be a requirement for NEXT_MAJOR release
There was a problem hiding this comment.
Or maybe it's the right time to do it ?
On other bundles, we made this change on the stable branch: sonata-project/SonataAdminBundle#5579
There was a problem hiding this comment.
Well, IMO it should be done, but fixing a bug is fixing a bug... If @core23 wants to invest time on it, good, if not it can be merged as is (I would suggest the next_major comment so another person can work on it)
There was a problem hiding this comment.
Yes sure.
Either changing the command, either adding a NEXT_MAJOR comment to this file.
That's right, we could improve our code later, but first we should fix the bug. The service was public by default in symfony 3, so we had no issue. But when bumping to symfony 4, we did not find this issue. Can you please approve this @VincentLanglet ? |
I am just asking for a NEXT MAJOR comment |
|
I created an issue as a reminder #464 |
|
Thanks ! |
Subject
The service is used inside a command: https://github.com/sonata-project/SonataNotificationBundle/blob/3.x/src/Command/RestartCommand.php#L118
I am targeting this branch, because this is a patch.
Changelog