See discussion at http://drupal.org/node/1911178#comment-7140322 and onwards.

sun's comments on there reminded me how much I dislike the fact we're using Symfony bundles in core.

There's several methods on the bundle class that we completely exclude from calling after #1716048: Do not boot bundle classes on every request - those methods don't really make any sense over events or hooks, and since we don't require modules to have a bundle class, we were doing nasty file system checks to see if the bundle exists each time before calling those methods.

This means that bundles are only used for service registration, and no-one has yet claimed to enjoy that over just implementing a hook.

Symfony itself allows for defining services via YAML, which is what we're also using for routes and now .info files, so if we moved to that, could we not drop bundles altogether?

See http://symfony.com/doc/2.1/cookbook/service_container/event_listener.html for an example, there's a PHP/YAML/XML tab showing the differences for an example event listener.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

I don't think we can entirely eliminate them. I haven't dug into the YAML/XML/JSON options for registering services (Symfony supports all 3 and defaults to XML for services, although it seems we're gravitating toward YAML, sadly), but I suspect there's at least some capability that is only possible with PHP. (Anything with a loop, for instance, which we do have.) Also, sun has argued that we should be using Bundle::boot() and Bundle::shutdown(). I'm undecided at the moment.

That said, I'm not against exposing YAML as an alternative (primary, but not only) mechanism for service registration. If we do that, though, we should probably also enable support for XML service registration since that's what nearly all Symfony bundles I've seen use. (fabpot has been trying to port some Symfony bundles to Drupal, which is a totally awesome idea we should try to make as simple as possible for people to do.)

msonnabaum’s picture

I'm not sure about YAML, but I very much agree with the premise here. Bundles seem to be analogous to modules in the symfony world, so bringing them in for service registration has always felt very awkward to me.

I'm not terribly interested in exploring which symfony technique fits us best. I'd rather just figure out what makes the most sense for us and go from there.

catch’s picture

Title: Use YAML for service registration, remove Symfony bundles from core » Remove Symfony bundles from core, use something else for service registration

I'm not that fussed about YAML vs. something else (although not keen on XML given we're avoiding that everywhere else with some success). Re-titling.

Crell’s picture

To be clear, I'm not arguing for making XML our default format for services. I'm saying if we're going to support config-file-based service definition, we should support multiple format to make porting code back and forth easier. And that I don't think we can drop support for code-based registration, even if it's not our primary tool.

tim.plunkett’s picture

Well, with routes we primarily use YAML, and so far it's really nice to use. But as the change notice points out, if you need a foreach loop, you have to use PHP with a RouteCollection (or whatever it's called).

I've registered my fair share of services, and only once did I need a loop (ViewsPluginManager).

A module.services.yml for the 80% would be awesome.

aspilicious’s picture

I had a small symfony training a few weeks ago and yes they do everything in yml EXCEPT for service registration. There they normally use xml because it's way easier to define services in that format.

Anyway I'm curious about the result of this issue. :)

Crell’s picture

aspilicious: Can you elaborate on why it's way easier to do in XML?

aspilicious’s picture

They mentioned better validation of the scheme but I don't think we care about that.
They also said "thats what most symfony devs do". Which is a prety lame excuse :p

So no, I don't have any real good arguments for xml. Oh everyone can compare the syntax http://symfony.com/doc/2.0/cookbook/doctrine/event_listeners_subscribers...

The yml version isn't that bad. I do miss examples of the more complex cases...

pounard’s picture

Whatever is the format, I think that this is a very good idea! But we should probably keep the possibility for modules to use bundles if they need it, there is probably use case for which files are not enough? I don't know Symfony 2 well enough so I'm not certain.

catch’s picture

The loop in Views service registration is only short hand, there's nothing actually dynamic about the services being registered so it could use YAML it'd just be very longwinded.

@pounard msonnabaum brought up that even if we keep PHP as an option for service registration it doesn't have to be via bundles. 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.

Crell’s picture

