This was basically what was suggested by catch over in #1939660-10: Use YAML as the primary means for service registration and I have really come around to this idea.

Right now we have a class defined by Symfony, on which we call one method when building the container - we can call a method on any class we like to do the same thing.

IRC conversation with Crell today:
katbailey: Crell: bundles as they are now are just container futzers - maybe we get some processing of them for free by the kernel but I'm not sure that's worth the confusion
[1:37pm] Crell: Confusing with Entity API or with Symfony?
[1:37pm] katbailey: both!
[1:37pm] katbailey: symfony bundles are a lot more than container futzing, we don't support any of the rest of it
[1:37pm] Crell: Entity API's naming is dumb to begin with and we've been trying to change it for a year, so no biggie there.
[1:37pm] Crell: Confusing with Symfony I'm more sensitive to.
[1:37pm] Crell: What's your suggestion then?
[1:38pm] katbailey: they are basically just for modifying existing services and adding compiler passes
[1:38pm] katbailey: ContainerFutzer?
[1:38pm] katbailey: j/k
[1:38pm] Crell:
[1:38pm] Crell: $modulename/lib/Drupal/Services.php?
[1:38pm] Crell: Silex calls them Providers (or the closest equivalent).
[1:39pm] katbailey: sure! something along those lines
[1:39pm] Crell: Or rather, ServiceProvider.

Discuss! :-)

CommentFileSizeAuthor
#33 kernel-1988508-33.patch3.86 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,089 pass(es). View
#28 1988508-base-class-28.patch9.05 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 57,974 pass(es). View
#21 1988508.rename-bundles.21.patch59.35 KBkatbailey
PASSED: [[SimpleTest]]: [MySQL] 58,477 pass(es). View
#21 interdiff.txt12.98 KBkatbailey
#19 1988508.rename-bundles.19.patch51.53 KBkatbailey
PASSED: [[SimpleTest]]: [MySQL] 57,949 pass(es). View
#19 interdiff.txt12.22 KBkatbailey
#15 1988508.rename-bundles.15.patch49.21 KBkatbailey
PASSED: [[SimpleTest]]: [MySQL] 58,341 pass(es). View
#15 interdiff.txt22.17 KBkatbailey
#13 1988508.rename-bundles.13.patch29.69 KBkatbailey
PASSED: [[SimpleTest]]: [MySQL] 58,421 pass(es). View
#11 1988508.rename-bundles.12.patch25.64 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] 57,710 pass(es), 6 fail(s), and 8 exception(s). View
#9 1988508.rename-bundles.patch24.45 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

bojanz’s picture

I like the idea of going with ServiceProvider, since it's what Silex and Laravel4 guys do. It makes sense.

effulgentsia’s picture

+1 to the issue title.

$modulename/lib/Drupal/Services.php

I'm assuming what's meant there is $modulename/lib/Drupal/$modulename/Services.php.
From a purely Drupal standpoint, I like that because of the parallel with $modulename.services.yml.

Silex calls them Providers...Or rather, ServiceProvider.

If anyone from Silex or another Symfony using project can provide more detail or suggestions here, that would be great. Picking a name that's already understood in the Symfony ecosystem would be great.

they are basically just for modifying existing services and adding compiler passes

Perhaps a little tangential to this issue, but I think we're currently doing 2 things wrong with our compiler passes. 1) We have too many of them: they pretty much all are just variants of making all services sharing a particular tag become an argument to some other service. Would be great to centralize that into a single compiler pass and allow the $modulename.services.yml file to identify service tag mapping via container parameters or similar. 2) Is implementing that functionality even an appropriate job for a compiler pass? Shouldn't compiling be just a matter of optimization, rather than changing functionality? Would be great if someone from Symfony could shed light on if there's a more appropriate way of mapping service tags to service arguments. I mention this here in case "adding compiler passes" becomes a minor part of what these classes do, and "modifying existing services" becomes the main part, and if that influences the naming.

aspilicious’s picture

Ok, when we are changing this let us *please* make sure the service loading order is configurable. For example: 2 modules want to overload the same service. One will win. I want control over who is going to win :)

Bundles now allow to set a parent, that is the least we should have in our own implementation. Or we could avoid this by making use of the module weight.

effulgentsia’s picture

Or we could avoid this by making use of the module weight.

AFAIK, that's already the case in HEAD. We first load all the $modulename.services.yml files, ordered by module weight,name. Then we invoke build() on all bundle classes (or whatever this issue will rename them to) ordered by module weight,name. So the higher weight module has its build() run later, and therefore, gets the final say.

katbailey’s picture

