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.
Comment | File | Size | Author |
---|---|---|---|
#57 | 55-57-interdiff.txt | 623 bytes | alexpott |
#57 | 1939660_57-do-not-test.patch | 111.35 KB | alexpott |
#55 | 1939660_55.patch | 111.36 KB | chx |
#55 | diffdiff.txt | 625 bytes | chx |
#52 | 1939660_52.patch | 111.36 KB | chx |
Comments
Comment #1
Crell CreditAttribution: Crell commentedI 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.)
Comment #2
msonnabaum CreditAttribution: msonnabaum commentedI'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.
Comment #3
catchI'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.
Comment #4
Crell CreditAttribution: Crell commentedTo 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.
Comment #5
tim.plunkettWell, 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.
Comment #6
aspilicious CreditAttribution: aspilicious commentedI 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. :)
Comment #7
Crell CreditAttribution: Crell commentedaspilicious: Can you elaborate on why it's way easier to do in XML?
Comment #8
aspilicious CreditAttribution: aspilicious commentedThey 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...
Comment #9
pounardWhatever 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.
Comment #10
catchThe 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.
Comment #11
Crell CreditAttribution: Crell commentedcatch: 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.
Comment #12
catch@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.).
Comment #13
Crell CreditAttribution: Crell commentedEven 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.
Comment #14
chx CreditAttribution: chx commentedHowever, 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).
Comment #15
tim.plunkettBoldly 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.
Comment #16
chx CreditAttribution: chx commentedRight 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?
Comment #17
msonnabaum CreditAttribution: msonnabaum commentedExcellent point about compiler passes. I think I'm convinced yaml is the way to go here.
Comment #18
plachI 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:
The patch in #55 is trying to implement this plan. Would the PHP pass mentioned above make this possible?
Comment #19
chx CreditAttribution: chx commentedLooking 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.
Comment #20
chx CreditAttribution: chx commentedComment #22
chx CreditAttribution: chx commentedThe 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.
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?
Comment #23
chx CreditAttribution: chx commentedMinus debug.
Comment #24
effulgentsia CreditAttribution: effulgentsia commentedI agree with #17: switching to static YAML definitions and moving all dynamic work to compiler passes seems clean to me.
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 tocore/services.yml
, and each module can have aPATH/TO/MODULE/services.yml
file? Or else,core/core.services.yml
andPATH/TO/MODULE/MODULE.services.yml
?Comment #25
effulgentsia CreditAttribution: effulgentsia commentedEr, it would if there were a way to register compiler passes via YAML, but I don't currently see how that's done.
Comment #26
chx CreditAttribution: chx commentedI 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.
Comment #27
chx CreditAttribution: chx commentedThis 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.
Comment #29
chx CreditAttribution: chx commentedComment #31
chx CreditAttribution: chx commentedComment #33
chx CreditAttribution: chx commentedComment #35
chx CreditAttribution: chx commentedComment #37
chx CreditAttribution: chx commentedinterdiff does not like me, so I did a diff of diffs. It's less readable but it's still OK.
Comment #38
chx CreditAttribution: chx commentedKeeping 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.
Comment #39
Anonymous (not verified) CreditAttribution: Anonymous commentedchx 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.
Comment #40
chx CreditAttribution: chx commentedThe 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.)
Comment #41
chx CreditAttribution: chx commenteddawehner 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.Comment #42
dawehnerI'm wondering whether we considered to name it $module.services.yml, to be consistent with $module.routing.yml.
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.
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"
Comment #43
chx CreditAttribution: chx commentedAlphabetic 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.
Comment #45
chx CreditAttribution: chx commented#43: 1939660_43.patch queued for re-testing.
Comment #46
msonnabaum CreditAttribution: msonnabaum commentedI 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.
Comment #47
Crell CreditAttribution: Crell commentedFor 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:
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.
Sometimes arguments are listed with @ before them, sometimes not. What's the difference?
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...)
Comment #48
Berdir@Crell: @somehing is a service, %something% an argument and others are just strings :)
Comment #49
chx CreditAttribution: chx commentedbecame
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.
Comment #50
Crell CreditAttribution: Crell commentedGiven 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. :-)
Comment #51
chx CreditAttribution: chx commentedComment #52
chx CreditAttribution: chx commentedalexpott reviewed this one and so lots of comments and $GLOBALS['conf']['container_yamls'] is added.
Comment #53
webchickAlex 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.
What a lovely diffstat! :D
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.
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.
We should mark this chunk of comments with a @todo so there's a hope we find it again next Drupal version. :)
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.
We're gonna need some pretty good docs on what the heck all these special symbols do. I've no clue from a glance.
Comment #54
Crell CreditAttribution: Crell commentedEntity 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.
Comment #55
chx CreditAttribution: chx commentedWe 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.
Comment #56
effulgentsia CreditAttribution: effulgentsia commentedUm, 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.
Comment #57
alexpottCommitted 34e6309 and pushed to 8.x. Thanks!
Removed some rogue whitespace during commit... patches attached to show what I did.
Comment #58
alexpottAnd now updating the status... thanks @timplunkett
Comment #59
Crell CreditAttribution: Crell commentedI 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.
Comment #60
chx CreditAttribution: chx commentedhttp://drupal.org/node/1539454 updated. I fixed it to use the event_subscriber tag as well.
Comment #61
chx CreditAttribution: chx commentedI 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.
Comment #63
katbailey CreditAttribution: katbailey commentedI'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.
Comment #64
xjmUntagging. Please remove the "Needs change notification" tag when the change notice task is complete.
Comment #64.0
xjmUpdated issue summary.