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

[DependencyInjection] Add support for excluding services with declared custom attribute #50447

Open
wants to merge 2 commits into
base: 7.1
Choose a base branch
from

Conversation

angelov
Copy link
Contributor

@angelov angelov commented May 26, 2023

Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets /
License MIT
Doc PR TODO

Currently, if we want to exclude classes from registering as services in the container, we can add their namespace(s) in the app configuration, or use the #[Exclude]/#[When('never')] attributes on each of the classes.

When it comes to certain types of classes, eg. entities, they can be placed all around the apps in many different folders, which can result in many entries under the exclude configuration. It is also usual for us to have many entity classes, so adding the attribute to each of them can be error-prone.

However, classes of a same type may already have a common attribute declared on them - such as Doctrine\ORM\Mapping\Entity.

Because of that, I'm proposing a new way for excluding classes by making it possible to register any attribute for exclusion.

Underneath, this is just extending the functionality for handling the classes with #[Exclude] attribute by additionally checking the list of attributes marked for exclusion.

Usage:

To exclude all classes with the #[Entity] attribute:

use Doctrine\ORM\Mapping\Entity;
use Symfony\Bundle\FrameworkBundle\Kernel\MicroKernelTrait;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\HttpKernel\Kernel as BaseKernel;

class Kernel extends BaseKernel
{
    use MicroKernelTrait;

    protected function build(ContainerBuilder $container)
    {
        $container->registerAttributeForExclusion(Entity::class);
    }
}

@angelov
Copy link
Contributor Author

angelov commented Jun 14, 2023

The failures in the checks seem unrelated.

namespace Symfony\Component\DependencyInjection\Tests\Fixtures\Utils;

#[\Attribute(\Attribute::TARGET_CLASS)]
class ToBeExcluded
Copy link
Member

Choose a reason for hiding this comment

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

It'd be great to allow this feature to work even if the attribute class doesn't not exist.
Can you remove this one and make the feature still work?

if ($r->getAttributes(Exclude::class)[0] ?? null) {
$this->addContainerExcludedTag($class, $source);
continue;
$exclusionAttributes = $this->container->getExclusionAttributes();
Copy link
Member

Choose a reason for hiding this comment

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

IIRC, file loaders are run on separate container instances, right? This would mean that a bundle cannot expect registerAttributeForExclusion to have any effect on classes in src/. Am I correct? If yes, don't we want to make these app-wide instead?

Copy link
Member

Choose a reason for hiding this comment

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

what about this? did you give it a try? configuring an exclusion within a bundle and seeing it work on the app?

@nicolas-grekas nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023
Copy link
Member

@nicolas-grekas 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.

Small rebase needed, but more importantly: did you try using registerForAutoconfiguration to define the container.exclude tag?

7.1
---

* Add support for excluding services with declared custom attribute (`ContainerBuilder::registerAttributeForExclusion`)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Add support for excluding services with declared custom attribute (`ContainerBuilder::registerAttributeForExclusion`)
* Add support for excluding services by attribute with `ContainerBuilder::registerAttributeForExclusion()`

@@ -131,6 +132,11 @@ class ContainerBuilder extends Container implements TaggedContainerInterface
*/
private array $autoconfiguredAttributes = [];

/**
* @var array<class-string>
Copy link
Member

Choose a reason for hiding this comment

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

?

Suggested change
* @var array<class-string>
* @var array<class-string, true>

if ($r->getAttributes(Exclude::class)[0] ?? null) {
$this->addContainerExcludedTag($class, $source);
continue;
$exclusionAttributes = $this->container->getExclusionAttributes();
Copy link
Member

Choose a reason for hiding this comment

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

what about this? did you give it a try? configuring an exclusion within a bundle and seeing it work on the app?

@xabbuh xabbuh modified the milestones: 7.1, 7.2 May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants