deprecate magic ServiceProvider file discovery; declare in services.yml

Made a MR with patch #23.

Still needs work for various comments above.

Oh this is very interesting! I wonder if this might work for the install requirements: 📌 Create class to replace hook_install_requirements Active

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. I remember I hated the very existence of this.

#1716048: Do not boot bundle classes on every request →

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 📌 Use autowiring for core modules and services Needs work #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 filed 📌 Optimize Containerbuilder::findTaggedServiceIds() Active as related to the new super simple implementation.

nicxvan → changed the visibility of the branch 2910814-deprecate-magic-service-provider to hidden.

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 is about service alterers I can see some value

I think renaming them is cleaner, how about EarlyServices?

Also, why not do the priority and apricot 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. I looked at 📌 Enable service autoconfiguration in the DI container Needs review and I am terrified. How on earth did that go through? Y'all looked at Symfony documentation and just trusted it sight unseen? Performance? Any caveats? Did anyone read the code in there? How does it work? While it looks good on paper, it's still Symfony which has proven multiple times not to care that much for the scale Drupal does things. I for one refuse to use it until it's better understood. Just as an example, because for the Hook OOP issue, I did the relevant research which is completely missing here: the FileLoader::findClasses() scans the filesystem and then converts the path and filename into a classname PSR-4 style. But Drupal's YamlFileLoader does not extend FileLoader. So how does it work? I am going to unfollow this issue and any followups for mental health reasons until this resolved.

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?

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 you're 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.

I upgraded https://www.drupal.org/node/3357408 → to be quite a bit more clear about what is happening and what is not 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.

Nonetheless, autoconfiguration does not apply here since as that's a service pass and this runs before any service passes.

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 📌 Automatically create a logger channel for autoconfigured modules Active ) and it could definitely work for this ServiceProvider 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.

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.

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?

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

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.

No, it's not worth not converting, these bugs are BC only and temporary. Just because I didn't get it right at once doesn't mean we should use that as justification to make core worse.

@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 martinis it's to illustrate they can be renamed.

Yes alter and register are based on the method.

ghost of drupal past → changed the visibility of the branch 2910814-deprecate-magic-service-provider to active.

ghost of drupal past → changed the visibility of the branch 2910814-tagged-services to hidden.

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

acbramley → changed the visibility of the branch 2910814-tagged-services to active.

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.

acbramley → changed the visibility of the branch 2910814-deprecate-magic-service-provider to hidden.

acbramley → closed merge request !11154

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

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 📌 Add string return type to all hook_help implementations Active

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.