slope-it / breadcrumb-bundle

A bundle for generating dynamic breadcrumbs in Symfony applications
Other
10 stars 4 forks source link

BreadcrumbBuilder service is not public #10

Closed Jupi007 closed 2 years ago

Jupi007 commented 2 years ago

I need to add this config to service.yaml when I want to use it:

SlopeIt\BreadcrumbBundle\Service\BreadcrumbBuilder:
        alias: slope_it.breadcrumb.builder
        public: true
asprega commented 2 years ago

Hi, thanks for your report. You should already be able to do what you intend to without declaring the service as public. Is it possible that your configuration has an issue? Are you able to inject any non-public service in your controllers?

https://symfony.com/blog/new-in-symfony-3-4-services-are-private-by-default

Jupi007 commented 2 years ago

Well in each bundle I use, all service which aim to be injected are public. It is the first time I need to configure something to load an external service.

asprega commented 2 years ago

The fact is, I am using this library in several projects and I am always able to inject BreadcrumbBuilder as-is. Is it possible you're missing this in your configuration?

   Your\App\Controllers\:
        resource: '%kernel.project_dir%/src/Your/App/Controllers/*'
        tags: ['controller.service_arguments']
Jupi007 commented 2 years ago

Oh yes, you're right. I miss this in my config. I'm sorry.

Thanks for your help :)

Jupi007 commented 2 years ago

I'm really sorry to annoy you again but the symfony doc tells this:

If your controllers don't extend the AbstractController class, you must explicitly mark your controller services as public. Alternatively, you can apply the controller.service_arguments tag to your controller services.

As all my controllers extends the AbstractController, this mean I shouldn't need to add this configuration to service.yaml.

So sorry to ask again, but don't you think that the BreadcrumbBuilder should be public?

Btw, I don't inject the service from the controller action, but directly in class constructor.

asprega commented 2 years ago

No problem! You're not annoying :)

Which Symfony version are you currently using? I just verified on a project of mine running Symfony 5.2 and, without that configuration, controller action injection does not work even if my controller extends Symfony's AbstractController. Anyway, regardless of controller action injection, the recommendation is to never make services public: https://symfony.com/doc/current/service_container/alias_private.html

So unless you specifically need to access a service directly from the container via $container->get(), the best-practice is to make your services private. In fact, All services are private by default.

Jupi007 commented 2 years ago

I'm using Sf 5.4

I checked and it's indeed because I'm not wiring the builder in the controller action that I have this error. Even if I add this configuration to service.yaml, the error still shown up when I wire the builder in the constructor. There's no problem when directly inject it in actions (with and without the additional config as I extend the AbstractController).

But I would prefer doing the injection in the constructor, so do you know another solution?

Jupi007 commented 2 years ago

Sorry to talk again of this. Maybe could you consider to add the code snippet of my first message to the doc, for people who want to autowire this service in the constructor?

I can make a PR if you want.

asprega commented 2 years ago

Hi. No problem!

I am currently injecting the BreadcrumbBuilder both in my controller actions and constructor and I am not having any problem. I'm pretty sure there's something in your project that prevents that. Are you able to test this in an "empty" symfony project and provide me with some code to reproduce the issue?

Jupi007 commented 2 years ago

Okay, I'll try to reproduce this in an empty project :+1:

Jupi007 commented 2 years ago

@asprega I reproduced the issue here: https://github.com/Jupi007/breadcrumb-bundle-issue-10

Capture d’écran 2022-03-31 à 15 49 02

asprega commented 2 years ago

oh ok! that's not about the service being private, but because the service is not aliased with its class name. I forgot to add that when it became possible. I will fix this as well and let you know.

Jupi007 commented 2 years ago

Yeah, sorry I expressed myself badly in the initial issue message 😕

asprega commented 2 years ago

Fixed in https://github.com/slope-it/breadcrumb-bundle/commit/e280dcdb28bf8a048edc1e95e9c1aec131ffc665

asprega commented 2 years ago

Just released 1.3.0 including this fix!