catch: Sure. But if we're going to have a magically named class that exists to support service registration... why wouldn't we use the one that's already there? There is no "module definition class" in Drupal. I don't see why would invent a new one rather than using the model/interface that's there.

catch’s picture

@Crell because it comes with a lot of assumptions that aren't applicable with Drupal or actively conflict with concepts we already have (and I'd argue with concepts that Symfony itself has - I don't see the point of the boot() methods etc. over events).

Also if Views is the only example of a loop for registering services I think we could just remove support for dynamic registration for now - we already don't support that per discussion in other issues (i.e. registering stuff based on other stuff that depends on the container etc.).

Crell’s picture

Even if we switch over to config files for core services (which I'm not against at all), we should retain the ability to define them in code. Removing that is just needlessly painting ourselves into a corner.

chx’s picture

However, having code in there is a loaded gun. If you call code and it happens to be DIC-dependent which is more and more of our codebase then you are so throughly screwed. So, yes, let's remove code because there is little code that could be meaningfully ran from there (like that views shorthand but that can be a manual script generating that yaml needs to be ran once).

tim.plunkett’s picture

Title: Remove Symfony bundles from core, use something else for service registration » Use YAML as the primary means for service registration

Boldly retitling. In light of #1840914: Convert routes to CMI, does anyone want to go so far as to say "Use CMI as the primary means for service registration"?

EDIT: Considering that issue has gained little traction, let's just focus on adding support for YAML first.

chx’s picture

Right now we already presume that short of a module enable/disable nothing rebuilds the container. That's a presumption. If you have a contrib which wantonly puts dynamic code into the Bundle and then the module triggers a rebuild -- you'd think it works. Unless you have multiple webfrontends which won't know the need for rebuild. Let's not do this. Let's go YAML because it is static.

Isn't it enough that we have PHP in compiler passes?

msonnabaum’s picture

Excellent point about compiler passes. I think I'm convinced yaml is the way to go here.

plach’s picture

I am not enough into this stuff to have a strong opinion about this, but I'd like to mention #1862202: Objectify the language system as a possibile legitimate use case for allowing to execute PHP while rebuilding the DIC. Quoting @chx from #7 there:

To recap: language, when on, might want to override every CMI object we load so we have a circular dependency where you need to read various CMI objects to determine the language. To avoid this, I am converting the language negotiation callbacks into plugins and then copy the language negotiation settings into the container on compile time. To do the latter, I will change DrupalKernel::buildContainer to add the configStorage as a synthetic service and then LanguageBundle can use that to read whatever CMI objects are necessary and store them neatly in a container parameter. Thus, when we have a kernel, we have the negotiation services and we have the negotiation settings so as soon as we have a request to extract things from it we have the language and can override. Hurray!

The patch in #55 is trying to implement this plan. Would the PHP pass mentioned above make this possible?

chx’s picture

Looking at the patch: yes. If you look at the current usages, typically method calls are added to an existing registration, in this case that patch adds an argument but the registration is already done.

chx’s picture

Status: Active » Needs review
FileSize
11.02 KB

Status: Needs review » Needs work

The last submitted patch, 1939660_20.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
1.91 KB
11.92 KB

The only nontrivial change here is system_update_8046. It's passing wrong arguments to updateModules. I need to talk to dawehner about the test coverage of 8046.

* @var array
* An associative array whose keys are module names and whose values are
* ignored.
*/
protected $moduleList;

keys are module names. Keys. Not values. No. Keys. So. Why the array_keys? And why is chx on this commit message? What have I done?

chx’s picture

FileSize
11.75 KB

Minus debug.

effulgentsia’s picture

I agree with #17: switching to static YAML definitions and moving all dynamic work to compiler passes seems clean to me.

+++ b/core/lib/Drupal/Core/CoreBundle.yml

a) we shouldn't have YAML files in /lib
b) there's no need for "Bundle" to be in the name

Looks like in Symfony docs, the file name is just services.yml, so what about moving this to core/services.yml, and each module can have a PATH/TO/MODULE/services.yml file? Or else, core/core.services.yml and PATH/TO/MODULE/MODULE.services.yml?