The recommended way in Symfony to alter a service provided by another bundle is to use a compiler pass - see http://symfony.com/doc/current/cookbook/bundles/override.html under "Services & Configuration". This ensures that all service definitions will be in place before any code that attempts to modify them runs. I think this should be our recommended way of redefining services as well.

effulgentsia’s picture

Indeed that's what that docs page says, further reinforced by http://symfony.com/doc/current/components/dependency_injection/tags.html.... IMO, that's a serious design flaw though, since it makes compiling non-optional, and at least the intro portion of http://symfony.com/doc/current/components/dependency_injection/compilati... discusses compilation as an optimizer, not a behavior changer, though halfway down that document, it also mentions the tags use case.

But, given these docs, I'll concede that switching from compiler passes to some other technique for resolving service tags is out of scope for this issue. Once this issue lands though, and we're no longer bound to using BundleInterface, I'll probably open an issue for adding a alter() or process() method in addition to build() to ServiceProviderInterface (or whatever we call it), so that we can decouple alterations from compilation.

Eronarn’s picture

I've been working with getting Symfony bundles running in D8, and I strongly agree with this. It looks bleak in the short term, but we'll eventually have people running Symfony bundles on Drupal sites (whether with some glue to turn them into 'ServiceProviders', or something else). May as well make life easier for them.

katbailey’s picture

Assigned: Unassigned » katbailey

Hoping to get a patch up for this tonight...

katbailey’s picture

