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/, or src/EventSubscriber/, such that new modules can get all the benefits of services without an explicit definition. And things like registerAttributeForAutoconfiguration just 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 subdirectories and 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.
  • Similarly to resource, service definitions are a 'template' for newly created services. Properties like autoconfigure, 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\AsAlias attribute.
    • 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.yml file
  • 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

https://www.drupal.org/node/3423656

CommentFileSizeAuthor
#15 3422359-nr-bot.txt90 bytesneeds-review-queue-bot

Issue fork drupal-3422359

Command icon 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

dpi created an issue. See original summary.

dpi’s picture

Issue summary: View changes
dpi’s picture

Assigned: dpi » Unassigned
Status: Active » Needs review

dpi’s picture

MR !6669 implementation notes:

  • symfony/config is 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.
  • The changes here are very much inspired by the resource parts of Symfony's version of YamlFileLoader, at the tail of \Symfony\Component\DependencyInjection\Loader\YamlFileLoader::parseDefinition and the method it calls: \Symfony\Component\DependencyInjection\Loader\FileLoader::registerClasses.
  • Though 'module' / 'modules' are used in this MR, there is nothing specific to modules in particular, compared to other extension types.
  • Terminology such as 'ModuleDiscovery' is used, though I'm happy to hear better names.
  • Compared to resource:
    • paths is relative to module directories.
    • No namespace override is permitted.
    • Multiple paths allowed.
longwave’s picture

This sounds like a really nice feature. If we're going to bring in symfony/config we 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?

joachim’s picture

> 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

wim leers’s picture

Agreed with #6, and curious about the responses to #6 😊

longwave’s picture

There 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 resource in Symfony:

Wait, does this mean that every class in src/ is registered as a service? Even model classes? Actually, no. As long as you keep your imported services as private, all classes in src/ that are not explicitly used as services are automatically removed from the final container. In reality, the import means that all classes are "available to be used as services" without needing to be manually configured.

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.

dpi’s picture

Assigned: Unassigned » dpi
Issue summary: View changes

#6 + #8, @longwave @wimleers

We've since discussed in Slack, the symfony/config dependency 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/config code.

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.

dpi’s picture

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 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 and resource act more as templates. Whereas if we do src/* on a framework level, we'd be setting global defaults for all services, such as public: 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'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

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.

dpi’s picture

Status: Needs review » Needs work

Working on it.

dpi’s picture

Assigned: dpi » Unassigned
Status: Needs work » Needs review

I'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\GlobResource also used Finder, so I've copied relevant non-glob code and moved some path normalization/relative-to-absolute pieces up to our YamlFileLoader.

Green and back to needs review.

dpi’s picture

Issue summary: View changes

Created a change record @ https://www.drupal.org/node/3423656

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new90 bytes

The 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.

joachim’s picture

> 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.

andypost’s picture

event subscribers in src/EventSubscriber

I'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

mxr576’s picture

PS: 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?

kingdutch’s picture

I'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.

Allow a module to define directories within *all* enabled modules that will be used for discovery.

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 CacheTagInvalidatorInterface can 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 have src/EventManagement/{Controller/*.php,ScheduleAvailabilityManager.php} and src/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.

andypost’s picture

Great 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

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

catch’s picture

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.

We 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 Services directory and then a service anywhere else can still be defined via one or two lines in services.yml?

dpi’s picture

We 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

To 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.