effulgentsia’s picture

switching to static YAML definitions and moving all dynamic work to compiler passes seems clean to me

Er, it would if there were a way to register compiler passes via YAML, but I don't currently see how that's done.

chx’s picture

I am fine with moving to core/core.services.yml and PATH/TO/MODULE/services.yml

There will be some PHP left cos the twig registration is using new Definition which is not yet supported by YAML (but it'll be, I submitted the first attempt to Symfony, and it's well received just needs to be more complete).

This won't really hurt modules cos it'll be super rare for a module to use compiler passes.

chx’s picture

FileSize
109.47 KB

This converts almost all bundles, RouterTestBundle.php is the only one that can still be converted manually, the rest is dumped by YamlDumper and a little elbow grease.

Status: Needs review » Needs work

The last submitted patch, 1939660_27.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
108.63 KB

Status: Needs review » Needs work

The last submitted patch, 1939660_27.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
110.31 KB

Status: Needs review » Needs work

The last submitted patch, epic_yak_shaving_1939660_31.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
110.5 KB

Status: Needs review » Needs work

The last submitted patch, 1939660_33.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
110.61 KB

Status: Needs review » Needs work

The last submitted patch, 1939660_34.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
1.88 KB
111.91 KB
  1. The config tests were failing because I used the wrong constant for the staging dir.
  2. The serialization tests failed because I didnt flatten the serialization_test format tag
  3. Finally, poor ViewsUI tests were failing because that bundle neither got converted nor preserved.

interdiff does not like me, so I did a diff of diffs. It's less readable but it's still OK.

chx’s picture

Issue tags: +Avoid commit conflicts
FileSize
438 bytes
111.89 KB

Keeping up with HEAD. It's a pain to reroll this ( I even asked StackOverflow on how to make it easier) and would be a downright nightmare to reroll if a bundle changes. Note that interdiff is not working in this case but the diffdiff approach does and it works beautifully -- the Classloader commit changed the context of the patch, that's all.

Anonymous’s picture

chx made a functional change to the way the encoders are registered with the Serializer. This could cause problems, but they are minor. We can fix them in a followup, #1958484: Serialization uses addTag incorrectly. So no objection from me.

chx’s picture

FileSize
1.41 KB
110.54 KB

The error.inc changes are not necessary -- I knew that and I had the patches tested in a throway because I wanted this out there ASAP -- despite no reviews still :/ ( I know. I am so very patient.)

chx’s picture

FileSize
107.78 KB

dawehner asked for two spaces instead of four. This will be fine, in the future we are unlikely to use YamlDumper (and I have code to change the hardwired four in there already) and if you do, sed -i s/' '/' '/g is your friend. I already wrote ViewsUI services.yml by hand based on the numerous examples.