Status: Active » Needs review
FileSize
24.45 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Am I being ridiculously naïve in thinking this has a hope of getting in?
We only have 2 non-test bundles left in core. This patch changes them to ServiceProviders (though I'm not sure what advantage there is to using the same term that Silex uses given how different the interfaces actually are), and eliminates the need to use a compiler pass just to modify an existing service definition (a new compiler pass in core will call the alter() method on each registered service provider). This means that modules altering service definitions will now need to write only 1 class (in addition to the new service class) instead of 2.

If we go this far, then it makes no sense at all to extend the Kernel class. We should just create our own interface (repurpose DrupalKernelInterface) that extends HttpKernelInterface. I will make that change in the next reroll after seeing what I have broken.

Status: Needs review » Needs work

The last submitted patch, 1988508.rename-bundles.patch, failed testing.

katbailey’s picture

Status: Needs work » Needs review
FileSize
25.64 KB
FAILED: [[SimpleTest]]: [MySQL] 57,710 pass(es), 6 fail(s), and 8 exception(s). View

Believe it or not, I actually had tested this with the installer, it's just that I still had the CoreBundle.php file at the time :-P

Status: Needs review » Needs work

The last submitted patch, 1988508.rename-bundles.12.patch, failed testing.

katbailey’s picture

Status: Needs work » Needs review
FileSize
29.69 KB
PASSED: [[SimpleTest]]: [MySQL] 58,421 pass(es). View

Whatever about RouterTestBundle, I have no idea how I forgot about SerializationBundle seeing as Lin has just mentioned it to me yesterday... derp.

catch’s picture

Priority: Normal » Major

Bumping to major, I'd like to get this in before freeze.

katbailey’s picture

FileSize
22.17 KB
49.21 KB
PASSED: [[SimpleTest]]: [MySQL] 58,341 pass(es). View

And now with less Kernel.
(Also renamed the BundleTest test and bundle_test module.)

msonnabaum’s picture

Really really like this. Only change I'd like to see is the alter method moved to a dedicated interface so modifying other services is more opt-in for contrib.

aspilicious’s picture

Yeah I agree with msonnabaum.

+++ b/core/lib/Drupal/Core/DependencyInjection/ServiceProviderInterface.phpundefined
@@ -0,0 +1,26 @@
\ No newline at end of file

Needs newline

+++ b/core/modules/serialization/lib/Drupal/serialization/SerializationServiceProvider.phpundefined
@@ -2,26 +2,33 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function alter(ContainerBuilder $container) {
+

I agree with the above comment. 2 interfaces would also kill this empty function (and others). That or define a base implementation with empty function bodies.

beejeebus’s picture

yay for this patch!

one nitpick, i thought we would generally use instanceof instead of is_a()?

+    if (!is_a($kernel, 'Drupal\Core\DrupalKernel')) {

i agree with #16, i think we should have two interfaces.

katbailey’s picture

FileSize
12.22 KB
51.53 KB
PASSED: [[SimpleTest]]: [MySQL] 57,949 pass(es). View

Ok, I've split the alter() method into its own interface and made the other changes. There was already a test that tested overriding existing services but it was faulty (it "overrode" a service that was not being registered in the first place because file module was not being enabled) - I changed it to use the new method of overriding services.

chx’s picture

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -9,26 +9,62 @@
-class DrupalKernel extends Kernel implements DrupalKernelInterface {

That's quite a bold move but I think we are good.

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -122,13 +165,15 @@ class DrupalKernel extends Kernel implements DrupalKernelInterface {
+    $this->debug = (Boolean) $debug;

That's valid PHP syntax????? wtf. I always thought it's (bool). Also, why do we need to cast this at all?

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -167,26 +202,43 @@ public function boot() {
+    if (false === $this->booted) {

Uppercase false needs to be, yes.

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -167,26 +202,43 @@ public function boot() {
+  {

{ needs to be on the funciton line

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -211,18 +264,52 @@ public function registerBundles() {
+    if (false === $this->booted) {

Here as well.

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -211,18 +264,52 @@ public function registerBundles() {
+    if (false === $this->booted) {

and here.

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -294,6 +381,20 @@ protected function getClassName() {
+      'kernel.environment'     => $this->environment,

We do not format arrays like this. This whole function is not Drupal code style compliant.

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -497,6 +616,17 @@ protected function dumpDrupalContainer(ContainerBuilder $container, $baseClass)
+  {

more symfony-formatting.

katbailey’s picture

FileSize
12.98 KB
59.35 KB
PASSED: [[SimpleTest]]: [MySQL] 58,477 pass(es). View

So, it turns out we don't need the debug parameter at all - one more thing we can get rid of :)
Fixed the other coding standards issues...

chx’s picture

Status: Needs review » Reviewed & tested by the community

Been addressed, the concerns have. Ready, this patch is. Yes, hmmm.

msonnabaum’s picture

Yup, love it.

alexpott’s picture

Title: Stop pretending we support Symfony bundles when we really just have container futzers » Change notice: Stop pretending we support Symfony bundles when we really just have service providers
Priority: Major » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed ed24241 and pushed to 8.x. Thanks!

Changed title to say what we are doing and I think we need to update the container change notice https://drupal.org/node/1539454

chx’s picture

Title: Change notice: Stop pretending we support Symfony bundles when we really just have service providers » Stop pretending we support Symfony bundles when we really just have service providers
Priority: Critical » Major
Status: Active » Fixed

Done.

aspilicious’s picture

Title: Stop pretending we support Symfony bundles when we really just have service providers » change notice: Stop pretending we support Symfony bundles when we really just have service providers
Priority: Major » Critical
Status: Fixed » Needs work

Change notice is incorrect and will lead to a lot of confusion.

You always HAVE to implement the "ServiceProviderInterface". The ater is "opt-in" by implementing the ServiceModifierInterface . So you must leave the register empty if you solely want to alter.
See example in patch.

+/**
+ * Defines the LanguageTest service provider.
+ */
+class LanguageTestServiceProvider implements ServiceProviderInterface, ServiceModifierInterface {
+
+  /**
+   * {@inheritdoc}
+   */
+  public function register(ContainerBuilder $container) {
+
+  }
....

I still think this is bas DX and the fact that chx makes the same mistake in his change notice prooves my point.
Anyway I do think we need a seperate change notice for providers as there is nothing said about how to register using the provider interface and when to use it.

chx’s picture

Title: change notice: Stop pretending we support Symfony bundles when we really just have service providers » Stop pretending we support Symfony bundles when we really just have service providers
Priority: Critical » Major
Issue tags: -Needs change record

So if one interface is always necessary then why did we split the interfaces? There are two avenues I see forward:

  1. One interface, a base class with empty methods.
  2. Three interfaces, like Traversable-Iterator-IteratorAggregate: a base, empty interface that we can use to find the service futzers and the two existing interfaces extending it.

Both of these approaches would allow for service modifiers needing one interface and getServiceProviders largely unchanged.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
9.05 KB
PASSED: [[SimpleTest]]: [MySQL] 57,974 pass(es). View

I prefer a baseclass like I said before. Something like my patch should work? Lets see...

katbailey’s picture

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/ModifyServiceDefinitionsPass.phpundefined
@@ -30,9 +29,7 @@ public function process(ContainerBuilder $container) {
-      if ($provider instanceof ServiceModifierInterface) {
-        $provider->alter($container);
-      }
+      $provider->alter($container);

I'm fine with providing a base class to make DX easier - but our code should not be written to work with base classes, it should work with interfaces, so I object to this change.

aspilicious’s picture

It's possible there is a misunderstanding here :)
The interface check here was needed because

$providers = $kernel->getServiceProviders();

could return a serviceProviderInterface instance or it could be a serviceProviderInterface instance only.
With merging the interfaces back again the providers returned by getServiceProviders always have an alter method.

I could leave the check and change the interface to "ServiceProviderInterface" but I'm prety sure it's not needed.

BTW: Same thing happens when calling ->register. There we dont check for the interface either.

aspilicious’s picture

So I guess you're saying. Leave the 2 interfaces.
Let the baseclass implement both (and leave empty).
And extend the base class when we want to alter and use the provider interface when we just want to register?

jibran’s picture

#28: 1988508-base-class-28.patch queued for re-testing.

dawehner’s picture

FileSize
3.86 KB
PASSED: [[SimpleTest]]: [MySQL] 59,089 pass(es). View

This basically adds exactly what 31 is saying and removes a couple of "unused" interfaces throughout core.

katbailey’s picture

This change looks reasonable to me.

dawehner’s picture

Anything blocks this to be RTBC?

katbailey’s picture

Status: Needs review » Reviewed & tested by the community

Oh all right then :)

dawehner’s picture

Anything I can review on exchange?

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Berdir’s picture

dawehner’s picture

Should there be a new change notice for the new base class?

chris_hall_hu_cheng’s picture

Newbie question, maybe someone can answer?

I assume as seems to be the case that the title of this issue means "Stop pretending we support Symfony Bundles (because we don't)" rather than "Stop pretending we support Symfony Bundles (and actually support them)"

On that assumption it appears that as a Drupal Dev I have lost the ability to pick and choose bits of a (module/bundle) to override as I can in Symfony, where it seems I could override in a similar piecemeal way as I can override bits of a base theme in a sub theme.

Instead I have the ability to override the services that the module provides. Which is fine but I feel a lot of people are just going to route their _content paths to a simple controller object, giving me no option to override. Even if they define services as with the book module in core, then BookManager service is a class, BookController expects a BookManager so how to override. Suppose I have a custom Book Improver module can I replace the book_manager with my own BetterBookManager class? or will that blow up (I may be thinking back to when I was a Java programmer) even if it doe not blow up there is no interface constraining me or giving me hints in my IDE so I could just as easily replace with and object that didn't quite fulfil the basic needs of BookController.

Some of the core services are defined as interfaces with factory classes to provide the implementations, this is pretty over weight for the standard contrib module.

Maybe I can override routes and that is my ultimate get out of jail card but I will then be sometimes cutting into routes, sometimes replacing services, sometimes .....

It feels as if I am losing some of the options that I previously had in D7 to avoid hacking core and contrib, or at least not gaining a clear cut approach.

Berdir’s picture

This is only about altering existing services.

This has nothing to do with routing. The routing system has it's own alter event that you can use to alter routes provided by other modules.

Those are two different things and while controllers usually use certain services, they are separately defined and altered.

For your example, you can pretty much change everything or only something, whatever your use case is. You could alter the route and use a different class but keep the default create() method, so you get the default book manager, you could alter the service to provide a different implementation but that has to respect the interface. You could even change the controller, override create() and pass in a different service, although that doesn't make much sense in this example :)

In short, you certainly don't lose any options, quite the opposite.

chris_hall_hu_cheng’s picture

@Berdir thanks for the answer.

I think my only confusion remaining is as BookManager is a concrete class, in I can't cut in and replace it? whereas if it was an Interface I could. So shouldn't best practice be that all services have an interface? But if an interface a whole lot more coding ensues, I presume I would need a factory then but perhaps not if BookController was expecting an interface...

book.manager:
    class: Drupal\book\BookManager
    arguments: ['@database', '@entity.manager']

Apologies, I only hijacked this issue for my questions as am working out how to make my own module flexible and it mentions bundles (I started from Symfony bundles and realised I couldn't take that approach).

Berdir’s picture

Yes, all/most services *should* have interfaces. There might even be an issue to add an interface for the book manager.

That said, you can still replace that service, you are however limited to making it a subclass instead of a completely separate implementation that just implements the same interface.

chris_hall_hu_cheng’s picture

Cheers @Berdir

You have taken my fuzzy questions and helped refine down to what I need to know and do. The slight problem is that many people are regarding the book module as nearest thing to best practice at the moment..

Thanks again, my mind is now full of clarity and purpose replacing a small cloud of noxious smelling mist :)

Berdir’s picture

No problem. I can recommend http://drupal.stackexchange.com/ for Drupal 8 questions. Multiple active core contributers are watching the "8" tag there and try to answer all questions that come up.

The main advantage of Drupal Answers is that questions and answers can easily be found by others and updated/improved over time. Discussions in old issues will get lost.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.