Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
drupal_container()->get('some.service.name')
has two big problems: a) the possible service names are hard to discover b) the type of the returned object is totally unknown by IDE or a casual Drupal programmer.
Proposed resolution
- For common services provided by the Drupal core base system (eg, the "top 20" that would be used):
- drupal_container()->get('flood')->register($event_name); + Drupal::flood()->register($event_name);
-
For a service provided by a Drupal module (the core Edit module in this case):
- drupal_container()->get('edit.editor.selector')->getAllEditorAttachments(); + Edit::editorSelector()->getAllEditorAttachments();
-
And for a service that is obscure, rarely used, or not intended to be used as a public API:
- drupal_container()->get('some.service')->someMethod(); + Drupal::container()->get('some.service')->someMethod(); + Drupal::service('some.service.')->someMethod(); // Either this or the previous line, not both.
Remaining tasks
This needs coding.
User interface changes
This is backend, no UI changes.
API changes
See the proposed resolution.
Original report by effulgentsia
This keeps coming up, and Dries asked for it in #1269742-88: Make path lookup code into a pluggable class, so here's an issue to discuss it.
Comment | File | Size | Author |
---|---|---|---|
#166 | 1875086.patch | 18.86 KB | RobLoach |
#166 | interdiff.txt | 1.03 KB | RobLoach |
#143 | drupal-container_get-1875086-143.interdiff.do_not_test.patch | 2.35 KB | plach |
#143 | drupal-container_get-1875086-143.patch | 19.02 KB | plach |
#124 | drupal-container_get-1875086-124.patch | 20.06 KB | msonnabaum |
Comments
Comment #1
tim.plunkettOften times if I repeat drupal_container()->get() calls within a method/function, I'll do
Now, I realize that drupal_container() is wrapped in a static, but leaving things as is will also make any future conversions away from the drupal_container() procedural function easier.
Comment #2
vijaycs85if we are going to use it, instead of:
we can go with:
Comment #3
David_Rothstein CreditAttribution: David_Rothstein commentedSo if you compare Drupal 7 to Drupal 8 what you roughly see is lots of changes like this (with lots of different API functions substituting for the example below):
This is bad developer experience for three reasons:
Switching to drupal_service() partially addresses #1 and #2 (but not fully), and does not address #3.
Meanwhile, it might also introduce some inconsistencies, since I thought one of the purposes of drupal_container() was to introduce parity with certain objects that must call $this->container->get('path.alias_manager')->getPathAlias() so as to use a passed-in container... On the other hand, it doesn't actually seem like this is done consistently across the codebase at all; for example, in the particular case of path aliases, I'm seeing things like $this->aliasManager->getPathAlias() in the codebase, where there's no sign of any "container" anyway.
Putting that all together, I think I'd be in favor of going further and doing something that is more specific than drupal_service() for whichever service you're actually using. For example:
path_manager()->getPathAlias()
?Comment #4
David_Rothstein CreditAttribution: David_Rothstein commentedBy the way, given the number of times I've seen this come up in various issues lately (in comments by various developers starting to work with Drupal 8 for the first time), as well as how widespread its use is across the codebase, I think we should really consider bumping this to a "major" priority developer experience issue... but not doing that just yet :)
Comment #5
tim.plunkettAs far as #3.3 goes,
debug(drupal_container()->getServiceIds());
is your friend.The path_manager()-esque wrappers have been discouraged, and are actively being removed from core.
I am of the opinion that while for accomplishing one task, seeing drupal_container() is overwhelming, but if we cut down on the wrappers of that, you'll see it A LOT and then once you understand it, you'll understand that much more about new systems also using the container.
drupal_container() -> Manager -> Bundle -> Container
Now you know 4 things!
Comment #6
adamdicarlo CreditAttribution: adamdicarlo commentedFrom a contrib dev's perspective: The API should be simple and shield us from core's inner workings. For simply using D8's out-of-the-box API, I'd hate to ever have to call drupal_container() and look up the configuration variable path to use with it.
How about wrappers like this?
Allowing this usage:
Seems to me like this could decouple various core modules' from each others' implementations, too.
This might not speak to the heart of the problem; I'm not sure. Just my thoughts.
Comment #7
Crell CreditAttribution: Crell commentedTim is correct.
Because we're in a transitional state, there are a variety of ways to get at services offered by Drupal. In order of the "rightness" of the approach, they are:
1) Have an object injected directly into your object by the DIC. In this case, your code knows nothing of containers, nor should it.
2) Make your object ContainerAware, and have the DIC (or something else) inject the container into it so that you can do $this->container->get('service_id'). This is mostly the case for objects with unpredictable dependencies, like Controllers, that are mostly glue code and so not really intended for heavy testing in the first place.
3) drupal_container()->get('service_id'). This is a fallback for code that doesn't have the ability to get injected services, mostly procedural code. This has approximate parity with option 2, in that drupal_container() ~ $this->container.
4) service(), e.g. cache(). This has no parity with anything except for legacy code, where such wrappers already existed. Mostly these are for BC only, and I'd like to see them go away, too.
Why is that the order? Simple. Only code in the first group can be unit tested. There are ugly ways to make the second one unit testable, by creating a fake container, but they're ugly. The third and forth are even harder to unit test because they're accessing a global. Basically if code is using drupal_container(), you give up on unit testing. If you want your code unit testable, you don't use drupal_container(). The two are mutually exclusive.
The extra wrapper functions don't change that. They simply obscure it. And obscuring a design flaw is exactly what a good API should not do.
So why do we favor option 3 over option 4? The answer is the same as why the syntax in C++ for type casting is godawful. "An ugly operation should have an ugly syntactic form." (Bjarne Stroustrup, creator of C++) You're accessing a global and breaking any hope of worthwhile unit testing. That is an ugly thing to do. It shouldn't have a pretty syntax. You should instead be refactoring your code into self-contained services that just get injected objects, and then your code contains no direct or indirect references to the container at all.
As to the other points, just having a crapload of utility functions does not make the list of available services any more discoverable. All it does it make a larger list of functions that we have to load into memory before we can even try to use Drupal, "just in case". We should have a debug way to get a list of all available services, yes. Probably a CLI command, or a Drush command, or something like that.
They also do not in any way help with decoupling of different systems. As above, you still can't actually swap out implementations. Whether you have a wrapper around drupal_container() or not, you're still hard coded to a global object. But even then, drupal_container() is a stepping stone away from just hard coding function calls to deep within other systems.
Comment #8
quicksketch@Crell: I'm having trouble understanding your first two suggestions because they are based on the premise that you already have an object in which you can stash the container and manager. Can you describe where this object would come from assuming you were in a traditionally procedural location, such as a menu callback, in a form building function, or node API hook?
While writing *brand new* APIs in #1833716: WYSIWYG: Introduce "Text editors" as part of filter format configuration, we're still using approach #3, calling drupal_container() directly. If using drupal_container() is far down on the list of "right things" to do, we need better patterns to follow. Any suggestions?
Comment #9
effulgentsia CreditAttribution: effulgentsia commentedI agree that refactoring *some* code from procedural to OOP, so that dependencies can be properly injected ala #7.1, is desirable. But I think it's nearly certain that we'll still have some procedural code (e.g., hook implementations) left in D8, and for that code, I agree with the problems identified in #3. I also agree with #7 that a proliferation of global wrapper functions is not ideal. How about having each Bundle class supply static wrappers that can be used by procedural code? Proof of concept attached, but for anyone not wanting to read all of it, here's the important part:
Comment #10
Wim LeersGreat question, quicksketch :) (Hint: I'm the one who's doing it wrong in the issue quicksketch links to, "wrong" by the standards set forth in #7.)
Comment #11
Crell CreditAttribution: Crell commentedFor procedural code, yes, drupal_container() is the best we can do. That's because procedural code is naturally not injectable, which is one of the problems of procedural code. :-) If you're writing a new API for Drupal 8, it should be mostly OO. I know not everyone agrees with this statement, but I would consider "writing a new API in Drupal 8 that's not OO" to be a serious design flaw right from the start (unless it has a natural dependency on some existing system that's not going OO, such as FAPI, in which case that's really hard to do).
That's one of the reasons why page callbacks are (hopefully) going away in favor of controller objects, and I want to convert node hooks into events.
An injected mindset is definitely a switch from what we're used to; that's not debatable. But it is a worthwhile and very important switch for us to make.
@ #9: The only advantage of putting the wrapper on the bundle class is that it then lazy-loads. It doesn't address any of the other drawbacks. However, it then brings with it the rest of the bundle class, which we specifically setup so that it's not loaded on every request.
Comment #12
David_Rothstein CreditAttribution: David_Rothstein commentedThat just prints of list of available string keys, but doesn't help me find documentation for the function I'm using. To figure that out, I have no information about which classes/interfaces this is related to, so I basically have to search the codebase for all functions named getPathAlias(). Luckily in this case there are only three of them so it's not too bad, but that's still around three times harder than it should be. It gets a lot more fun when the method is named something like save().
I've eventually figured out that if you search the codebase for something like
$container->register('string')
you can usually get an idea which class the method is attached to. And of course, if you are actually editing code (rather than just reading it) you can dodebug(drupal_container()->get('string'))
as well. But none of this is a substitute for actual, easy-to-access documentation that tells me what kind of object I should be expecting.In principle something like
PathBundle::aliasManager()->getPathAlias()
works here but it's still a bit conceptually heavy, in my opinion (two method calls rather than one, not to mention the PSR-0 class you need to use it and the word "bundle"). And as Crell said, it forces all the bundle code to be loaded. I don't really understand the problem with global wrappers (for the commonly-used things in the container); there aren't really likely to be that many of them that it's a burden to have them defined in the global namespace. In any case, whatever we do here should be consistent and there are a number of such global wrappers already, including at least a couple (config() and state()) that were introduced in Drupal 8.Having a wrapper function still allows anyone who wants to understand how it works to easily read the code inside the wrapper function. But for someone trying to use Drupal 8 to build an actual website, I have yet to see the value of this; if you are trying to figure out how to do something with path aliases, you really just want to get the path alias and understand how to use that directly.
PHP has a sort() function which contains an internal algorithm to sort things. I think it is only a bit of an exaggeration to say that what we are doing currently in Drupal 8 is as if instead of sort(), PHP made you call something like php_algorithm()->get('sort')->sortItems() every time you wanted to sort an array in procedural code.
Comment #13
webchickI think #12 has convinced me this needs to be major.
Comment #14
tim.plunkett#7 convinced me that this is won't fix. So I'll just let the issue run its course...
Comment #15
BerdirI agree that injecting services is the most optimal case, but that is not an argument for making drupal_container() as ugly as possible to use.
About the problem of which service id's exist and what they do, see the discussion in #1862398: [meta] Replace drupal_http_request() with Guzzle. The references cli script is app/console container:debug I guess, see at the bottom of http://symfony.com/doc/current/book/service_container.html. I guess we can do something similar and generate a list of services that core provides and put it somewhere very public (what about a new docgroup that is listed directly on api.drupal.org for the whole container/services thing?). With a link to the interface, of course. Maybe api.module can do that? Either with retrospection directly out of the code, or by introducing something like @service service_id \Drupal\something\MyInterface. Which would allow to separate between internal helper services and public ones that are considered API's.
The unit test argument is IMHO a bit flawed, only very few things are really unit testable in Drupal at the moment. Everything that relies bundles, modules, plugins, ... quickly gets complicated. As I've said before, our current Plugin and Router Unit tests rely on unpredictable/broken behavior, for example that plugins of disabled modules are still found. Which is going to change as that is a bug and then we need to use DrupalUnitTestBase there, just like in most other cases. Which IMHO, is quite nice to use and also allows to test .module and drupal_container() code.
Comment #16
Grayside CreditAttribution: Grayside commentedOne side says "This is ugly because it shouldn't be used. Use better ways."
Other side says "This seems to be used everywhere, and most core contributors don't grok 'better ways'."
Sounds like a documentation problem. Or signs of Symfony indigestion.
Comment #17
David_Rothstein CreditAttribution: David_Rothstein commentedI don't understand #7; if you think $this->aliasManager->getPathAlias() is the number one option, then why wouldn't we want our procedural code to look as close to that as we possibly can? This promotes consistency and gets people in the right mindframe for how their code should be organized.
If I see drupal_container() calls all over the place, all it tells me is that Drupal thinks this "container" idea is great and I probably should be using it everywhere I can. So when it comes time for injectable OO code, I'm just going to try to structure everything so that it uses $this->container. But that's the wrong conclusion to draw.
Comment #18
effulgentsia CreditAttribution: effulgentsia commentedWhat would "as close to that as we possibly can" be, given procedural code limitations?
$this
withdrupal_container()
and replacealiasManager
with something localized to the 'path' subsystem, and end up with something likedrupal_container()->pathAliasManager
which isn't all that different fromdrupal_container()->get('path.alias_manager')
$this
with something localized to the 'path' subsystem (e.g.,PathBundle
) and end up with something likePathBundle::aliasManager()
Comment #19
BerdirAnd how would that work? That we can't really inject stuff in procedural code is exactly the problem, we just have hardcoded function calls calling other functions in a hardcoded way. The only place where we can inject stuff are the function arguments, and that just means that the function that calls it a) needs to know what that function implementation needs and b) it needs to call drupal_container() itself.
I still think the original purpose of this issue does make sense we just went quite off-topic with the discussion.
Consider this example:
We have 3 get() calls and each of them does something completely else. (container -> service factory -> implementation)
That is not that much better, but I think it is an improvement and is more readable.
And I've been thinking about a way to have dynamic services with an argument that would be useful with service factories (so you could define keyvalue.%collection) and the container would know that keyvalue is the service and knows that it needs to call service.getCollection($collection) or something like that. This would be very useful with things like keyvalue, config, cache and so on, where constructors are currently complicated because we have to pass the key value factory to another service that then needs to get the actual collection) But that, again, is completely off topic :)
Comment #20
webchick"end up with something like drupal_container()->pathAliasManager which isn't all that different from drupal_container()->get('path.alias_manager')"
The big difference there is pathAliasManager() is a method and can be PHPDoced.
Comment #21
effulgentsia CreditAttribution: effulgentsia commentedThere are currently 251 places in HEAD's codebase that match
drupal_container()->get('
. This includes tests and does not include alternate constructs like$container = drupal_container(); $container->get('
. Of these 251 occurrences, the most common services gotten are:- 46: plugin.manager.FOO (there are many different plugin managers, so I might be cheating here by lumping them all together)
- 30: request
- 29: views.views_data
- 29: path.alias_manager
- 21: path.crud
- 15: user.data
That leaves 81 others. I didn't get statistics on those. Someone else can if motivated. These numbers will surely change over the coming months, and I don't yet know what conclusion to draw from them, just sharing the data.
But if we decide to provide a global wrapper for commonly called services and not for uncommonly called ones, does that satisfy the desire for consistency, or will people still be confused why sometimes you call request() and other times you call drupal_container()->get('some_rare_service')?
Comment #22
webchickThis is a fair argument, but I see it as akin to the following argument:
In Drupal 7, we didn't have a consistent data manipulation API, so you'll notice in some places in http://drupalcode.org/project/drupal.git/blob/refs/heads/7.x:/profiles/s... for example we do function calls like
menu_link_save($item);
and in some other places we do direct DBTNG calls likedb_insert('users_roles')->fields(array('uid' => 1, 'rid' => $admin_role->rid))->->execute();
. Because there wasn't a way to use nice wrapper functions everywhere, we should've therefore removed the ones we did have in favour of raw SQL calls.I see this argument as the same. While
db_insert('menu_links')->fields(array('link_title' => st('Home'), 'link_path' => '<front>', 'menu_name' => 'main-menu'))->execute();
is undoubtedly more consistent, I don't think there's anyone alive that would say it has better DX thanmenu_link_save($item);
Comment #23
David_Rothstein CreditAttribution: David_Rothstein commentedIn my opinion, the closest equivalent of $this->aliasManager->getPathAlias() would be alias_manager()->getPathAlias(). The concept of "$this" doesn't exist in a procedural context so it's simply removed rather than replaced. (Or to make it even more consistent, the former could actually be $this->aliasManager()->getPathAlias() if someone wanted to define the necessary methods, but I'm not sure it's always worth the effort.)
This matches the pattern already in use in certain places. For example, Drupal 8 already has instances of state()->get() vs. $this->state->get(), and a few other similar ones.
Comment #24
Crell CreditAttribution: Crell commentedI don't see drupal_service() as defined in the patch in the OP as having much value, but I could live with it.
For me, the key goal is pushing developers toward a properly-injected setup, and hopefully getting as far as removing drupal_container() in Drupal 9. That means, yes, in order to code for Drupal you need to know what a service container is. It's hardly the first time we've expected Drupal devs to understand some programming concept. Having a plethora of wrapper functions hides that part of the architecture rather than providing a learning opportunity for developers.
David: Yes, we've added a few procedural wrappers in Drupal 8. I disagree with nearly all of them. :-) Many (like config()) are frankly left-overs from pre-container work that was done in a more procedural way and haven't been cleaned up yet.
IMO we should not make a doc page listing all core services. That's a waste, because contrib services are no less important. We should bundle into core a CLI utility that dumps information on all available services, with whatever information we can extract.
As for unit testing, yes, most of core is not unit testable at present. That's a bug, not an excuse. :-) I also challenge the claim that the router unit tests are based on flawed assumptions. The only thing they rely on is Simpletest's provided database connection, and that is because we have no facility to mock the database. Everything else is completely self-contained.
Comment #25
quicksketchI think @webchick's rebuttal in #22 lays out the argument for wrapper functions quite nicely. Just because we haven't had consistent CRUD functions in the past doesn't mean that the use of CRUD functions (which essentially wrap around db_query() or drupal_write_record()) are a bad thing. From @Crell's side of the story:
A) drupal_container() was made intentionally ugly, which should discourage its use.
B) Drupal 8 won't be able to fix this ugliness and drupal_container() is going to be required in common places like menu handlers, form building functions, or any procedural location.
Given both of these statements, wrapper functions effectively solve the problem temporarily until supposedly in Drupal 9, when the requirement of drupal_container() goes away. Perhaps we shouldn't do this in ALL places (just like we sometimes go to straight SQL queries), but in places where a module is exposing public information, such as the list of text formats, entity types, etc, using a wrapper is okay, and even encouraged, so that the function can be documented and discovered.
It looks like this exact same argument has played out already in these issues:
#1269742: Make path lookup code into a pluggable class
#1862398: [meta] Replace drupal_http_request() with Guzzle
In these cases, @Crell has repeated the statements that "eventually" and "ideally" use of drupal_container() will be unnecessary. But that's not where we are in Drupal 8. Long-term we're going to be able to eliminate almost all uses of drupal_container() when we cease to use procedural code. In the mean time we *are* still using procedural code. There is no benefit in intentionally making procedural code difficult to use while its use is still required.
Comment #26
tim.plunkettI don't think the comparison in #22 is fair.
As I understood it, this issue is about wrapping our usage of drupal_container() as a service locator.
However, that comment implies we'd have wrappers for each method of each object.
In D7 we had:
Now we have:
This issue is proposing something like:
Whereas #22, with the example of menu_link_save(), contradicts that.
If we go with something like path_crud for each registered service, it will improve DX in the sense that it will be easier to use those. But I still think it will make the DIC more scary by keeping it hidden from developers.
Comment #27
quicksketchThere are two proposals on the table here I think:
1. Make a global wrapper around drupal_container()->get('some_service').
2. Make individual wrappers around drupal_container()->get('some_service')->someAction().
The benefit of #1 is purely in shortening the typing. It doesn't have any advantage in terms of discoverability or documentation though.
The #2 approach is proposing that we continue to keep our existing functions (or modify as needed) that wraps around the entire action. In the path example, this could mean that path_save(), path_load(), and path_delete() stick around until Drupal 9.
Comment #28
tim.plunkettOption #1 DOES give us documentation, and possibly discoverability. Because then you have a procedural function with a docblock.
Option #2 makes me very very sad.
Option #3 is to mark this "won't fix".
Comment #29
quicksketchOption 1 only gives you a single function for each set of an estimated 4 methods (assuming we're talking about a standard CRUD situation). It helps slightly with discoverability but probably not as effectively as separate functions, since you still have to go digging to locate the original object that registered the service.
Option 2 essentially would mean there would be a procedural and OO way of handling most tasks. Similar to PHP itself, you'd use the one that matched your current situation. Until we can get through this transitional period where we're using both procedural and OO, I don't see why this is a bad thing.
Comment #30
chx CreditAttribution: chx commentedHeh. Wazzup guys, are you waking up? Took long enough. I wonder how long it will take to realize I was also right in everything else I am bitching about for year+ now. But, I am taking whatever little I get.
#1852512-17: Directory structure is fail, part 1: Composer "My recommendation, would be to make your IDE open classes for you when you double click on them". That ain't happening with
drupal_container()->get()
cos no IDE can figure that out.I started writing a hook upgrade cheatsheet but found two things. A) there are not a lot of hooks I can mark removed or even changed meaningfully B) even the absolute newest API employs hooks to alter because we do not have another way to do it. This is hardly a critique of blocks-as-plugins but a fact: hooks are staying. If hooks are staying, you must have procedural code working. And I do not mean an also-run-poor-cousin-from-procedureville but a nice, usable DX.
Having a wrapper around drupal_container()->get('some_service') is plenty. All I need from that wrapper is the @return. Then when I write foo_wrapper()-> my IDE will give me a dropdown of the available methods. If I do not use an IDE I can fight our idiotic directory structure and eventually find the class.
Wrapping every method of the interface that drupal_container()->get('some_service') @return seems an awful waste to me.
Comment #31
chx CreditAttribution: chx commented> since you still have to go digging to locate the original object that registered the service.
Well, the truth is that developing D8 without an IDE will be a truly miserable experience regardless of this issue.
Comment #32
BerdirAgreed with what @chx said, the most useful thing of the wrapper functions is @return.
You might not notice that when working with API's that you know already. But I started on converting stuff to Guzzle, which was a completely API to me and I had to look through the guzzle classes more than once, looking if and which method I need to use. Would have been *much* easier if my IDE would have helped me there.
The only problem I see with that is what @timplunkett said, the existence of these helper functions could hide the existence of the container or make it seem like a magical, hard to understand thing, less so in #option 1 than 2.
Quoting David_Rothstein from #23:
The problem is that while this looks similar, it is not. when you don't know/understand that alias_manager() is a service, then you will just continue to use that in your classes, and it will work fine. I think the step from using those wrapper functions to using classes with injected dependencies is bigger than when you are aware of the container.
But I think this is something that we can help with better documentation. I still think we need to add a new top-level item on api.drupal.org about the whole service container thing. How to use it in which scenario, how to add your own services to it, how to properly inject things, ... Whether or not that will feature a list of services provided by core seems to be argued about although I don't quite understand the counter-arguments (the fact that we can't document contrib was never an argument against documenting core? And while contrib can replace services, it can not replace the interface that those services implement?). Maybe open a separate documentation issue to figure that out?
Comment #33
xjmI had a negative reaction to this being "major" over shortening a function name, but @berdir and @chx are trying to convince me it's not actually about the number of characters but about understanding what the returned thing is. Retitling. I'm still not sure I agree it's major.
Comment #34
dawehnerThis problem does not only exist in Drupal, but also on symfony itself, see https://groups.google.com/forum/?fromgroups=#!topic/symfony-users/_VVA1t... as one example of many.
The solution which is proposed there is
which is ugly from my point of view, but at least allows people to easier understand what is going on.
Comment #35
chx CreditAttribution: chx commentedPutting that burden on the consumer side? Ugh? What? isnt the very problem that the developer has no clue what the interface is? And, while there's widespread IDE support for @return how's IDE support for @var? No, i don't like that idea.
Comment #36
David_Rothstein CreditAttribution: David_Rothstein commentedFor documentation I think we can do something like
@defgroup services
, and then any contrib module can add to that using@ingroup services
. The defgroup documentation would also be the place to explain the overall concept of the services container.It's true that people will probably call things like alias_manager()->getPathAlias() in situations where they could have injected it, but the same thing is true for calling drupal_container(). The only difference I see is that when reviewing or reading code, instances of drupal_container() are easier to spot. However, it shouldn't be that different; pretty much anything with the pattern
function()->method(...)
is what you'd be looking for.Comment #37
BerdirOpened #1881612: Define a services @defgroup and add add related classes to it
Comment #38
sunNote sure whether this helps, but I like where @effulgentsia was heading towards.
Apparently, all ::get*() methods on a compiled + dumped Container have proper phpDoc including appropriate @return statements already.
The general idea of attached patch is to simply leverage what's already there. Essentially translating into this:
To actually leverage the @return, I guess we'd have to actually use the compiled method names (i.e.:
->getPathAliasManagerService()
) or futz with the compiled container's method names a bit.In general, I agree with @Crell on encapsulation and testability.
However, I also agree with @David_Rothstein, although not necessarily on the arguments/considerations that have been raised so far. The new design is generally good. People can and will learn it.
The real and biggest problem we're starting to face more and more is the total loss of "grep-ability" for function usages. There have been a couple of grep examples in this issue already, and reality is that you can actually be dead sure that the results of those greps are incomplete (unless some advanced grepping actually happened but was left out of the comments here).
For instance, if I were to refactor/rewrite some config-related stuff, we're currently able to do:
With the new injected, swappable, pluggable, and indirected code, that no longer works, because all kind of config calls are buried into advanced patterns like:
In short: Former "config()" is used in places that are close to impossible to find. You need the testbot + top-notch test coverage to find them for you.
Part of that cannot be avoided and is just natural to how OO works, and only good standards and well-authored examples are able to keep the system greppable to a certain extent.
The other parts are places where the indirection happens right in your face through service container calls that refer to service IDs instead of concrete function invocations. The service IDs cannot be resolved into the actual mapped classes, unless the code is actually executed. This is what breaks IDEs, but IMHO even more importantly, api.drupal.org.
Long story short:
drupal_yada()->yada(yada)->yada()
... generally shouldn't be a concern at all. Over the past couple of years, you've learned and taught yourself what$yada['#yada] = array('yada', 'yada');
actually means and you've turned that into a habit, with outstanding, exceptional success. :) That won't change and will continue to happen and succeed, since the abstraction is still yada.The only substantial difference that will cause us major maintenance problems, headaches, and nightmares is that our legacy systems had a surprisingly excellent discovery and grepability baked in. We were able to simply grep for #yada or yada() to get concrete + concise + absolute results and be happy. Unless we do something about it, we can completely forget about that and will face a virtually unmaintainable system. Likewise, "let me grep contrib for yada" will be a thing of the past.
Comment #39
effulgentsia CreditAttribution: effulgentsia commentedYes, but there's three problems with trying to leverage that:
- Those methods are protected, so they wouldn't autocomplete in an IDE.
- We'd have to modify the @return of drupal_container() to the name of the compiled class (based on current implementation of DrupalKernel::getClassName(), this is 'service_container_prod_').
- The default implementation of MTimeProtectedFileStorage makes the directory the compiled class is in unreadable even to owner, so the IDE wouldn't find it or its docs.
Maybe these problems are solvable and we should continue thinking about #38. In the meantime, as an alternate idea, here's a continuation of #9 with these additions:
- Renamed PathBundle to PathServices based on #12's feedback that the word "bundle" is unintuitive here.
- Added the drupal_service() wrapper as in the issue's original patch.
- Added constants to PathServices as yet another place where docs and autocomplete can help.
Although
PathServices::getAliasManager()->getPathAlias()
isn't much shorter to type or read thandrupal_container()->get('path.alias_manager')->getPathAlias()
, it's much friendlier to autocompletion: oh, I need a service related to the Path subsystem, ok type "Path" + [TAB], autocomplete to PathServices, now I get a list of the services available in the subsystem, pick "getAliasManager", which has an @return, so now my IDE shows me what methods are available, pick "getPathAlias".Right. Most bundle classes will continue to not need to be loaded on every request. Only the ones specifically used in the procedural code flow for that request.
Comment #40
chx CreditAttribution: chx commentedStrange that #38 mentions api.drupal.org and wants to rely on the compiled class which is not accessible to api.drupal.org at all (because it's module dependent). So I do not see #38 as viable/helpful either.
Re #39, what do the constants buy us? I have no problems with them but I do not see the point either -- you do not use them outside of the class, so why bother? Otherwise, sure, the actual implementation whether it's class methods like this or not, I am fine either way. Btw. I do not know whether it's time to finally start using
@return \Drupal...
which was decided more or less in #1487760: [policy, no patch] Decide on documentation standards for namespaced items just it was never made into a patch because it'd broken all the outstanding patches and it def can wait till code freeze but a patch whose sole existence is to add @return, please add the starting backslashes.Comment #41
effulgentsia CreditAttribution: effulgentsia commentedFor #7.2.
I made up a new annotation
@service_implements
here. I don't know if that's useful or if there's a way to make it useful.Comment #42
RobLoachI'd opt to get rid of these static functions entirely as it's bad practice to reference the container from an object, let alone a static function. Usually it's better to use constructor injection, an object should just be responsible for only its dependencies.
Recommendations:
Due to the removal of those static functions, this change would end up being:
Recommendations:
Comment #43
effulgentsia CreditAttribution: effulgentsia commentedI agree with you about constructor injection for *objects*, but not for static methods. Static methods can't get anything injected. The point of these static methods is for the benefit of procedural code that we still have lots of in D8. If global functions can call drupal_container() or drupal_service(), why can't static methods? Especially considering that PathServices is not an implementation class: it's only purpose is to define services.
The problem with that is there's no way for an IDE to know what type of object is returned by drupal_service(), so you don't get any autocompletion / easy discoverability of what methods you can call on it and the signature of those methods.
I'm not saying #39 is commit worthy. Just trying to explore / communicate possibilities. Especially as an alternative to global functions like
path_alias_manager()
that can't be lazy loaded.Comment #44
RobLoachThanks for the clarification :-) . Just concerned that we'd have to put a wrapper like this in place for every service that we introduce. Would introduce a lot of code that doesn't need to be there. I definitely like the idea of moving the Path service registration over to its own Bundle, with the Container build process.
Comment #45
effulgentsia CreditAttribution: effulgentsia commentedDepends on your definition of "need to be there". It's a trade off of DX gain for developers/maintainers of Drupal's remaining procedural code vs. whatever the burden is of these extra wrappers.
Comment #46
msonnabaum CreditAttribution: msonnabaum commentedThe idea that code using drupal_container isn't unit testable is incorrect. Yes, you have to mock the container and set it globally, but it's a unit test, so it should only impact the unit of code you are testing.
To show this, I wrote a unit test for the first class I found where we were using drupal_container:
That test passes. Yes, drupal_container is global, but you can inject a mock container, so this works.
I'm sure some will find this to be "ugly" or a "bad practice", but this is testing. Whatever allows us to isolate a piece of code and test it is legitimate. We should only evaluate solutions based on what they allow or prevent us from doing.
One part of that test that is legitimately ugly is that I copied in drupal_container. I did this so I wouldnt have to include bootstrap.inc, which is an awkward thing to do in a unit test. This problem is solvable though, you just move it to a static method.
In my opinion, we had this right in D7 with the queue api:
We changed it to queue() in d8 for consistency reasons, but I think that was a regression. An autoloadable class with a Factory Method is a big step up from a procedural function in some .inc file.
I also agree with the sentiment that drupal_container introduces an unrelated concept to the calling code. The service container is not a terribly complex idea on its own, but with bundles and the different types of tags, it gets considerably more complex. I think there is value in hiding this from contrib developers who don't need to know about it. If the calling code is saying, "Hey Drupal, give me a ___", that should be reflected.
If it were me, I'd just move this to a Drupal object:
Procedural code is in the global namespace, so you don't have to "use" it or bother with \. It could essentially be identical to drupal_container now, just cleaner looking and autoloadable. If we wanted to bother, I could see an argument for moving the current wrappers to static methods on that object.
To be clear, I agree that we should avoid using the container within constructors or instance methods, but we are going to be living with procedural code for a long while yet, so we should accept it. If testing a given piece of procedural code is causing us pain, we refactor it then. Not a big deal. Refactoring should be based on pain we're feeling now, not pain we could theoretically feel at some point. Right now, drupal_container is painful.
Comment #47
webchickThe latest head-desk instance of this comes from #1887046-8: Convert drupal_http_request usage in install.core.inc to Guzzle.:
:( :( :(
Comment #48
webchickBtw, it was asserted in IRC by someone that this might be a "procedural vs. OO" generation gap problem. I want to clarify that it is not.
I don't, and I assume no one else in this issue hating on drupal_container() doesn't, have any problem with OO. OO brings us more inline with modern standards, it's what everyone who learned programming in the past ~5-6 years knows, etc. Bring on the OO!
What the problem is with #47 is this:
In any other programming language: e.g. Java, .NET, C++, etc. I can load an IDE that will give me a class hierarchy, that class hierarchy will show me all the properties/methods of that class, and when I type out references to the classes, they will get auto-completed for me, including nice docs on all the params to tell me what they are.
drupal_container()->get('magical string that's impossible to discover')
throws that completely out the window. You have no idea what you're getting back from that ->get() call, what methods/parameters it has, what parameters it takes, etc.The idea we were toying around with was something like declaring a top-level "Manager" class (or maybe "Service" class) for each of the main things you'd get from drupal_container(), so like:
Their constructors would handle the drupal_container() injector crap, and then as a developer your calls would be more like.
All of those methods/parameters would be nicely documented and nicely auto-complete-y. We still get the DIC stuff. Everyone wins?
Comment #49
effulgentsia CreditAttribution: effulgentsia commentedI'm gonna work on a couple simple patches that demonstrate #48 and related ideas.
Comment #50
moshe weitzman CreditAttribution: moshe weitzman commented#48 looks great. Others have pointed out that you don't really know which implementation of HttpClientManagerService will win and each implementation can have different methods. So you could get incomplete options in Autocomplete. Not sure that this matters.
I agree that IDE is going to be even more useful for 8 (as is a debugger) and we should architect our software with this in mind.
Comment #51
msonnabaum CreditAttribution: msonnabaum commentedTalked this over in IRC today with webchick, catch, and Crell, and we narrowed it down to a couple options.
1. Replace drupal_container with individual factory classes like #48 describes.
2. Replace drupal_container with a Drupal object that has static factories for the services we currently put in procedural wrappers, and also provide a ::get() method that allows access to the service container.
Advantages of both:
- Moving to classes is a win. They are autoloadable, so this makes it much easier to test and mock for unit tests. We don't want code that's dependent on bootstrap.inc.
- Both provide a solution to the @return issue since we'll always have a class or method to call that we know returns an object of a certain type.
Option 1:
Advantages
- Having a manager classes for each type of service is a pattern that could grow with more services added, and potentially provide a pattern for contrib to follow.
Disadvantages
- More boilerplate. We're essentially moving one function to a class with one method.
- Will require procedural code to be aware of namespaces and "use" the classes or write out really long names.
I'm not totally clear how we'd go about testing these, if they have a copy of the DIC and it can be injected or what.
Option 2:
Advantages
- No "use" statements. Procedural code is in the global namespace, so they can simply reference "Drupal".
- Less verbose, easy to read.
- Testable by injecting a mocked container object like I did with drupal_container in #46.
Disadvantages
- Sort of feels wrong
- This object could have ~20-30 methods depending on how many actual services we have
I personally favor option 2. All it's really proposing is to combine drupal_container and the procedural wrappers into an autoloaded object that we can easily test. I think it will also more friendly to contributors who mostly live in .module and provide a gentle introduction to using classes, without having to worry about namespaces. Sure, it feels a little odd to have a "Drupal" object, but it's not exactly unprecedented. Rails has a global module with Rails.loogger, Rails.cache, and Rails.config. Not that different.
Maybe someone else can defend option 1 a bit better than me.
Comment #52
webchickOne more disadvantage about option 2 is it's not extensible from contrib. Other than that, I think that's an excellent summary. Thank you so much!!
Comment #53
effulgentsia CreditAttribution: effulgentsia commentedHere's 4 options capturing some of the ideas discussed in this issue:
- global function wrappers ala #23.
- function wrappers as methods within autoloaded classes ala #39, but unlike that patch, decoupled from the Bundle classes.
- a class factory wrapper for each service (not explicitly suggested in any of the above comments, but for completeness)
- a proxy class for each service ala #48.
Comment #54
effulgentsia CreditAttribution: effulgentsia commented#53.3 and #53.4 are two variants on #51.1. Of the two, I prefer the proxy to the factory.
#53.2 is an option in between #51.1 and #51.2, and I prefer over both, because it addresses #52.
Personally, I think the two best options are #53.2 and #53.4. I think the latter is better DX for consumers, but requires more work for service authors.
Comment #55
Dave ReidAs a primarily contrib developer, I really like option #53.4.
I wonder if something like the following would be possible:
Probably not but I haven't been able to try it. According to https://bugs.php.net/bug.php?id=41162 it looks like it wouldn't work. Would have been nice to use this syntax rather than having to replicate each method.
Comment #56
tim.plunkett@Dave Reid, unfortunately, you cannot. See http://drupalcode.org/project/drupal.git/blob?f=core/modules/views/views... for how we dealt with that.
Comment #57
plachI like the extensibility provided by #53.3 and #53.4, but I think the latter puts a bit too much burden on the service provider. What about the following variant of #53.3?
Comment #58
geerlingguy CreditAttribution: geerlingguy commented[Also as primarily a contrib author] #53.4 looks right to me. Unlike other proposals I've seen in this thread (after reading through most of it twice), I can see what's going on without thinking hard, and it would offer the opportunity for documentation per method, right?
Comment #59
msonnabaum CreditAttribution: msonnabaum commentedLots of "likes/dislikes", but not a lot of reasons. Let's focus on what each option allows us or prevents us from doing.
I personally do not like any option that does not remove drupal_container. We can't have un-autoloadable functions in classes.
Comment #60
plach@msonnabaum:
Usually I wholeheartedly agree with this statement, but
drupal_container()
lives in bootstrap.inc, which I doubt will be dropped in D8, hence it will be always available. Am I missing anything?Comment #61
msonnabaum CreditAttribution: msonnabaum commentedUnit tests. No .inc should ever be available in a for-real unit test.
Comment #62
plachIn a unit test the$container
parameter could be used, if we went the #57 way :)Sorry, it's too late here ;)
Comment #63
effulgentsia CreditAttribution: effulgentsia commentedAll options in #51 and #53 are documentable and therefore, nicely integratable with IDEs.
#53.1: Pros: Direct (call a single function and get back the object you want, without needing to deal with namespaces, "use" statements, method chaining, etc.). Cons: Requires a .inc file to be loaded, violating #61 and adding code bloat to every request, even ones that don't need to call those functions.
#53.2: Pros: Extensible (each contrib project can add its own *Services classes). Organized (if you know you want something from the Path system, type
PathServices::
and your IDE shows you just path system services, not every service in core). Cons: Requires multiple "use" statements at the top of each file containing procedural code: one for each system whose services are used by that file.#51.2: A variant of #53.2 where all core service wrappers are in a single Drupal class rather than per system. Pros: No "use" statements needed. Cons: Each contrib module would require its own similar class. Questions: Would the contrib classes be global or namespaced? What about core modules: would their services go in the Drupal class or a per-module class? If the latter, it starts looking increasingly like #53.2.
#53.3/#51.1 (they're equivalent): Pros: I can't think of any relative to #53.2. Cons: Requires an extra class per service (rather than per module/subsystem), so more files/boilerplate to deal with.
#53.4: Pros: Direct (type the name of a class, then go directly to the service methods with no intermediary step (in terms of readability of the consuming code)). Cons: Similar to #53.2, requires multiple use statements, but even more of them, cause per-service rather than per-subsystem. Lots of extra boilerplate, because each service wrapper class needs to proxy all interface methods to the actual service object.
Comment #64
plach@msonnabaum:
Correct me if I'm wrong but it seems to me that in #46 you are saying that we can have (ugly) unit testing even with
drupal_container()
, right? So perhaps it would be more accurate to say that #53.3/4 make unit testing uglier rather than impossible. Btw, if a piece of code needs a service wouldn't it make more sense to test it throughDrupalUnitTestBase
and leave pure unit testing for code that can actually be tested in isolation?OTOH with #51.2 contrib services would be treated as second-class citizens, unless they went for something similar to #53.3/4. In both cases we would have an inconsistent DX between core and contrib services: they probably wouldn't even appear to be the same thing at first sight.
Comment #65
David_Rothstein CreditAttribution: David_Rothstein commentedPer-method documentation should already be possible in the original method (on the interface); these new ones would at most be documented as "Implements XYZ::ABC()".
Looking at #53.4, I don't understand how it works, though. It calls AliasManagerService::getPathAlias(), but that method isn't static... And sure enough, when I tried the patch I got this:
That seems.... bad :)
Comment #66
David_Rothstein CreditAttribution: David_Rothstein commentedSo did #53.4 actually intend for this to work more like the bottom of @webchick's post in #48? No static methods, just:
(Note that I removed "service" and "manager" from the name... whatever we do I hope we can find a way to keep those to a minimum.)
That would work, and also has the advantage that it's extremely straightforward PHP. However, we've mostly avoided "new Object()" calls in Drupal, I think because it prevents the underlying implementation from being swapped out. #53.4 is an interesting trick to allow it to look like a simple PHP object instantiation on the outside but still be possible to swap it out underneath, but at the cost of every author of a service having to write wrappers for every one of their methods? ... That seems like a little too much to expect.
#53.2, #53.3, and some of the others look OK, but string multiple methods together to achieve a single thing, which also seems more complicated than necessary. And in doing so they may allow for "interesting" collisions... Anyone really want to see
HttpClient::get()->get($url)
in the codebase? No thanks :) I agree that #53.2 is probably the best of these due to the organization:PathServices::aliasManager()->getPathAlias()
is wordy and uses lots of terminology, but at least uses it in an organized way. Although, how would it work when that much organization isn't needed? Would it beHttpServices::client()->get($url)
, even if there are no other HTTP services? (One could imagine a contrib module wanting to add something else inside the HTTP service, but it doesn't seem like they'd be able to... something to keep in mind as well.)Which brings me back to #53.1. This still seems like the simplest option and I don't really understand the objections. Yes, it adds code that will always be loaded, but even adding up all the services on a website (including both core and contrib) isn't it a tiny amount of code? Will it make a practical difference? As for unit testing, why does including a file makes something less unit-testable, especially when it already requires a file to be included in the first place (for the drupal_container() call that is made within it)? And related to what @plach said, this type of code is barely unit-testable in the first place; it's already a stretch to call it that.
I wonder if we aren't just overthinking this a bit. To me, this:
doesn't look a whole lot different from this:
Which hasn't generated many complaints or controversies in the years we've been using it...
Comment #67
msonnabaum CreditAttribution: msonnabaum commentedOn unit testability:
If a unit test requires bootstrap.inc file with 100+ functions defined in it so it can get one function it needs, that's not really a unit test. That function cannot be autoloaded, so every single unit test file you run has to have that bootstrap.inc file loaded. Your class could be calling out to any of the functions in that file, and they wouldn't fail, because that code is loaded implicitely. That is not a unit test.
If a class makes a call to Drupal::get('whatever'), yes, that is a dependency, but we can autoload *only* that dependency to perform the test. If application code that is *not* a dependency is loaded, that's not a unit test.
We've made this shift into breaking up our codebase into classes, and added a significant amount of complexity by adding a service container, but we do that in the name of flexibility and unit testability. If we aren't going to value true isolation and testability and we're comfortable with using DrupalUnitTestBase, I dont know wtf we're even doing with a service container. Why add all this complexity if we aren't going to reap any of the benefits?
Whatever option we choose, there is no sane argument for not at least moving drupal_container to a static method on a class.
Also, we should not avoid static methods on principle. It's a very common practice to use them as factory methods. Please don't make me instantiate a manager class so I can then call a method on it to get an instance of the object I want. That's a DX regression.
Comment #68
David_Rothstein CreditAttribution: David_Rothstein commentedAs far as I can tell, all the options discussed above (with the possible exception of #57) require bootstrap.inc to be loaded already...
Unit testability is good in general, but what we're talking about here are basically very dumb wrappers. You can already unit test the (real) service that lives underneath.
Comment #69
Dries CreditAttribution: Dries commentedI think we should explore using class loaders for locating complete services (and keeping dependency injection for more narrow configurability).
Comment #70
effulgentsia CreditAttribution: effulgentsia commented#69 is inspired by Java. I'll work up a patch to demonstrate what it would look like in PHP/Drupal.
Comment #71
Crell CreditAttribution: Crell commentedI second pretty much everything @msonnabaum has said from comment #51 onward.
We're this committed to OO and injection, let's not kneecap ourselves needlessly. drupal_container()->get('foo') vs. Drupal::container()->get('foo') are effectively identical from a DX perspective and a hard-dependency perspective, except that the latter (Drupal::) can live in its own autoloading file while drupal_container() cannot. Moving to the latter is a no-brainer for me.
While I am generally opposed to static methods on the grounds that they are no less of a hard-dependency than a floating function (and thus all the DI-based arguments against floating functions still apply), in this particular case we're talking about something that is a hard-dependency by design; or rather, it's a tool to be used in places (e.g., existing procedural code) where doing clean DI is prohibitively complicated anyway. So using
$http_client = new HttpClientManagerService(); $http_client->get($uri);
overHttpClientManagerService::getInstance()->get($uri);
offers no advantage at all aside from adhering to a level of pedantry even beyond my standards (which is saying quite a bit).In all of @effulgentsia's sample patches, we're still using drupal_container(). Thus, we still need to manually load bootstrap.inc, which we want to avoid. The only way to eliminate drupal_container() entirely is to move it to a Drupal::container() static method, which we should do regardless of whatever else we do here for reasons already stated.
Once we have that done, though, I see no compelling argument to NOT move our most common wrapper functions to static methods. cache() vs. Drupal::cache(), again, offers really no notable difference in DX or hard-dependency except that the latter lazy loads and the former does not. Actually, that's not true. Drupal::cache() means that you can type Drupal:: [wait for IDE to list out the wrapper utility methods on that class], which is more discoverable than a bunch of free-floating functions.
In turn, that leaves the only open question as whether we put "common" utility wrappers on the Drupal class or not. There's no way we can *stop* module developers from writing their own wrapper classes that do:
So really, it's just a question of what core does. And, IMO, the discoverability of Drupal:: [wait for the IDE to list common services] is better than trying to remember 20 different class names, which is no better than remembering 20 different function names.
The counter argument is that contrib modules cannot then add their own methods to that Drupal:: global class, which is completely true. However, I suspect very very few contrib modules will be defining their own services that are common enough that they should have a dedicated wrapper in the first place. The preferred approach should still be doing for-reals injection whenever possible. For cases where that's not feasible, Drupal::container('service') will work everywhere. If you're Views, Panels, Rules, or that sort of system, then OK, maybe you'll define your own Rules:: static class with a half-dozen utility factories a la the sample above. I think the number of modules that have a legitimate cause to do that is fairly small.
[Edit: Fixed "duh" issue with the code sample.]
Comment #72
David_Rothstein CreditAttribution: David_Rothstein commentedThat makes a lot of sense to me.
So to summarize with some actual examples, we'd change things like this?
A little unclear on which namespace "Drupal" and "Edit" would live in, but regardless this looks good.
Comment #73
msonnabaum CreditAttribution: msonnabaum commentedThose all look more or less correct to me.
Drupal would actually live in the global namespace, so just
Drupal
in procedural code, and\Drupal
in classes where not having the injected container isnt *too* bad (factory methods for example).Core modules are a bit less clear. I think probably the simplest for that would be
\Drupal\Edit::editorSelector()
, which doesn't benefit from being namespace-less, but I think these will get used much less in contrib anyhow.And for the container access, I kinda prefer
Drupal::get('some.service')->someMethod()
. I think hiding the concept of the container in this case is a good thing. Also, I wouldnt have aset
method here, because this object shouldnt be treated like its a container, it's just a wrapper that obscures the container.Comment #74
chx CreditAttribution: chx commentedA consensus is forming so I took #72 and made an issue summary out of it.
Comment #74.0
Crell CreditAttribution: Crell commentedWrote an issue summary.
Comment #74.1
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #75
Crell CreditAttribution: Crell commentedTweaked the summary language slightly, and added an option for Drupal::service('service.name') instead of Drupal::container()->get('service.name'). Given the rest of the class, I somewhat prefer the former.
Any dissenters to #72 / the current summary?
Comment #76
BerdirI'm not sure if \Drupal can be autoloaded through a PSR-0 classloader and I'm quite sure that \Drupal\Edit can not be. It would have to be \Drupal\edit\Edit?
And I *think* \Drupal is, even if we can make it load (or load it hardcoded but that would mean it could just as well be a function in bootstrap.inc ;)), not PSR-0 conform:
I'm not sure if \Drupal, even if it is equal the Vendor Name, is a valid exception?
So given that modules would have \Drupal\edit\EditBundle and optionally a \Drupal\edit\Edit, would't \Drupal\Core\Core be the most logical companion for \Drupal\Core\CoreBundle?
Edit: Well, not quite, as everything would be available through Core::service()...
Comment #77
msonnabaum CreditAttribution: msonnabaum commentedHonestly, I don't care about what PSR-0 says regarding the vendor name being in the namespace. We're Drupal, that's our namespace and our vendor, we're not polluting the global namespace by making a \Drupal class.
I haven't tested it (and you actually can't with the way our autoloader is setup because we register Drupal/Core and Drupal/Component instead of registering Drupal), but a PSR-0 autoloader should handle that just fine.
So something like \Drupal\Edit would actually work just fine, it'd just map to core/modules/edit/lib/Drupal/Edit.php. Sure, that's a bit awkward, but so was our decision to keep module namespaces lower.
Comment #78
jhodgdonRegarding #72/current proposal... Would a contrib module be able to override the class providing the service? How would they do that, if so? Or is that staying the same as it was?
Comment #79
tim.plunkettThey wouldn't need to (and couldn't) override the class providing the service, they would just swap out the service.
Comment #80
Crell CreditAttribution: Crell commentedjhodgdon: What Tim said. The wrapper static class is no more swappable than a function, but doesn't have to be. It is *not* the class that is the service being offered through the Container. (If you try to combine the two I will come after you with a foam machete.) It's just a shorter syntax utility.
The Symfony ClassLoader supports non-namespaced PEAR-style classes, too, which is how we can autoload Twig. When/if we move to the Compose autoloader, there's ways to configure that, too. Getting a non-namespaced Drupal class to autoload is a solvable implementation detail.
Comment #81
plachGiven this, the current proposal would work for me. Perhaps I'd prefer to avoid the
::service()
method to encourage contrib modules to expose their own wrappers instead of being lazy and make everyone rely on it. This would make easier to have all the good stuff we discussed here available also in contrib.I'd also leave the possibililty to pass a container instance, as suggested in #57, so that in contexts where we are able to inject the container we could still leverage the wrappers and avoid hardcoding the string service identifiers, besides having autocompletion, etc.
Comment #82
Crell CreditAttribution: Crell commentedI would actually want to retain ::service() exactly to discourage contrib maintainers from writing their own wrappers, as we want to be nudging everyone toward properly injected services in the first place. This is all about BC shims and crutches as we transition to clean injection. :-)
Comment #83
natted CreditAttribution: natted commentedA few weeks ago I began to follow this thread. Somewhere (I don't recall the specific example), I had seen
drupal_container()
. It immediately made me cringe just from DX perspective, so I googled and found this thread.Since then, I've just been lurking to see what may come of it.
I can't add much except that I like the proposed resolution, particularly with
::service
suggested by Crell.Comment #84
sunThe proposed solution in the issue summary looks great to me (which seems to be identical to #72, for posterity, in case the summary is changed).
That said, I agree with others that the class autoloading for the thus far suggested class names presents a technical/architectural problem, and I'm fairly sure that any kind of suggestion to replace the class loader with a custom one is a straight won't fix, since I originally suggested a minimally customized class loader for Drupal's needs in the past already.
Specifically:
Drupal
can't be a class, unless it'sDrupal\Drupal
, which is the outermost that we're able to autoload in PSR-0. Anything outer than that means to manually include the class in procedural code (or to customize the autoloader) — the first option has been expressively denied in here already, the second is just a deeper variant of it (since our class loader isdrupal_classloader()
, which lives in bootstrap.inc). The argument that some alternative class loader might support this edge-case doesn't really change the fact that you have to manually register this edge-case for every single unit test that should be decoupled - which in turn is the whole point of turningdrupal_container()->get()
intoDrupal::service()
instead ofdrupal_service()
. The only difference between the two would be aninclude_once
vs.spl_autoload_register('wtf')
.Edit::whatever()
is great, but at the same time, also highly inconsistent and prone to errors: #1550680: Automatically convert PSR-0 namespaces for extensions into upper CamelCase was essentially marked won't fix (even though the issue is still open), because we did not want to get into the business of converting extension names back and forth from under_score to camelCase. Yes, that issue is about namespace roots, but if you fully think through the proposal ofEdit::whatever()
, you'll arrive at the conclusion that it hits the same problem space.Drupal\Core\Core::whatever()
(nicely corresponding toCoreBundle
) andDrupal\edit\Edit::whatever()
(nicely corresponding toEditBundle
).Drupal\[Provider]\[Name]
into my file, but that's most certainly not the onlyuse
statement that will exist in files, so "trying to avoid that" is a misleading/bogus goal in the first place, IMHO.It feels utterly wrong that we need to work around the DI container in order to regain DX quality - by slapping even more classes that need to be autoloaded onto the system. You know, instead of having a DI container architecture that Just Works™. However, sleeping over this for a few days, the proposed solution is fine and will work for now.
Thus, +1 to the proposed solution in the summary, in combination with the reality-facing technical solution in #76.
Comment #85
chx CreditAttribution: chx commentedWhile
Drupal\edit\Edit::whatever()
sounds good because after ause
it becomes justEdit::whatever()
, I would like to suggestDrupal\Core\Drupal::whatever()
so that it looksDrupal::whatever()
after ause
.Comment #86
Berdir\Drupal\Core\Drupal
sounds fine to me as well, given that it not only exposes explicitly defined services as Drupal::something() that map to something defined in CoreBundle but all defined services as Drupal::service('something').I think @sun very nicely explained my concerns from #76.
Comment #87
msonnabaum CreditAttribution: msonnabaum commentedOur autoloader totally supports Drupal as a class.
Just change:
$prefixes = array();
to$prefixes = array('Drupal' => DRUPAL_ROOT . '/core/lib');
in drupal_classloader().Comment #88
Crell CreditAttribution: Crell commentedSo something like this.
The attached patch adds a global Drupal class, uses it in place of drupal_container() in a few places, and turns drupal_container() into a dumb wrapper for it. I only converted one or two spots to use the class directly in place of the function because there's, oh, 436 of them according to NetBeans and I figured I wouldn't rob the Novice queue of that many easy patches. :-)
I added dedicated wrappers for only two services: database and cache. The cache one needs to be simplified. (Ping catch!) I would rather not bikeshed which other services we should give dedicated methods to right now. Let's just get the framework in place and we can then discuss what else "deserves" to be there piecemeal in parallel.
Comment #89
cweagans+1000 to this. I will personally commit to helping people file patches to add other services to this during Core Mentoring Hours.
Comment #90
cweagansYou know, I'm just going to RTBC this. The patch is beautiful, is a DX improvement, and I really like it. I can't imagine this will fail testing, but if it does, the bot can still set it back to CNW.
Comment #91
BerdirStill would like to know if that negatively impacts performance. Probably depends in which order these things are checked.
I would say this should be database()
cache() is IMHO a bad example because it's not actually in the container yet, just "routed" through it.
Also, would like to see an *example* of such a Class provided by a module, I think that makes sense to do here as there were quite some discussions about naming/namespace and I'd like to see that resolved/proved before we can open novice issues.
Comment #92
plachPushed a couple of fixes:
This should be "Contains \Drupal." but I think it deserves an exception :)
Given the amount of times this will probably be called I'd save a method invocation and directly access
static::$container
.Comment #93
Crell CreditAttribution: Crell commentedNew patch, includes plach's fixes as well as directly calling $container in the db() method. (I have no strong feeling about db() vs. database(), so I'll go with consensus on that.)
There's a
patchbranch in the wscci sandbox for those who want to play along at home.Committers, please be sure to credit plach as well. (The auto-commit message on this issue is wrong, since there's been so much discussion before a for-reals patch.)
Comment #95
Crell CreditAttribution: Crell commented#93: 1875086-drupal-container.patch queued for re-testing.
Comment #96
msonnabaum CreditAttribution: msonnabaum commentedI really like this for the most part, it's exactly what I had in mind.
I think we should change db to database, especially considering that's what we called the service.
I also agree that cache is a bad one to start with because it calls procedural functions. We should start with something simpler like lock, which is just a passthrough to the container.
Comment #97
Crell CreditAttribution: Crell commentedOK, renamed db() to database(), dropped the cache utility method, added a lock utility method. Again updated variable_initialize() to demonstrate the syntax change.
Comment #99
plachLet's try this one. Didn't push this since the latest changes from #97 are missing on the wscci repository.
Comment #100
RobLoachAlthough
\Drupal
as a class does work, it is better to use an explicit namespace likeDrupal\Core\Drupal
. This is because if we register theDrupal\Core
andDrupal\Component
as separate namespaces, classes likeDrupal\views\ViewsAwesomeClass
end up not having to be checked for in core/lib. I was putting in some work towards that split at #1658720: Use ClassLoader instead of UniversalClassLoader, but that patch needs an update. And yes, benchmarks were run.Recommendations:
Drupal\Core\Drupal
as suggested in #85 and #86 by both chx and Berdir.Comment #101
RobLoachTagging.
Comment #102
msonnabaum CreditAttribution: msonnabaum commentedRob, if you look at how its registered, it's registered as a prefix, not a namespace, so the performance penalty you're suggesting won't actually happen.
Comment #103
msonnabaum CreditAttribution: msonnabaum commentedAlso, let's not make any design decisions based on the current performance characteristics of our class loader. We can make it load however we want, so if there's an issue, we can fix it.
I can't imagine we'll ship d8 without supporting classmaps anyways, so let's just assume O(1) here.
Comment #104
plachIf we were to change the current approach,
Drupal\Core\Service
,Service::lock()
andService::get()
would look more self-documenting to me.Comment #105
jhodgdonPlease... We have standards on naming classes:
http://drupal.org/node/608152
So...
"Classes and interfaces should have names that stand alone to tell what they do without having to refer to the namespace, read well, and are as short as possible without losing functionality information or leading to ambiguity."
So don't name the class "Service". That is so vague as to be totally useless.
Also:
"Names should not include "Drupal"."
We have about ... well, nearly all of the classes in Drupal 8, I would venture to say, violate these standards, and there's an effort afoot to fix this:
#1809930: [META] Many core class names violate naming standards
Please don't make it worse by adding a non-compliant name here.
Comment #106
plachThanks for brigning this on table :)
IMO the class we are talking about here is a very special case and as such could deserve an exception (I'm not saying it necessarily should), anyway I think that code like:
is very readable and sef-documenting and I strongly disagree that it is so vague as to be totally useless. Moreover one of the pros of this class name is being short, which makes using it in environments where autocomplete is not available easier.
Comment #107
jhodgdonIt's not clear to me, without knowing about the drupal container from before, what "Service" means... What would the first line of the class documentation say? That might help us choose a better name. I mean, previously this was handled by something called drupal_container(), and if that was a good name, how is Service also a good name for (as far as I can tell) pretty much the same thing?
Comment #108
tim.plunkettOur usage of drupal_container() follows the Service locator pattern. The things we are get()-ing from drupal_container() are services. I think we could easily document our way to making Service the best option.
Comment #109
chx CreditAttribution: chx commentedI am fine with \Drupal, when I suggest something else that was on the grounds that the autoloader doesn't support it.
Comment #110
plachWe are currently using the object returned by
drupal_container()
as service container. TheDrupal
class holds a singleton instance of the service container, that is the same object currently returned bydrupal_container()
. Hence theDrupal
object has a slightly different role. The static methods on it are the equivalents of the various non-autloadablecache()
,lock()
, ecc. procedural wrappers. The class in itself is kind of namespacing these methods more than providing something on its own.If we were to provide a fully tale-telling name we should go for something like:
which would be very verbose and actually less readable to my eyes (the signal to noise ratio is very low IMO).
Comment #111
cweagans+1 for just \Drupal. Less keystrokes and it reads very nicely.
Comment #112
plachJust to clarify: I was suggesting
Service
only in the case we'd go for a properly namespaced class:Drupal\Core\Drupal
is fairly ugly to my eyes :)Comment #113
effulgentsia CreditAttribution: effulgentsia commentedIt would be great for the next patch here to include an example of the issue summary's proposed resolution items 2 (a service provided by a module) and 3 (a rarely needed service).
For what's already in the patch, I agree with just \Drupal, because per #46, the primary callers this is intended for is procedural code in the global namespace, so that code can just write
Drupal::
without needing ause
statement anywhere. There should be very few places where namespaced code needs to do anything on theDrupal
class: the patch includes a usage from DrupalKernel and TestBase, and ideally, those should be the only two places where that's needed.Comment #114
Crell CreditAttribution: Crell commentedNew patch:
- Incorporates plach's fix above. (Ugh, Crell-- for such a dumb mistake.)
- Converts all usage of drupal_container()->get('flood') to the new class, including in FloodTest.
Additional notes:
- I officially don't care if we call the class Drupal, Service, ServiceContainer, or whatever. Let me know when we come to a conclusion.
- I do care about sticking the class into a namespace. This issue is about improving DX of procedural code that needs a container-provided service. Procedural code is virtually all (I think 100% all, at the moment) globally namespaced. That makes any namespacing on this class a DX loss.
- I agree with Mark that worrying about autoload order here is putting the cart before the horse. There's so many other ways we can optimize that later it's not even funny. Not topical for this issue.
- FloodTest is a horrid test class. There's no reason at all for it to be a WebTest. It should be one or more unit tests, probably not even DUTB. File that as one of the things we need to clean up later, which having the flood system as a proper injected service would enable. :-)
Code has been rebased against 8.x and repushed to the WSCCI sandbox.
Comment #116
plachI'm picking this just because we are somehow setting a standard that will be reused through out all Drupal code: in the lock hunk you are using just
$lock
. For consistency I guess we should either have$lock_manager
there or$flood
here. I'd prefer the short version.Comment #117
catchBumping this to critical, mainly because I wince every time I see issues like #1916134: Remove module_* deprecated functions and we probably want to mark those postponed on this.
Comment #118
Crell CreditAttribution: Crell commentedI don't particularly care which variable name convention we use here.
Comment #119
plachThis should pass tests now and address #113:
Not sure why I had to fix the sitewide contact tests, maybe it was just a local failure. Anyway, the fix doesn't change the test logic and is pretty harmless.
Comment #120
Crell CreditAttribution: Crell commentedErp. No. This is exactly what we're trying to eliminate. No function wrappers. They can't autoload.
Comment #121
plachYep, this was kind of a provocative patch :)
I choose the field synchronizer service because, besides the fact that I know it, IMHO it exemplifies well the risk of discouraging module-provided autoloadable wrappers: in this case we have a service that is originally used only in a hook implementation, hence a place that is not a good candidate for unit testing (I guess we'd need at least a DUTB-based test here). Someone not particularly familiar with DI might think that providing a procedural wrapper is the proper way to encapsulate the service name string and make autocompletion possible. If this happens, other non-OO but more unit-testable pieces of code might wrongly leverage the procedural wrapper.
I was wondering whether the proposal in the OP foresees namespaced wrappers or classes declared in the global namespace as for
Drupal
. In the latter case should we declare a prefix for each module'slib
directory?Comment #122
Crell CreditAttribution: Crell commentedQuite frankly I think that's grossly over-thinking things. We're talking about a crutch for code that hasn't been converted to be injected/injectable. This is not a first-class system, it's a hack. We're just trying to make it a somewhat less hacky hack. Standardized namespace locations for extensions to a hack is an inherently backward concept.
To be blunt, if a contrib module throws in a function wrapper rather than a class wrapper and hurts its own code, that's not my problem. I care that core sets a good standard, which is that most of the time we DO cleanly inject things, and for those cases we can't we use Drupal::. Don't over-optimize an edge case.
CNW because #119 is not commitable as long as that 3rd commit is in there.
Comment #123
plachHere is how things should work according to my understanding of #122.
Comment #124
msonnabaum CreditAttribution: msonnabaum commentedHere's the version that Alex asked for that implements 2).
It works and there are no performance or autoloader issues here, but it's definitely a step more dirty than the \Drupal class. If you look at the changes in overlay.module however, I think it's a pretty big DX win.
One thing to consider here is just how few services are in modules. I started with the example of Edit::editorSelector(), but really, that's not much of a service at all. It looks like that was only added so that we didn't have to hardcode the classname when registering edit.metadata.generator. I can't imagine swapping that class out…
I had to look around quite a bit to find a module provided service, and UserData is the best I came up with. It's a good example as it's called in a few .module's, but this is hardly an analog to contrib considering user is a required module.
So if people like what's in this patch, I am ok with it, but I'd almost prefer to punt on it altogether and just use Drupal::service for modules. I don't think it'll be a big enough issue to justify the additional class.
Comment #125
plachPersonally I definitely like what is in this patch because of its obvious DX improvements.
OTOH I understand the argument that module-provided services might be so rare that bothering with providing autoloadable wrappers for procedural code would be overkill. Moreover a possible DX regression is class name clashes: if Node provides a service then we'd have a
node
class in the global namespace that would clash with\Drupal\node\Plugin\Core\Entity\Node
, thus preventing us from cleanly usinguse
(pun non intended :).Anyway if there is no performance penalty I'd say we could allow for this by registering module prefixes and let module authors choose on a case-by-case basis.
One thing I originally didn't consider is that this solution makes dependencies explicit: if module A defines a service S (implemented by Sa) that is used by module B and module C provides an alternative implementation Sc, B still depends on A because of the S interface. Not sure whether this is a big pro.
Minor thing: if we decide to go with module-provided wrappers, I'd use the camelized version of the module name for the wrapper's class name for consistency with the module bundle class.
Comment #126
plachAnother minor thing: if we go this way, I'd extend the
Drupal
class so we can directly accessstatic::$container
and save a method invocation.Comment #127
BerdirWe can argue whether it's valid to have Drupal as a top level non-namespaces class but I'm pretty sure we don't own the top level namespace for every of the thousands of contrib and custom modules out there (even if they don't use it, this would still register the prefix).
So we'd at least have to do Drupal\user and if we do that, we can also just do Drupal\user\User in the default namespace, we'd have to add a use anyway and we're going to have lots of those, so what matters if we have one more? I really don't understand this...
The possible namespace clashes outlined in #125 are another valid argument. overlay_user_update() should actually type hint $account with User and then we'd have a problem with \user. In fact, we also have a problem when it's \Drupal\user\User, not sure what to do about that. "$moduleService(s)" ?
Agreed that this looks better.
I think it's important to understand that this is *not* in all cases the wrong way to access a service as the added @deprecated on the Drupal class seems to suggest. There are a lot of cases where there's no alternative, all hook implementations for example.
I think we can only add a @deprecated when we there is a better alternative for every possible use case. Otherwise, we could just as well add a @deprecated to every single function/class we have as it might get removed/changed in 9.x :)
@plach in #125: It can only be a camlized class name if it's within the Drupal\user namespace, for the same reason we made the module namespace not camelized (contrib module name classes, some_name vs. somename, which do exist)
I also disagree with #126, doing an (u/U)user extends Drupal would also mean method autocomplete in IDE's would suggest things like user::database() or user::lock(). Drupal should in fact probably be final?
Comment #128
plachI fully agree with this statement, it's one of the main reasons I was advocating for having also module-provided wrappers. We probably need a better name convention as shown in #127.
Makes sense. But since it seems we'd better namespace the wrapper class my suggestion still stands :)
Yes, this is a drawback. I think that writing
User::database()
wouldn't make any sense, but conceptually wouldn't be wrong because we are still dealing with a service container wrapper. Anyway, I'm not married with this idea, especially given that contrib-provided services aren't likely to be called as often as core ones, hence I don't really care if it's accepted :)Comment #129
plach@effulgentsia:
Any comment on the latest patches?
Comment #130
webchickNote that effulgentsia is on vacation for the next 2 weeks, so it's probably good not to hold things up on his feedback if possible. :)
Comment #131
RobLoachOverall, I think this is a step forward. Although it doesn't really gain us much out of the box, I do find it adds some nice documentation and helpful links and brain queues for developers reading through it. I have a few notes below. [EDIT] Note that I haven't read all the backlog, this is just a review and thoughts on the latest patch.
I like this design, just concerned with what happens when Drupal modules introduce their own services. Modules won't be able to add functions to the Drupal class, which runs us into the same issue that we had before. I can understand why though. Either way, I think it's a step forward.
See also: #1658720: Use ClassLoader instead of UniversalClassLoader ... That allows the namespace definitions to be in
composer.json
rather than handled indrupal_classloader()
itself.I like to see the @deprecated tag on global PHP functions. Would love to deprecated more of our global functions in the future.
It has been mentioned before, but using
\Drupal
as both a class name and a namespace limits what we can accomplish with the class loader.Drupal\Core\Drupal
gives us a bit more flexibility. See the RTBC #1658720: Use ClassLoader instead of UniversalClassLoader for details on that.We might be able to make Drupal inherit from Symfony/Component/DependencyInjection/ContainerAware. Would have to turn it into a singleton, however. Probably not be worth it.
Again, my only concern about this is what happens when contributed modules want to introduce new Drupal::myservice() methods. We could introduce __callStatic() use, but that can be quite the performance hit. This puts us at
Drupal::service('myservice')
for contributed modules, which does seem a bit better.This is a change in the design of how a module's
lib
directory works. Should this be a separate issue?Ah, now I understand what you're trying. Instead of Drupal::service('user'), we'd use a static class registered at modules/user/lib/user.php for it. Couldn't that be
Drupal\user\User
instead?I understand it's desired to keep this simple for newbies, but this is Drupal Core we're talking about. We're suppose to be a good example of how things should be done.
Yeah, that should probably just be
Drupal\user\User
. Some documentation would be good too.Comment #132
Crell CreditAttribution: Crell commentedRob: To the main points you mention, they've already been discussed above. :-)
A) There's two general factions on module-supplied services: 1) We should automate a similar static class for modules to expose their services with type hinting. 2) Only about 3 contrib modules will have services significant enough to bother with, and they should be doing real injection anyway; this is just for legacy code. They can make their own class if they want, don't coddle what is essentially a BC crutch. (You can probably guess which camp I'm in. :-) )
B) The global namespace for both the Drupal class and, if supported, any contrib classes is to avoid the need to "use". These classes should only ever be used in procedural code in the first place, and our procedural code is all in the global namespace. It's not good for DX to force a procedural routine to "use" and then 400 lines later mention the class, when in this special case we can make it global and be done with it.
Comment #133
plachWhat about going the "Crell way" (core-only globally-namespaced service container wrapper) for now and introduce module-provided wrappers later, if we should find that the legitimate cases are a big enough number to encourage this solution as a best practice? We can rehash the "naming patterns" and "global vs local namspace" discussions in that case.
At least we would improve DX for people working on core right now and we could go ahead discussing which services we actually need on the core wrapper.
Comment #134
sunYes. Let's move forward with a helper class for Drupal core services only for now. We can revisit the situation for modules later.
Comment #135
Crell CreditAttribution: Crell commented#133++
Comment #136
plachIf there is consensus on #133 then #123 is the patch to review/RTBC.
Comment #137
msonnabaum CreditAttribution: msonnabaum commentedAs expressed in #125, my preference is also to not deal with modules for now, so I'm in favor of #123.
Comment #138
Crell CreditAttribution: Crell commented#123: drupal-container_get-1875086-123.patch queued for re-testing.
Comment #139
plachTangentially related: #1935762: Remove entity_load_unchanged() call from FieldTranslationSynchronizer::synchronizeFields() (discussing dependency injection DX for entities there).
Comment #140
Crell CreditAttribution: Crell commentedSomeone want to RTBC #123 then? (I don't think I can, since I wrote too much of it.)
Comment #141
tim.plunkettI'll bite. The only big difference between #123 and #124 was the user::userData() addition, which it seems needs further discussion.
So in that case, let's go with #123, and open a follow-up for discussing how modules should utilize this new pattern.
Comment #142
David_Rothstein CreditAttribution: David_Rothstein commentedThere was this (from #127):
But I'm not changing the status because I don't have time to rewrite the docs right now. However, I agree the @deprecated should be removed, and instead I think the docs should focus on when vs. when not to use this class (including basic examples showing the dependency injection alternative).
Also think
Drupal::service('flood')
should becomeDrupal::flood()
but that's not too important now since there will be followup issues to do the full set of conversions anyway.Comment #143
plachDone.
We agreed above that we should evaluate whether services deserve a dedicated method on case by case basis. Flood is a rarely used service and as such was deemed not worth a dedicated method.
Comment #144
quicksketchI didn't realize that this had been agreed upon until you described it in this way. The entire point of this issue is DX and discoverability of services. I find it strange that we would provide some services but not make them easily discoverable based on how frequently we think they will be used. For the case of all core APIs (i.e. those previously provided by anything in /includes) all of them should have methods. If I missed an earlier consensus where this had been stated, my apologies for missing it and bringing it up at this point.
Comment #145
Crell CreditAttribution: Crell commentedquicksketch: There are going to be hundreds of services, likely. Most of them will be rarely if ever used. Making an extra method for every single one of them is a wasted effort, especially when we're talking about something that is a BC crutch, not a primary API. It also sets the wrong expectation. Flood, for instance, is used twice in core, and the odds of contrib code using it are pretty small. It's not worth adding a method there. Database or cache, however, are used all over the place so for them it makes sense to expose a type-hintable BC method.
Which services "qualify" as big enough to warrant their own wrapper is something we should be determining case-by-case later on.
Comment #146
webchickWhat exactly is the reason making a wrapper is wasted effort..? It may be a BC crutch indeed, but we'll be stuck with it for the next 5 years, so what's the harm in making it consistent?
Comment #147
quicksketchPerhaps I'm still dreaming of the days when I could "guess" at an function/method name and find it easily through auto-complete. The previously procedural equivalent of these services had not hundreds, but thousands of functions in the global namespace, yet finding the one you wanted was still easy to guess in most cases. However I see the difference in that each one of these wrappers has to be manually created, as opposed to the procedural version where each function was the real thing, not a wrapper to something else. It still seems like we're going to be leading developers into a confusing scenario where they'll probably discover all of the Drupal::* services first and more easily. Then later they find that, oh surprise: we have another hundred services that we didn't tell you about earlier, and you have to call them differently. It doesn't seem ideal.
Even with all that, the current patch is a big step forward in DX. If we can start with that but leave the consideration open on possibly adding all services later, I'm more than satisfied. It seems we have to decide which services are important enough anyway at some point, so this discussion will probably have its own issue anyway. I just don't want that new issue to have someone pointing at this one and saying, "we already made that decision".
Comment #148
Crell CreditAttribution: Crell commentedquicksketch: Remember that every single access checker, every route enhancer, the matcher dumper itself, every single event subscriber, etc. will all be registered as services on the container. Yet there's no reason anyone sane would want to access those directly from a random hook. (There's plenty of insane people in Drupal, myself included, but all of those services fall into the "if you have to ask you probably shouldn't touch" category for 99% of developers.)
So "wrap all the things" doesn't even make sense, since there's an awful lot of services that aren't even entry points module developers should know or care about in the first place.
We've said before in this thread and elsewhere that we need to develop a "list public services" operation of some kind. The container supports it, Symfony fullstack has code we can likely steal, someone just needs to do it. But that's not relevant to this thread, which is just a modest DX and structural upgrade to the legacy bridge.
I looked over #143, and it looks good to me based on the discussion to date. I know I said before I wasn't going to RTBC, but I haven't posted a patch in about 6 patches so I think I'm far enough removed now. :-)
Comment #149
plach@quicksketch:
Sorry if I gave the impression of dismissing other people's concerns: fact is that bullet #3 of the proposed solution in the issue summary is exactly what I reported as what we agreed upon. Since the latest patches were implementing it and no one was complaining about that, I thought we had an agreement.
FWIW I think that every non module-provided actual service should have a wrapper in the Drupal class, which leaves out the tons of stuff Crell is referring to in #148, but includes the Flood service. However I'm totally ok with discussing this elsewhere.
Comment #150
plachI was having another look to the patch and I noticed that in a couple of places we are going against the best practices we are trying to promote:
This could use a @todo as the flood service should be injected.
We should be doing
$this->container->get('flood')
here.If we agree I can do a quick reroll.
Comment #151
msonnabaum CreditAttribution: msonnabaum commentedI wouldn't bother, the issue is unrelated to this one, which is already hard enough to follow.
Comment #152
sunHm. I'd recommend to use this rule of thumb instead: Every service component in Drupal\Core\* should have a dedicated method.
That would be simple grasp and follow. As of now, with a quick run-down on the directory names, we'd end up with ~these methods:
cache(), config(), database(), module(), flood(), keyvalue(), lock(), mail(), password(), path(), queue(), transliteration()
Apparently, that pretty much maps to all the procedural helper/wrapper functions we have right now.
Comment #153
quicksketch+1 I'd like to see all general-purpose services (even obscure ones) to be available in this way.
Comment #154
Crell CreditAttribution: Crell commented"Every service in \Drupal\Core" includes:
- 11 pieces of the config system.
- 2 database connections.
- 2 queue systems.
- 2 key-value systems.
- Settings
- State
- path alias manager
- path alias manager cached version (the only version you should be using)
- path crud
- Some ill-defined set of cache bins
- The request itself
- Guzzle
- Multiple caches
- The kernel
- Multiple typed data-related things I don't even understand
- EFQ
- router dumper
- router builder
- About 15 event subscribers
- Our trivial content negotiation library
- The exception controller
- Transliteration
- Flood control
- Module handler
- 10 different services relating to routing
- Twig
- 4 inbound path processors
And that's not even all of CoreBundle. There's about 6-8 of those that anyone outside of \Drupal\Core has any reason to access directly.
No, we're not going to make 62 wrappers "on principle" when people shouldn't even be touching more than 10 of those, and likely won't need to touch more than 6, and we're talking only about the legacy bridge.
We're going to have to exercise good contextually-relevant judgment. I'm sure we can manage that.
Comment #155
BerdirAgreed with Crell that it's pointless to add a method for everything that is registered in the container. However, I think it's the initial definition (often used services) that caused the confusion. We shouldn't decide based on how often something is used but if it's a public service or not.
As Crell mentioned, the Symfony container has the notion of a service being public or private (http://symfony.com/doc/2.0/components/dependency_injection/advanced.html) but making something private means that it's impossible to access it from the outside at all (it is inlined in the dumped container), so that probably goes too far in many cases.
Comment #156
catchI'd have a method for flood. It's true that it's not used much in either core or contrib, but if it was used more we'd have more protection against DDOS/brute force than we do. Same as the lock framework's not used much but I can't remember the last time I worked on a site that hasn't suffered from a nasty race condition in contrib or core code.
A lot of this can be handled in follow-up issues though, happy with this patch but since there's still discussion happening here not committing yet.
Comment #157
xjmOverall I think this is a great step forward, both for code readability and for DX.
Drupal::service()
explains to me what the heck the code is doing in a way thatdrupal_container()->get()
never did.lol.
Nitpick: should start with a verb.
The first two sentences here could use some work. It took me two reads to realize what "constructor injection or setter method injection" meant (
__construct($dependency)
,Thing::set($dependency)
). Also, the "particularly in legacy procedural code is sort of snide. ;) We don't need the class description to be a soap box.This example is very helpful, thanks.
Is this comparison still relevant? Both of these things are new to D8 (so "legacy" doesn't make sense to me), and if we're not going to use the first pattern, then I see no point in including it here. (Unless we think we won't get the existing uses cleaned up before release? I hope not, since those are essentially string replace patches plus defining methods, and noviceable.)
If I'm not confused, we should simply remove the first hunk and keep the "correct procedural code."
Leaving at RTBC, since my comments can be handled in followups if appropriate. Once this is in, we can create some followup issues to convert specific public services.
Comment #157.0
xjmTweak descriptions a bit.
Comment #158
Dries CreditAttribution: Dries commentedDeciding what services to include/hardcode can be discussed in follow-up issues. Other than that, there appears to be strong consensus that this is a big step forward. I'm happy with this patch, and will let catch commit it when he is comfortable that we've had enough discussion.
Comment #159
Crell CreditAttribution: Crell commentedRegardless of whether or not there's a Drupal:: method, the service is still available to any service by injection. We can certainly push more use of flood/lock/whatever where appropriate either way.
The only discussion here is around what the follow-ups will be, really. I think this is commitable. We already said before that we'd punt on "so what qualifies for its own utility method" to later; this code is just demonstrating how it all works, so we can start using it. The bikeshedding about each service can happen later, in a shorter thread.
Edit: This was in reply to catch; I didn't see xjm and Dries' comments before hitting submit.
Comment #160
webchickYeah, I'm worried that in the past ~20 comments, we're discussing application of this pattern, rather than introduction of this pattern, which would actually be better in a more focused issue once the basics are in. The introduction of the pattern seems very non-controversial, and it closes a critical. :D
Comment #161
msonnabaum CreditAttribution: msonnabaum commented100% agreed with Angie.
Comment #162
catchJust discussed this briefly with Rob Loach and msonnabaum in irc. We should require_once() the class rather than registering core/Drupal with the autoloader - there's no point autoloading one class that's always loaded and this will prevent the autoloader looking in that directory for anything else that matches Drupal.
Comment #163
webchickMakes sense.
Comment #164
catchAlso committing this and having a follow-up for applying the pattern is fine with me, just didn't want to kill of that discussion with a "Fixed, thanks!".
Comment #165
webchickCool, done: #1937600: Determine what services to register in the new Drupal class. Feel free to touch up the issue summary there.
Comment #166
RobLoachIncluding the file directly saves us from registering the "Drupal" prefix and having duplicate namespace matches ("Drupal/views", "Drupal/user", etc matching in core/lib as "Drupal").
Comment #167
chx CreditAttribution: chx commentedI like this.
Comment #168
plachPlease note that #157 has not been addressed yet.
Comment #169
webchickSince catch was OK with a follow-up issue to discuss where we should use this, and since Dries was OK with it as well, I decided to commit this without the feedback in #157 so it's ready to go in time for the global sprint weekend, which is bound to get lots of new core contributors looking at D8 for the first time. This might yield some good data that better informs #1937600: Determine what services to register in the new Drupal class.
So, Committed and pushed to 8.x. Thanks!! (seriously, I *really* appreciate all of the great work / brainstorming / discussion that went into this issue!!), but marking needs work because we should do #157 as a small follow-up now.
Also, this will need a change notice, so updating that as well.
Comment #170
amanire CreditAttribution: amanire commentedWorking on the change notice for this with Ehud.
Comment #171
DamienMcKennaPer feedback in IRC from msonnabaum and timplunkett, I've added a meta issue to track updating the rest of the codebase for this change: #1938334: META: Replace uses of drupal_container()
Comment #172
DamienMcKennaRelated to the change notice, how would the following code be changed (from config.inc)?
Comment #173
amanire CreditAttribution: amanire commentedEhud and I updated the following change records that had implemented the older drupal_container() syntax:
http://drupal.org/node/1853148
http://drupal.org/node/1848332
http://drupal.org/node/1862446
http://drupal.org/node/1479680
http://drupal.org/node/1929006
http://drupal.org/node/1852360
http://drupal.org/node/1918702
http://drupal.org/node/1842748
http://drupal.org/node/1805940
http://drupal.org/node/1635626
http://drupal.org/node/1887798
http://drupal.org/node/1901518
I will update http://drupal.org/node/1539454 with the documentation for the new service wrapper later tonight.
Comment #174
ParisLiakos CreditAttribution: ParisLiakos commentedre #172
i think this should do it
Comment #175
msonnabaum CreditAttribution: msonnabaum commentedEh, I'm not crazy about pulling the container out like that just to call the "has" method on it. I'd do this instead:
Comment #176
DamienMcKenna@msonnabaum: Good idea, thanks.
Comment #177
webchickRestoring lost tag.
Comment #178
podarokchecked all #173 issues and found all done
Thanks @amanire
This one is fixed.
Comment #179
salvisRe #172: Can we not get a no-throw version, so that we can write something like
or
?
Comment #180
xjm@salvis, to propose that, I'd suggest filing a followup issue.
Comment #182
salvisI've created #1957804: Add a nothrow option to Drupal::service() as suggested by xjm.
Comment #183
yched CreditAttribution: yched commentedFrom the issue summary :
So, is this still the official agreement that was reached ?
Is there an existing core example of a module helper class providing access to a service through static methods, or is \Drupal the only example ?
(asking
for a friendfor #1950088: Move FieldInfo class to the DIC)Comment #184
tim.plunkettSee http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul... for one example
Comment #185
amateescu CreditAttribution: amateescu commentedNote that the Views example is not quite correct, it shouldn't 'use' the Drupal class but prefix it with "\" whenever it's called in a namespaced file.
Comment #186
effulgentsia CreditAttribution: effulgentsia commentedWhy?
Comment #187
tim.plunkettNot having
use SomeTopLevelClass;
is in our coding standards now, you just have\SomeTopLevelClass
inline (stdClass, Exception, PDO, etc). I think that standard was adopted after this Views class got committed.However, because it is superflous, you don't do
\SomeTopLevelClass
in non-namespaced/procedural code, because it is redundant.Comment #188
effulgentsia CreditAttribution: effulgentsia commentedAh, thanks.
Comment #189
BerdirI also opened #1961548: [Policy, no patch] Decide how the module-specific services are made available as static methods (similar to \Drupal::$service()) for such classes for modules.
Comment #190
andypostHaving 4
Field
classes now really trouble so filed #2089807: Rename Field::fieldInfo() to FieldService::fieldInfo() for better DXComment #191
yched CreditAttribution: yched commentedRegarding #190: more generally than just field.module, see the discussion in #1961548: [Policy, no patch] Decide how the module-specific services are made available as static methods (similar to \Drupal::$service()) about why our current pattern is wrong.
Comment #191.0
yched CreditAttribution: yched commentedFixing small typo.