dawehner’s picture

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -178,6 +190,10 @@ public function registerBundles() {
+      $filename = dirname($module_filenames[$module]) . "/services.yml";

I'm wondering whether we considered to name it $module.services.yml, to be consistent with $module.routing.yml.

+++ b/core/modules/aggregator/services.ymlundefined
@@ -0,0 +1,10 @@
+    arguments: [fetcher, '%container.namespaces%']

It's odd to use inline values, but well, we have to live with that, even it is separate to all other yml files we have.

+++ b/core/modules/ckeditor/services.ymlundefined
@@ -0,0 +1,4 @@
+    arguments: ['%container.namespaces%']

+++ b/core/modules/views/views_ui/services.ymlundefined
--- /dev/null
+++ b/core/services.ymlundefined

Did we thought about making the file alphabetically sorted, so it's maybe easier to scan the list? I guess the existing coreBundle somehow got sorted by "it could be more important than X + i put it at the end"

chx’s picture

FileSize
108.72 KB

Alphabetic sorting IMO could be a followup... I hope I got my bash commands right and this will pass, moving services.yml to directoryname.services.yml.

The last submitted patch, 1939660_43.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
Issue tags: +Avoid commit conflicts

#43: 1939660_43.patch queued for re-testing.

msonnabaum’s picture

I don't mind the inline yaml arguments array. It's less consistent, but I see it as the same choice we make in code sometimes. Whichever is more readable is preferred.

Crell’s picture

For the record, we discussed this issue in IRC a few days ago in the context of the cache system. One of the big wins here is that it becomes possible to tell what services a given module wants to offer without having to rebuild the entire container (or, worse, do a partial fake build). That becomes important for #1167144: Make cache backends responsible for their own storage, as then we can introspect the services file for a given module on uninstall, see what cache bins it registered, and clean those up (ie, delete their DB tables). That's the argument that won me over.

Given that, and that bundle classes are still possible, if rare (they have to be so that we can register compiler passes), this approach has my approval. Looking at the patch, I admit the terseness of it is appealing. (I'd still support a follow-up to support XML as well, for cross-project compatibility. That's not for this issue, though.)

A few comments:

+++ b/core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php
@@ -0,0 +1,236 @@
+/**
+ * Drupal does not use Symfony's Config component, so this is a partial of the
+ * \Symfony\Component\DependencyInjection\Loader\YamlFileLoade class not
+ * depending on the Config component.

Docblock needs proper one line summary, then longdesc. The text here is fine as the longdesc, except "this is a partial of the..." Partial what? :-) "Excerpt" would work better than Partial here.

+++ b/core/modules/ban/ban.services.yml
@@ -0,0 +1,9 @@
+    arguments: ['@database']

Sometimes arguments are listed with @ before them, sometimes not. What's the difference?

+++ b/core/modules/serialization/serialization.services.yml
@@ -0,0 +1,24 @@
+    arguments: [{  }, {  }]

Can we just omit this line, if the arguments are null? Or is that needed for the compiler pass later? (I have vague memories of that...)

Berdir’s picture

@Crell: @somehing is a service, %something% an argument and others are just strings :)

chx’s picture

FileSize
913 bytes
108.85 KB
    $container->register('serializer', 'Symfony\Component\Serializer\Serializer')
      ->addArgument(array())
      ->addArgument(array());

became

arguments:
  [{  }, {  }]

please file a serialization issue if you don't like this, has nothing to do with this patch.

Fixed doxygen, keeping up with HEAD -- I hope this is the last reroll. I will put a bounty on http://stackoverflow.com/questions/15796528/partial-git-apply to get an answer but I am not holding my breath.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Given the fragility of this issue, and no one is objecting to the concept, and the patch is green, I'm going to go out on a limb. Tidying up the default YAML syntax we use can be a follow up as it wouldn't conflict with a dozen patches in the queue the way this does. :-)

chx’s picture

FileSize
741 bytes
109.11 KB
chx’s picture

FileSize
7.33 KB
111.36 KB

alexpott reviewed this one and so lots of comments and $GLOBALS['conf']['container_yamls'] is added.

webchick’s picture

Alex Pott asked me to look at this, so here's my 2 cents. Nothing blocking, so not moving down from RTBC, but if someone feels up to doing a few tweaks later on tonight, cool. I would prefer either Alex or Catch to commit this because I'm not as "Symfonic" as them.

Sorry, I'm doing this real quick before supper, so I might be covering things in earlier comments.

75 files changed, 1110 insertions, 1386 deletions.

What a lovely diffstat! :D

+++ b/core/core.services.ymlundefined
@@ -0,0 +1,372 @@
+  entity.query:
...
+  entity.query.config:

Same question as in #1957148: Replace entity_query() with Drupal::entityQuery().. should these really be registered as "core" services, or are they more "entity" services?

Over there, we didn't move them to an Entity class because of namespace collisions, but I don't think YAML files would have the same problem.

+++ b/core/core.services.ymlundefined
@@ -0,0 +1,372 @@
+  path.alias_manager.cached:

Similar question; should this one be in core.services.yml, or path.services.yml?

The other path ones don't seem to be related to aliases. Might just make sense to keep them all together, not sure.

+++ b/core/core.services.ymlundefined
@@ -0,0 +1,372 @@
+# The argument to the hashing service defined in services.yml, to the
+# constructor of PhpassHashedPassword is the log2 number of iterations for
+# password stretching. This should increase by 1 every Drupal version in order

We should mark this chunk of comments with a @todo so there's a hope we find it again next Drupal version. :)

+++ b/core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.phpundefined
@@ -0,0 +1,255 @@
+ * YamlFileLoader loads YAML files service definitions.
+ *
+ * Drupal does not use Symfony's Config component, so this is a partial copy
+ * of \Symfony\Component\DependencyInjection\Loader\YamlFileLoader class not
+ * depending on the Config component.

Ick. Can we please file an upstream issue to decouple those? This is an awful lot of code to copy/paste and be responsible for maintaining.

+++ b/core/modules/rest/rest.services.ymlundefined
@@ -0,0 +1,21 @@
+    arguments: ['%container.namespaces%']
...
+    arguments: ['@plugin.manager.rest', '@config.factory']

We're gonna need some pretty good docs on what the heck all these special symbols do. I've no clue from a glance.

Crell’s picture

Entity API itself in in /core/lib, so it makes sense to be registered by core.

Path aliasing is in a weird place; right now it's still totally hard coded into core so it should be registered by core. Once the generator issue gets in, it will be just another sub-service, meaning it will become possible to run Drupal entirely sans-aliasing. At that point we can discuss if we should move all of the aliasing logic to path module, in which case the service registration would go with it. (I'm open to it.) We can't do that yet, though, until the generator issue gets in.

chx’s picture

FileSize
625 bytes
111.36 KB

We are not changing service definitions beyond what's absolutely necessary (ie serialization using patently disalllowed stuff that happens to not throw exceptions in PHP yet). If you are unhappy with those file separate issues. It's hard enough to roll this 1:1.

> Ick. Can we please file an upstream issue to decouple those? This is an awful lot of code to copy/paste and be responsible for maintaining.

I will.

> We're gonna need some pretty good docs on what the heck all these special symbols do. I've no clue from a glance.

Fully agreed. Where?

> We should mark this chunk of comments with a @todo so there's a hope we find it again next Drupal version. :)

Here.

effulgentsia’s picture

We're gonna need some pretty good docs on what the heck all these special symbols do....Fully agreed. Where?

Um, well, % is documented here: http://symfony.com/doc/current/book/service_container.html#service-param.... And @ is documented here: http://symfony.com/doc/current/book/service_container.html#referencing-i.... So I don't think we need to add our own docs on top of that.

alexpott’s picture

Committed 34e6309 and pushed to 8.x. Thanks!

Removed some rogue whitespace during commit... patches attached to show what I did.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

And now updating the status... thanks @timplunkett

Crell’s picture

Title: Use YAML as the primary means for service registration » Change Notice: Use YAML as the primary means for service registration
Priority: Major » Critical
Status: Fixed » Active
Issue tags: +Needs change record

I don't know if this is a new change notice, or an update to an existing one. Either way, there needs to be change notice action here.

chx’s picture

Title: Change Notice: Use YAML as the primary means for service registration » Use YAML as the primary means for service registration
Priority: Critical » Major
Status: Active » Fixed

http://drupal.org/node/1539454 updated. I fixed it to use the event_subscriber tag as well.

chx’s picture

I also updated http://drupal.org/node/1837872 and filed an issue #1965128: The password inc change notice is both broken and outdated for the password inc change notice. There are no more occurences of bundleinterface.

Status: Fixed » Closed (fixed)

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

katbailey’s picture

I've created a follow-up issue about getting rid of the notion of bundles altogether: #1988508: Stop pretending we support Symfony bundles when we really just have service providers
Would love to get input from y'all.

xjm’s picture

Issue tags: -Needs change record

Untagging. Please remove the "Needs change notification" tag when the change notice task is complete.

xjm’s picture

Issue summary: View changes

Updated issue summary.