Deprecate magic ServiceProvider file discovery

If I understand you correctly you are talking about something like this?

service_providers:
  - \Drupal\...
services:
  ...

I would love that! I've been trying someone to help debugging a problem where something worked on their machine, but not on the testbot. It turned out, they camelcased their module name in the wrong way, and as such had issues with case (in)sensitive file systems.

Here is just a start for people to play around with it.

Here is also a test to prove it works.

#2817627: Make service swapping easier is not a duplicate but it allows overriding services provided by a different module directly in the *.services.yml file.

I added a deprecation notice for now.

I tried to test the deprecation, with @expectedDeprecation, in \Drupal\KernelTests\Core\DrupalKernel\DrupalKernelTest::testServiceProviders() but it doesn't work. Have no idea why.

Fixed all core deprecations.

This is looking really great, I like these changes. I think that the current core deprecation testing is sufficient and that we don't need to add that for this class.

Added @todo, improved a little around the docs.

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -597,17 +599,41 @@ public function discoverServiceProviders() {
   +      // @todo Remove the implicit class name service provider in Drupal 9.0.x.
   

   I would argue that the deprecation is a much more effective @todo than this comment. Especially if we have a legacy test.

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -597,17 +599,41 @@ public function discoverServiceProviders() {
   +      if (!empty($this->serviceProviderClasses['app'][$module])) {
   +        @trigger_error("Implicit service providers are deprecated in Drupal 8.7.x and will be removed before Drupal 9.0.0. Instead declare your service provider class $class in .services.yml files. See https://www.drupal.org/node/2974194.", E_USER_DEPRECATED);
          }
   

   This should tell you which module has implicit service providers and what they are. This should be tested.

@alexpott, I tried to test, see my comment from #13 but the deprecation is not catched. Probably it's too early?

Added the test. Regarding the @todo, I think it doesn't hurt.

Finally, I found out that DrupalKernelTest::setUp() is not invoking parent::setUp() and this makes the PHPUnit Bridge not to be in effect. I haven't dig too much to find the exact point where this happens. Creating a new kernel test that is just using parent::setUp() is fixing the problem.

@claudiu.cristea nice sleuthing! Can you open an issue about DrupalKernelTest not having deprecation testing. I agree that fixing that here is out-of-scope.

Re the @todo - there's a standard that an @todo should be created with a corresponding issue. For me adding the deprecation is the @todo and we shouldn't be creating issues against Drupal 9 for every deprecation. Or maybe we should I don't know but to paraphrase @catch "a @todo without an issue is like a scream in space"

Removed the @todo.

Created #3001128: DrupalKernelTest doesn't test deprecation. I will add a "test only" patch there, as soon as this gets in, just to reuse the testing module introduced here.

The one thing I wonder is are we missing a trick and not doing more of this in \Drupal\Core\DependencyInjection\YamlFileLoader - otoh introducing a drupalism into services.yml files is also interesting.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -609,9 +611,30 @@ public function discoverServiceProviders() {
+        // Load service providers from the $module.services.yml file.
+        if (!$content = $file_cache->get($filename)) {
+          $content = Yaml::decode(file_get_contents($filename));
+        }

This is depending on an internal detail of YamlFileLoader outside of YamlFileLoader. That feels pretty odd.

@alexpott doing that in YamlFileLoader means we have to diverge more our ContainerBuilder in order to store there the service provider classes.

Alternatively, we ca avoid the drupalism by moving the service provider classes in separate file $module.service_providers.yml

The one thing I wonder is are we missing a trick and not doing more of this in \Drupal\Core\DependencyInjection\YamlFileLoader - otoh introducing a drupalism into services.yml files is also interesting.

At least we replace one kind of Drupalism with sort of one other.

I totally agree that declaration is much better than magic discovery based on names. It's great to deprecate the magic and replace with a declarative approach and yes doing this inline with things like permissions.

But that still leaves open the how and the where. I think the current approach is the first new thing we've added to Symfony's services.yml files and moving us away from that means that we should take time to carefully consider if and how we do that. I agree with @joachim that a big +1 for putting it in the services.yml file is that this is were we declare a modules services so provider and modifier classes also belong here. A separate yaml file does seem icky and the only other places would be a modules .info.yml or composer.json and I don't think this belongs there. So +1 for putting in the module.services.yml. Also there are a number of Symfony things that we don't support - for example https://symfony.com/doc/current/service_container/import.html#importing-... so this divergence has already occurred.

So now we're extending what goes in services.yml files I think we need to look at our implementation. As noted in #25 the current implementation reaches into the internals of a cache to do an optimised read. That feels wrong. Also the uncached read takes place because we do a read for adding the services so on a completely uncached container rebuild we'll be doing 2 reads of the same file.

Oh this is very interesting! I wonder if this might work for the install requirements: #3490843: Create class to replace hook_requirements('install')

Did a pass at calling out what needs to change for tests to run.

I think this looks great, not sure if there is anything to address 25 specifically.

To be clear, this is an MR I made of the branch @joachim of the patch @claudiu.cristea made in 2018. There's definitely a lot of cruft in it.

@nicxvan feel free to push to the MR fork!

I know, I wanted to just take on reviewing for now.

I'm gonna apply my comments and work on the other changes needed.

Fixed the easy stuff, this is going to need a lot more work, currently core/tests/Drupal/Tests/Core/DependencyInjection/YamlFileLoaderTest.php needs a lot of updates for the new error checking for valid keys.

As always, I was curious how this came to be.

Well, there's no explanation. The camelize was added in d1e52e0089 which doesn't have an issue ID associated with it neither does the commit message "Various cleanups" tells us anything. This is what happened:

-      $class = "\Drupal\\{$module}\\{$module}Bundle";
+      $camelized = ContainerBuilder::camelize($module);
+      $class = "\Drupal\\{$module}\\{$camelized}Bundle";

And then in #1988508: Stop pretending we support Symfony bundles when we really just have service providers it was merely kept. In that issue effulgenstia had an idea which might or might not be worth reconsidering: call the service provider simply Drupal\modulename\Services.

Went back a bit further via Google/related issues instead of git blame. I remember I hated the very existence of this, both the code and the way initiatives were ran which produced it.

#1716048: Do not boot bundle classes on every request

#1715940: Remove DIC compilation until it can be dumped to disk which has the choice phrase 'premature optimization' when talking about making changes to what was already a more than ten years old legacy code base.

This might be it. Don't think I was wrong in comment #1 #1599108: Allow modules to register services and subscriber services (events) glad we are finally getting there even if it involved reversing out of a lot of dead ends.

That was quite the read!

This is going to take some effort though I think.

Ok, there were a couple of modules with service providers missing the key in services.yml. I added those.

I also moved the service providers above the services key so it's clearer if a module uses one.

Let's see how this does with the tests.

I added a few ideas and wrote a new issue summary.

Talked about this issue briefly with @ghost of drupal past. I'm not entirely sure that diverging from the services.yml format is worth it to avoid magic naming, so wondering about #2 in the issue summary.

Call the class Drupal\module\Services or ServiceProvider. Pros: it's very simple, avoids weird naming for more complex module names. Cons: it's still magic naming -- however checking for the all lower case version as well is not too hard to avoid FS problem.

That makes finding a specific one hard to find, even the few test classes that have the same name it's hard to be sure you're in the right one or find it.

I generally navigate to a file using ctrl + E then type it, if there are more than a couple of files with the same name it becomes painful to pick them out.

moduleServiceProvider is magic naming, but at least it helps you find it and the file name tells you which module it's for if you're comparing two service providers.

Approach number 4 is encouraging!

Do we want to release the new feature here and do the core conversion / deprecation separately? Or do we want to do it all in one issue?

There are about 20 Service providers in core so it's not too bad to do it all at once.

The tagged services branch is ready for review. I do not think it needs an explicit test, converting LanguageServiceProvider shows it works. I added preliminary code for deprecation, it just needs to be uncommented.

1 small nit/comment on the MR.

On a related tangent, has there been thought about deprecating service providers altogether? A lot of what service providers do can be done with service decorators. (Granted that LanguageServiceProvider setting a container parameter is an exception.)

But allowing services to be instantiated in service providers is a blocker to autowiring core services, as seen in #3295751: Autowire core services that do not require explicit configuration #48.

I tried to shorten the code by moving things into a service pass but ordering makes it not viable. This is short enough.

I reimplemented the tagged services branch with an even shorter version -- only DrupalKernel needs change and not much change at that.

I hid the other MR since this one seems a lot more concise.

@catch mentioned in slack that we can likely do the conversion here too which let's us deprecate too.

I'm going to start on converting the remaining ones.

Ok I've converted everything and enabled the deprecation message.

Sorry we have not answered #65 adequately. I checked the core service providers:

1. multiple providers add registerFormat calls to the http_middleware.negotiation
2. InlineFormErrorsServiceProvider does not decorate but uses extension. Since it overrides a protected class, a decorator refactoring would be somewhat difficult.
3. entire service passes are added by some providers
4. Many modules (layout_builder, mysql, node...) module provides services which require other modules to be installed.

Overall, I do not think this can be dropped.

IMO if we're going to remove the magic naming, we should also support autoconfigure for these new tagged services like we have for EventSubscribers and loggers now.

It seems backwards to me to require less magic and more yaml when we've been progressing so much towards less yaml with autowiring, autoconfiguring, etc.

Why don't we

1. Add autoconfiguration based on the interface similar to https://www.drupal.org/node/3357408
2. Keep class names as is, saves bike shedding a new naming convention and reduces changes
3. Only throw the deprecation if the class has the magic name AND it wasn't already autoconfigured or manually defined in a services.yml

Should we introduce also “priority” for the tag, same as for destructible services? As it’s about service alterers I can see some value

I think renaming them is cleaner, how about EarlyServices?

Also, why not do the priority and autoconfigure work in a follow up, this is already a clean and easy to review issue with three things.

• A new feature
• A conversion
• A deprecation

Adding more stuff that can be done in a follow up feels like it is out of scope.

Also, why not do the priority and autoconfigure work in a follow up

I think autoconfigure should be done here so we're not going backwards in terms of now having to define it as a tagged service in yaml, then all of a sudden not having to. Priority imo can be a separate issue as that's entirely new.

I still don't think we should change the names. EarlyServices doesn't make much sense to me. ServiceProvider imo is the best. If we're going to force modules to have to change class names for the sake of it that's even worse.

1. The way this currently works, legacy first and then new service providers if their class is not legacy named. We could very easily move the new services first and only fire legacies if they have not fired as services. This indeed would allow not renaming them. However, I am for renaming to show things are changing
2. Re autoconfigure, sure, I upgraded https://www.drupal.org/node/3357408 to be more clear what is going on.

The way this currently works, legacy first and then new service providers if their class is not legacy named. We could very easily move the new services first and only fire legacies if they have not fired as services. This indeed would allow not renaming them. However, I am for renaming to show things are changing

I agree with this, but I also agree services is too generic.
I am going to name them ServiceAlter, or Service Register, for the ones that do both I'll split them, how is that?

78.2 is precisely why this should be a follow up.

This is the only magically named class in core, removing this isn't backwards even if we are adding a tag in services.yml

Ok putting aside the autoconfig stuff for now (did not expect that to be so controversial), can we look at allowing the class names to remain as is and only fire a deprecation if it's a magicly named service provider rather than a tagged one?

Otherwise we're telling contrib "your service provider can't be called ServiceProvider"

This is the only magically named class in core, removing this isn't backwards even if we are adding a tag in services.yml

Backwards was referring to requiring more yaml. All the other autodiscovery/autowire/autoconfig stuff we've got into core in the past year is moving to less yaml

We could very easily move the new services first and only fire legacies if they have not fired as services. This indeed would allow not renaming them. However, I am for renaming to show things are changing

Missed this bit. I think the deprecation is perfect for showing things are changing without being as disruptive. Moving to NW to do the swap.

I didn't see your comments, I just finished renaming them to be service alter or service register depending on which they are doing.

That way we keep the simplest deprecation and the classes now actually describe what they are doing.

Service provider isn't what they are either.

I hope this is acceptable, I think it will be really confusing to keep the name the same and tell people they have to put the yaml, with the rename, which is just renaming the file and the class they can tag it, otherwise I think it seems like what you're saying, adding the tag for no reason.

Otherwise we're telling contrib "your service provider can't be called ServiceProvider"

Yep, but that's a clean way to tell people to add the tag too. Also what does service provider mean semantically I think these new names are more descriptive anyway.

I upgraded https://www.drupal.org/node/3357408 to be quite a bit more clear about what is happening and crucially removed the link to the Symfony documentation because Drupal only autoconfigures registered services -- Symfony picks classes up from the filesystem and registers them as services so it was quite a bit misleading as to what autoconfigure means to Drupal.

Now that I stepped through what autoconfiguration does, it's quite clear it does not apply here since autoconfiguration is handled by RegisterAutoconfigureAttributesPass and this runs before any service passes. It needs to. (A few days ago I tried to convert it to a pass, it just doesn't work.)

I rebased on 11.x.

Re #83

That way we keep the simplest deprecation and the classes now actually describe what they are doing.
Service provider isn't what they are either.

But it's simpler if we don't force modules to change class names? The classes extend ServiceProviderInterface, a class name with ServiceProvider in it seems to describe it well.

Also, if we force a name change now it means come Drupal 12 modules will technically be able to use that name again. It just seems odd to force that IMO.

...otherwise I think it seems like what you're saying, adding the tag for no reason.

The reason should be basically the same as why this issue itself exists? Is it for performance, or just that magic naming is bad? Either way that's justification enough IMO. I think it's stranger to artificially force a name change for 1 major version of Drupal in the sake of making it seem like there's a reason to? If that makes sense 😅

Re #84
Thanks for the clarification! I would love to see services autoregistered as well, We want to do that with loggers (see #3395439: Automatically create a logger channel for autoconfigured modules) and it could definitely work for this service provider tagged service and event subscribers too.

I'm still not seeing a good reason to force the name change, I won't change the status again though as maybe we need another opinion?

I'm coming at this from a DX perspective, the change from magic naming is good, just not the forced name change IMO.

To clarify what #83 says, if there's no rename then people won't understand why they need to tag it. But, the rename is not forced. Core does the renames to be more clear, contrib can do whatever they want. I just pushed a version where the tagged services fire first and then if any magic named classes remain then they fire and trigger the deprecation. Previously it was the other way 'round which meant if someone actually injected something in the service definition it wouldn't have worked -- but never was a forced rename proposed. If contrib wants to keep name, sure they can -- in fact I expect most contrib in the beginning to keep 'em so they can maintain a single branch.

Oh I like that compromise.

but never was a forced rename proposed

Thanks, it certainly was made to sound that way!

Still think it'd be better to leave core as is in case someone is doing something with these classes but I'm happy as long as we're not forcing the change :)

> in case someone is doing something with these classes

https://www.drupal.org/about/core/policies/core-change-policies/bc-policy basically everything that is like a "herd" implementation of an interface is not BC, specifically listed are plugins, paramconverters, entity handler etc. Also, it is not at all clear why would anyone want to, changing services has its own interface. Perhaps to reach a utility function but the only provider with a utility function is languageserviceprovider and that's not used in contrib https://git.drupalcode.org/search?group_id=2&scope=blobs&search=language... and since the functions are isMultilingual / getDefaultLanguageValue it's hard to see why custom code would need 'em , surely you know whether your site is multilingual or not. Oh and protected methods are also exempt from BC so you shouldn't be using them anyways. So: the class is exempted, the methods are exempted and they are not used for what little there is.

And, once again, the code in DrupalKernel ever since this idea came up always allowed for renaming or not renaming. This is not my first rodeo :)

I've tested this locally and there's a few issues.

1. The trigger_error wasn't throwing a deprecation, this has uncovered some providers that haven't been converted (e.g InstallerServiceProvider) - I've applied a fix for this.
2. Deprecations are still thrown if a class is named with the old magic naming. I tested this by reverting FileServiceAlter to FileServiceProvider and running a Kernel test. The deprecation is still thrown even with an entry in file.services.yml
3. Other random things are throwing deprecations now:

Service providers are now tagged services with the service_provider tag. Magic naming for Test is deprecated in drupal:11.2.0 and removed in drupal:12.0.0.
Service providers are now tagged services with the service_provider tag. Magic naming for core is deprecated in drupal:11.2.0 and removed in drupal:12.0.0.
Service providers are now tagged services with the service_provider tag. Magic naming for Drupal\mysql\MysqlServiceRegister is deprecated in drupal:11.2.0 and removed in drupal:12.0.0.

Thanks! Yes, that's a recent oversight from #86. Please test again and if there are still unexpected fails, I will need reproduction instructions but I think we are good now.

Tests are failing on non magic named things

Magic naming for Drupal\workspaces\WorkspacesServiceAlter...

Also can still reproduce my previous report of magic named classes still throwing errors with the new service_provider tag.

Magic naming for Drupal\file\FileServiceProvider is deprecated in drupal:11.2.0 and removed in drupal:12.0.0. Service providers are now tagged services with the service_provider tag. See https://www.drupal.org/node/2974194

Steps to reproduce this are applying the following diff:

diff --git a/core/modules/file/file.services.yml b/core/modules/file/file.services.yml
index b30f942c387..95c0c0c8e72 100644
--- a/core/modules/file/file.services.yml
+++ b/core/modules/file/file.services.yml
@@ -37,6 +37,6 @@ services:
     arguments: ['@file_system']
   Drupal\file\Upload\InputStreamFileWriterInterface: '@file.input_stream_file_writer'
   file.service_provider:
-    class: Drupal\file\FileServiceAlter
+    class: Drupal\file\FileServiceProvider
     tags:
       - { name: service_provider }
diff --git a/core/modules/file/src/FileServiceAlter.php b/core/modules/file/src/FileServiceProvider.php
similarity index 91%
rename from core/modules/file/src/FileServiceAlter.php
rename to core/modules/file/src/FileServiceProvider.php
index 4dc34fe2a6e..b22f3c69283 100644
--- a/core/modules/file/src/FileServiceAlter.php
+++ b/core/modules/file/src/FileServiceProvider.php
@@ -9,7 +9,7 @@
 /**
  * Adds 'application/octet-stream' as a known (bin) format.
  */
-class FileServiceAlter implements ServiceModifierInterface {
+class FileServiceProvider implements ServiceModifierInterface {
 
   /**
    * {@inheritdoc}

OK and after naming it back, what are you running? It's going to be something tests do, I bet...

@ghost of drupal past just running a random test that installs file module. In this case DownloadTest::testPublicFileTransfer.

I think the test failures in CI are showing that there's more to it seeing as non-magically named services that are tagged are also throwing errors.

I can also put a breakpoint on the trigger_error line and hit it via a drush cr which throws these:

PHP Deprecated: Magic naming for Drupal\file\FileServiceProvider is deprecated in drupal:11.2.0 and removed in drupal:12.0.0. Service providers are now tagged services with the service_provider tag. See https://www.drupal.org/node/2974194 in /data/app/core/lib/Drupal/Core/DrupalKernel.php on line 1384
PHP Deprecated: Magic naming for Drupal\mysql\MysqlServiceRegister is deprecated in drupal:11.2.0 and removed in drupal:12.0.0. Service providers are now tagged services with the service_provider tag. See https://www.drupal.org/node/2974194 in /data/app/core/lib/Drupal/Core/DrupalKernel.php on line 1384

OK. The problem was tagged services marked with servicemodifierinterace got added to the service provider objects list before depreciation message hit so those were thought to be deprecated.

(MysqlServiceRegister should be changed to implement only one of the interfaces.)

jsonapi was left behind, I converted it.

Nice, that latest commit has fixed both issues.

I do still think we should keep core classes as they were to cover us from these weird bugs in the future. That way we won't regress for contrib.

We are now running in circles. I thought the question of renaming was resolved in #88.

@ghost of drupal past no it was not resolved. I don't see the point in renaming core classes if it's not needed, especially since it's potentially hiding bugs from what we can see from my manual testing.

Now things are getting even weirder with the jsonapi split. We have class JsonapiServiceAlter implements ServiceModifierInterface where the other *ServiceAlter classes are extending ServiceProviderInterface. And
class JsonapiServiceRegister implements ServiceProviderInterface

I think I see where the *Alter and *Register are coming from though. They are matching the methods they are implementing (alter from ServiceModifierInterface and register from ServiceProviderInterface). However, classes that extend ServiceProviderBase which implements both don't fit here, this base class is used a lot by core and contrib.

As mentioned it's to illustrate they can be renamed. We could see what each is doing and find a name that is unique and split them further.

However, I think that's follow up territory.

Yes alter and register are based on the method.

The alters should probably use ServiceModifierInterface
And register should probably use ServiceProviderInterface

OK back to the service_providers approach it seems, our approach has failed to gain acceptance.

I wouldn't be too hasty, there is one person against renaming, and two for it, we can get other opinions.

Also if other's also agree that we shouldn't rename, we can always name everything back to serviceProvider and just tag them.

Putting this back to needs review so we can gather other opinions.

Yes please don't take my disagreement on renaming as a disagreement on the change as a whole, I have said that overall this is great. I am just trying to get it into a state that I think would be easiest for a committer to review and commit, and in my mind the renaming could block that and turn into a bike shedding exercise of what things should be called.

With that being said, I will happily concede the naming if it means getting this in faster.

Baseline seems out of sync.

If you would not mind asking core dev their opinion on renaming, no need to bike shed more here. If a core committer agrees I'll rename everything back.

Baseline is fixed

False alarm

Baseline is still out of sync, but i think it's a different issue.

https://git.drupalcode.org/project/drupal/-/merge_requests/11198/diffs?c...

Those were not changed in this issue.
They were fixed here: https://www.drupal.org/project/drupal/issues/3483146

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.

Rebased.

Are service providers considered internal, and not API? Service providers aren't mentioned specifically in the BC policy. The closest thing is that compiler passes are considered internal, and it certainly seems like service providers should not be considered API. (It's also really hard for me to imagine how contrib/custom modules would be extending core service providers.)

But if that is not generally understood, then with the service provider classes being moved/split, there probably should be deprecation messages to that effect. Classes that are moved can use the new container parameters from https://www.drupal.org/node/3509577 to deprecate the old class name.

Good question!

Will doing the deprecation alleviate concerns about renaming? Adding the container parameters aliases the old class name to the new class name and provides BC, though the added typehints would be a breaking change for any implementing subclass methods.

Let me restate #89: while the letter of the law doesn't include service providers the spirit of it certainly does. The way I read https://www.drupal.org/about/core/policies/core-change-policies/bc-policy basically everything that is like a "herd" implementation of an interface is not BC, specifically listed are plugins, paramconverters, entity handler etc.

It only makes sense TBH. You should only be interacting with the interface not the specific classes.

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.

Rebased.

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.