The goal of this issue is to enable automatic service creation, ultimately to enable autoconfiguration so we may simply apply an attribute to a class, and have it detected by other locator/manager/plugin systems.
This is an alternative/supplemental proposal for a different-but-compatible way of doing autodiscovery that's more on-brand with Drupal's way of doing modules. We also don't need to rely on YamlFileLoader updates. #3111008: Use native Symfony YamlFileLoader, and can be an alternative/supplemental implementation to resource service property in #3021899: Support resource key in services.yml.
Motivation
The need for this or #3021899: Support resource key in services.yml is becoming more evident, especially as we are coming to desire that classes or methods with PHP attributes are automatically converted to tagged services. Which would also be useful for the recent batch of service locator issues.
This issue will improve the container build performance, since for example contrib projects don't need to duplicate the process of traversing module directories looking for classes. E.g Hux / SM. These only get less performant as more modules need to do directory traversal and service-container tasks on their own.
A follow up issue to #3421573: Convert permission providers to tagged services; unify with a service locator will benefit such that we can just define a file with a class/method attribute.
Proposed resolution
- Allow a module to opt in any set of directories that it owns.
- A module may choose to opt in any directory, including `src/` itself.
- E.g.
src/, orsrc/EventSubscriber/, such that new modules can get all the benefits of services without an explicit definition. And things likeregisterAttributeForAutoconfigurationjust work.
-
Allow a module to define directories within *all* enabled modules that will be used for discovery.
- Modules may not choose to opt in
src/for all modules, only subdirectoriesand glob matches. - A module declaring directories here would be vigilant of declaring directory paths that conflict by directories already used. Especially when they contain classes that must not be converted to services.
- Modules may not choose to opt in
- Similarly to
resource, service definitions are a 'template' for newly created services. Properties likeautoconfigure,autowire,tags, etc, are copied over.
Example syntax:
services:
mymodule.foobar: # ID is not used for the created services.
module:
paths: # relative to module-in-context root dir.
- '/src/'
modules:
paths: # relative to all enabled modules' root dir.
- '/src/CacheTagInvalidator/'
- '/src/' # Invalid, exception!
- '../escaping_module' # Invalid, exception!
autowire: true
autoconfigure: true
tags:
- { name: tag_copied_to_all_services }
Scope and limitations
- A service per class is created, whose ID is the fully-qualified class name, allowing for autowiring. Customizing service ID is out of scope, and may be addressed in a future issue for implementing
\Symfony\Component\DependencyInjection\Attribute\AsAliasattribute.- Custom IDs are not necessary when we just want to be able to do attribute discovery for service locators.
- Exclude rules are not considered for an MVP. Exclude behavior needs deep discussion in the Drupal context, since the module which defines excludes may not know all ways excludes need to happen. So we may need extra configuration/hook-ability in some way, especially on the site-level.
Versus Symfony resource property
I don't think we can use use Symfony resource as its default implementation, since we're not allowing the outer namespace key to be modified. Paths should also be relative to the module directory root, rather than the application or using relative directory notation. The proposal here would be able to co-exist with Symfony resource if we do that in the future, at #3021899: Support resource key in services.yml
Theoretical future use of this feature (out of scope)
For example, the following in field.module could turn field formatters into services.
services:
modules:
paths:
- 'src/Plugin/Field/FieldFormatter/'
The formatters themselves would have a FieldFormatter attribute:
namespace Drupal\content_moderation\Plugin\Field\FieldFormatter;
#[FieldFormatter(
id: 'content_moderation_state',
label: 'Content moderation state',
field_types: ['string'],
)]
class ContentModerationStateFormatter extends FormatterBase { }
Field.module would register a container compiler pass:
$container->registerAttributeForAutoconfiguration(FieldFormatter::class, static function (ChildDefinition $definition, FieldFormatter $attribute, \ReflectionClass $reflector) {
$definition->addTag('plugin.field_formatter', $tagAttributes);
}
In the same compiler pass, a service locator would unite all field formatter services with that tag. Then pass the locator to the existing plugin manager as a new service argument:
$references = [];
foreach ($container->findTaggedServiceIds('plugin.field_formatter') as $serviceId => $tags) {
$references[$serviceId] = new Reference($serviceId);
}
$container->getDefinition('plugin.manager.field.formatter')
->setArgument('$plugins', ServiceLocatorTagPass::register($container, $references))
Then some extra glue such as using service factories to create instances equivalent to existing plugin managers.
Steps to reproduce
- Modules define configuration in their
services.ymlfile - Services are automatically created
Remaining tasks
- Implement
- Review
- Merge
User interface changes
Nil
API changes
New syntax in service YAML files.
Data model changes
Nil
Release notes snippet
@todo
Change record
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | 3422359-nr-bot.txt | 90 bytes | needs-review-queue-bot |
Issue fork drupal-3422359
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
dpiComment #3
dpiComment #5
dpiMR !6669 implementation notes:
symfony/configis brought in as a dependency so we have the same behaviour for globbing as Symfony. We're planning to bring in the dep for other issues like #3021899: Support resource key in services.yml so should be fine here.resourceparts of Symfony's version of YamlFileLoader, at the tail of\Symfony\Component\DependencyInjection\Loader\YamlFileLoader::parseDefinitionand the method it calls:\Symfony\Component\DependencyInjection\Loader\FileLoader::registerClasses.resource:pathsis relative to module directories.Comment #6
longwaveThis sounds like a really nice feature. If we're going to bring in
symfony/configwe should just do #3111008: Use native Symfony YamlFileLoader and then extend Symfony's YamlFileLoader to add our own features, if we can - but there was quite a lot of resistance to adding that dependency over there, for one because it starts to imply that we use other features of Symfony's configuration system, but Drupal does things entirely differently. Is there any way we can do this or #3021899: Support resource key in services.yml without adding the dependency?Comment #7
joachim commented> A module may choose to opt in any directory, including `src/` itself.
Can we make this automatic, rather than opt-in?
The opt-in code is just going to be repeated boilerplate.
I'd suggest we make it opinionated about where service classes are located, and allow modules to override if they want to. E.g.
- services in src/Services
- event subscribers in src/EventSubscriber
Comment #8
wim leersAgreed with #6, and curious about the responses to #6 😊
Comment #9
longwaveThere is also an interesting choice to make here regarding public vs private services. Drupal services are still public by default; Symfony switched to private by default some time ago. And when you use
resourcein Symfony:If we're going to use this for things like event subscribers, I see no reason for event subscriber services to be public; they are only useful in the event dispatcher and can just be privately injected into only that service.
Comment #10
dpi#6 + #8, @longwave @wimleers
We've since discussed in Slack, the
symfony/configdependency is undesirable and it conflicts with Drupal config.And we've discussed that globs are not necessary, so that negates the only use of
symfony/configcode.I've updated the issue summary with these decisions, and I'll update the associated MR removing use of
symfony/config, tests and supports for glob, and re-implement our own directory traversal utility.If we do decide to do globs in a future issue, I don't think what we're doing will conflict.
Comment #11
dpiI'd really like to do this, but I think we're going to get into trouble. One thing that comes to mind is plugins: Drupal has plugins which include references to non existent classes as a part of of constructors, and that plainly breaks autoloader. Migrate plugins immediately come to mind. In fact during the development of class-attribute autodiscovery for Hux, if I didnt limit to a precise subdirectory (instead of src/*), then service compilation/autoloader would just blow up on missing classes.
I think the proposal here isnt incompatible with straight up
src/*as you're suggesting, since both this proposal andresourceact more as templates. Whereas if we dosrc/*on a framework level, we'd be setting global defaults for all services, such aspublic: false, autowire: true, autoconfigure: true. This proposal is still useful as things like default tags and public: true would be used.As @longwave linked the quote from https://symfony.com/doc/current/service_container.html#importing-many-se... we can certainly do it if the automatic services are private by default.
My suggestion is that we can do this as a separate issue. Since this proposal is complimentary, and less likely to cause fundamental PHP and other conflicts.
I dont see why we need to add these constraints. I'd rather put a class anywhere and it just works. Directory/namespaces then become convention rather than constraints, just like Symfony.
Comment #12
dpiWorking on it.
Comment #13
dpiI've removed the dependency on
symfony/config, and the associated glob support it provides that we agreed on.The now-removed
\Symfony\Component\Config\Resource\GlobResourcealso used Finder, so I've copied relevant non-glob code and moved some path normalization/relative-to-absolute pieces up to ourYamlFileLoader.Green and back to needs review.
Comment #14
dpiCreated a change record @ https://www.drupal.org/node/3423656
Comment #15
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #16
joachim commented> I dont see why we need to add these constraints. I'd rather put a class anywhere and it just works. Directory/namespaces then become convention rather than constraints, just like Symfony.
I wasn't suggesting constraints, rather, defaults:
- if you don't specify in a mymodule.services.yml file, services are looked for in src/Services, and event subscribers in src/EventSubscriber
- if you do specify in a mymodule.services.yml file, you can override that.
Bur more generally, I don't see what is so wrong about constraints. They enforce consistency which is good for DX. And we already have constraints -- entity types have to be in src/Entity, plugins in src/Plugin/SUBFOLDER, and so on. Meanwhile, form classes are ALL OVER THE PLACE, and it is MUCH nicer working with plugins where you find the same structure everywhere you go, than working with forms where you never know where you'll find form classes (HELLO node module, with some in src/ and some in src/Form).
Furthermore, scanning for everything is just inefficient - it degrades performance and Drupal's efforts to be more sustainable.
Comment #17
andypostI'm sure it should not be so strict as #3376163: Support #[AsEventListener] attribute on classes and methods
Also event subscription sometimes a part of service
Moreover the restriction only applies to plugin discovery but it could have common reusable code
PS: curios how discovery could be optimized with composer's classmap
Comment #18
mxr576PS: curios how discovery could be optimized with composer's classmap+1, but until Drupal has a module installer feature, it cannot be an option, can be?
Comment #19
kingdutchI'm always in favour of making working with services easier, especially if it brings us closer to the way Symfony works and reduce WTFs that Symfony (or Laravel) developers may have coming to Drupal.
I do want to voice my opposition for the proposal where other modules can determine what namespaces are made available as services.
As someone working in projects where we have 300 modules enabled on average this sounds like a potential nightmare where any module can suddenly start exposing classes caused by any of the other modules, outside of my control. This creates scenario's where my module's behaviour can no longer be evaluated only looking at my own module but must be specifically evaluated within the context of every other possible Drupal module.
The example is to make classes of a specific type available (e.g.
CacheTagInvalidator) but whether I expose all the services in my module as cache tag invalidator really should be up to me as module author, not to another module. I may have a good reason to only expose a specific service in some scenarios.Symfony provides us with Service Tags which can provide exactly the desired behaviour. The module that provides the
CacheTagInvalidatorInterfacecan configure the service container to automatically tag services with that interface as cache tag invalidator. Any service that would want all cache tag invalidators could use a Tagged Iterator to collect all those services.Similarly within my module I may have many more services/components than I want in
src/Services. For example if I have sub-functionalities in my module that I want to break up I might havesrc/EventManagement/{Controller/*.php,ScheduleAvailabilityManager.php}andsrc/EventRegistration/{Controller/*.php,PaymendHandler.php}. We should be careful not to make assumptions about how module authors may want to lay out their modules, especially as we evolve the capabilities of the service container.Comment #20
andypostGreat point about performance of discovery, probably it needs something generalized like file cache as except services there's plugins and help topics and twog components
Comment #22
catchWe already have directories with special meaning like plugin attribute discovery, so if we allow services to be defined from anywhere, that will include the plugin directories, which means always scanning somewhere you'd never expect services to live just in case someone decides to put one in there. I don't think it's that restrictive to have a
Servicesdirectory and then a service anywhere else can still be defined via one or two lines in services.yml?Comment #23
dpiTo be honest plugins-as-services is something I've desired.
But for the various designated Drupally namespaces, like Plugin, we can utilise the exclude feature and prepopulate it with sensible defaults.