search

KnpLabs / DoctrineBehaviors

Doctrine2 behavior traits that help handling Blameable, Loggable, Sluggable, SoftDeletable, Uuidable, Timestampable, Translatable, Tree behavior
http://knplabs.com
MIT License
914 stars 294 forks source link

Register services explicitly #660

Closed franmomu closed 3 years ago

franmomu commented 3 years ago

Since it wasn't too much trouble, I've done the first step of https://github.com/KnpLabs/DoctrineBehaviors/issues/659.

After this, it would be nice to :

TomasVotruba commented 3 years ago

Hi, thanks for PR. What broken behavior this fixes?

franmomu commented 3 years ago

hey, there is nothing broken AFAIK.

It's about to follow best practices, in my case I was looking for the service definition of LocaleProviderInterface::class to decorate it and I couldn't find it easily.

The reason is that bundles shouldn’t rely on features such as service autowiring or autoconfiguration to not impose an overhead when compiling application services.

Services are supposed to be private by default, in this case these services are not meant to be fetched from the container. Creating them private would increase slightly the container's performance since it allows the container to optimize its instantiation.

UPDATE: I've created https://github.com/KnpLabs/DoctrineBehaviors/pull/661 for the failing checks

TomasVotruba commented 3 years ago

The service should be available via constructor injection.

We'll need failing test case to confirm.

TomasVotruba commented 3 years ago

I've just checked the content and manual registration of service is deprecated approach. We're using autodiscovery to delegate this work to framework.

franmomu commented 3 years ago

I've just checked the content and manual registration of service is deprecated approach. We're using autodiscovery to delegate this work to framework.

What do you mean by manual registration of service is deprecated approach?

Symfony states that (for bundles):

Services should not use autowiring or autoconfiguration. Instead, all services should be defined explicitly.

TomasVotruba commented 3 years ago

The config you provided registers services manually one by one.

franmomu commented 3 years ago

The config you provided registers services manually one by one.

yep, that is what I understand (and what other bundles do) by "all services should be defined explicitly", to explicitly register services one by one with all the dependencies, tags, etc.

TomasVotruba commented 3 years ago

That's the complexity we want to avoid.

franmomu commented 3 years ago

ok then.