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

  1. 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);
    
  2. 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();
    
  3. 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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Often times if I repeat drupal_container()->get() calls within a method/function, I'll do

$container = drupal_container();
$container->get('foo');
$container->get('bar');

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.

vijaycs85’s picture

if we are going to use it, instead of:

+function drupal_service($id) {
+  return drupal_container()->get($id);
+}

we can go with:

+function drupal_service($id, Container $new_container = NULL) {
+  return drupal_container($new_container)->get($id);
+}
David_Rothstein’s picture

So 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):

- drupal_get_path_alias($data->source, $data->language)
+ drupal_container()->get('path.alias_manager')->getPathAlias($data->source, $data->langcode))

This is bad developer experience for three reasons:

  1. Uses concepts ("container") unrelated to the task at hand.
  2. Lots more characters to type.
  3. Uses string keys ('path.alias_manager') rather than actual API functions, meaning that it's difficult to find your way to the documentation of the function you're calling, and difficult to see what else is available.

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()?

David_Rothstein’s picture

By 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 :)

tim.plunkett’s picture

As 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!

adamdicarlo’s picture

From 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?

function path_alias_manager() {
  return drupal_container()->get('path.alias_manager');
}

Allowing this usage:

  path_alias_manager()->getPathAlias($data->source, $data->langcode);

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.

Crell’s picture

Tim 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.

quicksketch’s picture

@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?

effulgentsia’s picture

FileSize
5.64 KB

I 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:

+++ b/core/lib/Drupal/Core/Path/PathBundle.php
@@ -0,0 +1,44 @@
+  public static function aliasManager() {
+    return drupal_container()->get('path.alias_manager');
+  }
+++ b/core/modules/path/path.admin.inc
@@ -75,7 +77,7 @@ function path_admin_overview($keys = NULL) {
-    if ($data->alias != drupal_container()->get('path.alias_manager')->getPathAlias($data->source, $data->langcode)) {
+    if ($data->alias != PathBundle::aliasManager()->getPathAlias($data->source, $data->langcode)) {
Wim Leers’s picture

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

Crell’s picture

For 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.

David_Rothstein’s picture

As far as #3.3 goes, debug(drupal_container()->getServiceIds()); is your friend.

That 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 do debug(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.

How about having each Bundle class supply static wrappers that can be used by procedural code?

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.

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.

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.

webchick’s picture

Priority: Normal » Major

I think #12 has convinced me this needs to be major.

tim.plunkett’s picture

#7 convinced me that this is won't fix. So I'll just let the issue run its course...

Berdir’s picture

I 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.

Grayside’s picture

One 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.

David_Rothstein’s picture

I 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.

effulgentsia’s picture

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?

What would "as close to that as we possibly can" be, given procedural code limitations?

  1. Replace $this with drupal_container() and replace aliasManager with something localized to the 'path' subsystem, and end up with something like drupal_container()->pathAliasManager which isn't all that different from drupal_container()->get('path.alias_manager')
  2. Replace $this with something localized to the 'path' subsystem (e.g., PathBundle) and end up with something like PathBundle::aliasManager()
  3. Other ideas??
Berdir’s picture

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.

And 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:

drupal_container()->get('keyvalue')->get('collection')->get('mymodule.key');

We have 3 get() calls and each of them does something completely else. (container -> service factory -> implementation)

drupal_service('keyvalue')->get('collection')->get('mymodule.key');

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 :)

webchick’s picture

"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.

effulgentsia’s picture

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.

There 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.

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.

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')?

webchick’s picture

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')?

This 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 like db_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 than menu_link_save($item);

David_Rothstein’s picture

What would "as close to that as we possibly can" be, given procedural code limitations?

In 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.

Crell’s picture

I 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.

quicksketch’s picture

I 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.

tim.plunkett’s picture

I 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:

  • path_save()
  • path_load()
  • path_delete()

Now we have:

  • drupal_container()->get('path.crud')->save()
  • drupal_container()->get('path.crud')->load()
  • drupal_container()->get('path.crud')->delete()

This issue is proposing something like:

  • path_crud()->save()
  • path_crud()->load()
  • path_crud()->delete()

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.

quicksketch’s picture

There 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.

tim.plunkett’s picture

Option #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".

quicksketch’s picture

Option #1 DOES give us documentation, and possibly discoverability. Because then you have a procedural function with a docblock.

Option 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.

chx’s picture

Heh. 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.

chx’s picture

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

Berdir’s picture

Agreed 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:

In my opinion, the closest equivalent of $this->aliasManager->getPathAlias() would be alias_manager()->getPathAlias().

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?

xjm’s picture

Title: Shorten drupal_container()->get() » Improve DX of drupal_container()->get()

I 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.

dawehner’s picture

This 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

/** @var SomeKindOfServiceInterface */
$service = drupal_container()->get('service');

which is ugly from my point of view, but at least allows people to easier understand what is going on.

chx’s picture

Putting 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.

David_Rothstein’s picture

For 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.

Berdir’s picture

sun’s picture

FileSize
3.2 KB

Note 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:

-  drupal_container()->get('path.alias_manager')->getPathAlias($data->source, $data->langcode)
+  drupal_container()->PathAliasManager()->getPathAlias($data->source, $data->langcode)

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:

grep -R 'config(' ./core

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:

class Blah implements ContainerAware {
  function __construct(Container $container) {
    $this->settings = $container->get('config.factory');
  }

  function yada($name) {
    if ($this->settings->get($name)->get('yell')) {
      return 'YARRR.';
    }
  }
}

class Zomg extends Blah {
  function swear() {
    $factory = $this->getSettings();
    return $factory->get('troll.settings')->get('enabled') ? '********' : '[censored]';
  }
}

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.

effulgentsia’s picture

FileSize
7.24 KB

Apparently, all ::get*() methods on a compiled + dumped Container have proper phpDoc including appropriate @return statements already.

Yes, 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 than drupal_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".

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.

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.

chx’s picture

Strange 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.

effulgentsia’s picture

what do the constants buy us?

For #7.2.

+++ b/core/lib/Drupal/Core/Path/PathServices.php
@@ -0,0 +1,89 @@
+  /**
+   * ID for the alias manager service.
+   *
+   * @service_implements Drupal\Core\Path\AliasManagerInterface.
+   */
+  const ALIAS_MANAGER = 'path.alias_manager';

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.

RobLoach’s picture

+++ b/core/lib/Drupal/Core/Path/PathServices.phpundefined
@@ -0,0 +1,89 @@
+  public static function getAliasManager() {
+    return drupal_service(self::ALIAS_MANAGER);

I'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:

  • Remove getAliasManager(), getAliasManagerCached() and getCRUD().
+++ b/core/modules/path/path.admin.incundefined
@@ -75,7 +77,7 @@ function path_admin_overview($keys = NULL) {
-    if ($data->alias != drupal_container()->get('path.alias_manager')->getPathAlias($data->source, $data->langcode)) {
+    if ($data->alias != PathServices::getAliasManager()->getPathAlias($data->source, $data->langcode)) {
       $row['class'] = array('warning');

Due to the removal of those static functions, this change would end up being:

  drupal_service('path.alias_manager')->getPathAlias(/* ... */);

Recommendations:

  • Leverage drupal_service() directly, instead of the old PathServices static class wrapper. Reflects what is presented in the rest of the Drupal when dealing with the container. K.I.S.S.
effulgentsia’s picture

I'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.

I 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.

Leverage drupal_service() directly, instead of the old PathServices static class wrapper.

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.

RobLoach’s picture

Thanks 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.

effulgentsia’s picture

Would introduce a lot of code that doesn't need to be there.

Depends 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.

msonnabaum’s picture

The 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:

function drupal_container(ContainerInterface $new_container = NULL) {
  static $container;
  if (isset($new_container)) {
    $container = $new_container;
  }
  return $container;
}

class ConfigTest extends PhpUnit_Framework_Testcase {
  function testNotify() {
    $dispatcher = $this->getMock('Symfony\Component\EventDispatcher\EventDispatcher');
    $dispatcher->expects($this->once())
      ->method('dispatch')
      ->with($this->equalTo('config.init'));

    $container = $this->getMock('Symfony\Component\DependencyInjection\ContainerInterface');
    $container->expects($this->any())
      ->method('get')
      ->will($this->returnCallback(function() use ($dispatcher) {return $dispatcher;}));

    drupal_container($container);
    $storage = $this->getMock('Drupal\Core\Config\StorageInterface');
    $config = new Drupal\Core\Config\Config('blah', $storage);
    $config->init();
  }

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:

$queue = DrupalQueue::get($queue_name);

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:

Drupal::get('path.alias_manager')->getPathAlias($data->source, $data->langcode))

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.

webchick’s picture

The latest head-desk instance of this comes from #1887046-8: Convert drupal_http_request usage in install.core.inc to Guzzle.:

-  $result = drupal_http_request($uri);
+    $request = drupal_container()->get('http_default_client')->get($uri, array('Accept' => 'text/plain'));

:( :( :(

webchick’s picture

Btw, 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:

drupal_container()->get('magical string that's impossible to discover')->magical_function_name_that's_unique_depending_on_what_magical_string_was($uri, array('something I need to pass in that I didn't before (to be fair, that's a Guzzle problem)'));

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:

class PathAliasManagerService
class HttpClientManagerService
...

Their constructors would handle the drupal_container() injector crap, and then as a developer your calls would be more like.

$http_client = new HttpClientManagerService();
$http_client->get($uri); // or ->post() or whatever.

All of those methods/parameters would be nicely documented and nicely auto-complete-y. We still get the DIC stuff. Everyone wins?

effulgentsia’s picture

Assigned: Unassigned » effulgentsia

I'm gonna work on a couple simple patches that demonstrate #48 and related ideas.

moshe weitzman’s picture

#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.

msonnabaum’s picture

Talked 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:

$http_client = new HttpClientManagerService();
$http_client->get($uri);
// or
HttpClientManagerService::getInstance()->get($uri);

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:

Drupal::httpClient()->get($uri);

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.

webchick’s picture

One 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!!

effulgentsia’s picture

Here'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.

effulgentsia’s picture

Assigned: effulgentsia » Unassigned

#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.

Dave Reid’s picture

As a primarily contrib developer, I really like option #53.4.

I wonder if something like the following would be possible:

/**
 * Proxies AliasManagerInterface methods to the service object.
 */
class AliasManagerService implements AliasManagerInterface {

  public function __call($method, $arguments) {
    return call_user_func_array((self::get(), $method), $arguments);
  }

  protected static function get() {
    return drupal_container()->get('path.alias_manager');
  }
}

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.

tim.plunkett’s picture

@Dave Reid, unfortunately, you cannot. See http://drupalcode.org/project/drupal.git/blob?f=core/modules/views/views... for how we dealt with that.

plach’s picture

I 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?

<?php
use Symfony\Component\DependencyInjection\ContainerInterface;

interface ServiceInterface {
  public static function service(ContainerInterface $container = NULL);
}

class AliasManager implements ServiceInterface {
  public static function service(ContainerInterface $container = NULL) {
    $container = $container ?: drupal_container();
    return $container->get('path.alias_manager');
  }
}

$alias = AliasManager::service()->getPathAlias($path, $langcode);
?>
geerlingguy’s picture

[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?

msonnabaum’s picture

Lots 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.

plach’s picture

@msonnabaum:

We can't have un-autoloadable functions in classes.

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?

msonnabaum’s picture

Unit tests. No .inc should ever be available in a for-real unit test.

plach’s picture

In a unit test the $container parameter could be used, if we went the #57 way :)

Sorry, it's too late here ;)

effulgentsia’s picture

Let's focus on what each option allows us or prevents us from doing.

All 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.

plach’s picture

@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 through DrupalUnitTestBase 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.

David_Rothstein’s picture

#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?

Per-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:

Strict warning: Non-static method Drupal\Core\Path\AliasManagerService::getPathAlias() should not be called statically in Drupal\Core\Path\AliasManagerService::getPathAlias() (line 80 of core/modules/path/path.admin.inc).

That seems.... bad :)

David_Rothstein’s picture

So did #53.4 actually intend for this to work more like the bottom of @webchick's post in #48? No static methods, just:

$http_client = new HttpClient();
$http_client->get($uri); // or ->post() or whatever.

(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 be HttpServices::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:

$http_client = drupal_http_client();
...
$http_client->get($uri);

doesn't look a whole lot different from this:

$query = db_update($table);
....
$query->execute();

Which hasn't generated many complaints or controversies in the years we've been using it...

msonnabaum’s picture

On 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.

David_Rothstein’s picture

As 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.

Dries’s picture

I think we should explore using class loaders for locating complete services (and keeping dependency injection for more narrow configurability).

effulgentsia’s picture

Assigned: Unassigned » effulgentsia

#69 is inspired by Java. I'll work up a patch to demonstrate what it would look like in PHP/Drupal.

Crell’s picture

Assigned: effulgentsia » Unassigned

I 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); over HttpClientManagerService::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:

class MyModuleService {
  /**
   * @return MyServiceClass
   */
  public static function service() {
    return Drupal::container()->get('myservice');
  }
}

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.]

David_Rothstein’s picture

That makes a lot of sense to me.

So to summarize with some actual examples, we'd change things like this?

  1. For a service provided by the Drupal core base system:
    - drupal_container()->get('flood')->register($event_name);
    + Drupal::flood()->register($event_name);
    
  2. 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();
    
  3. And for a service that is obscure and no one intended to be used as a public API:
    - drupal_container()->get('some.service')->someMethod();
    + Drupal::container()->get('some.service')->someMethod();
    

A little unclear on which namespace "Drupal" and "Edit" would live in, but regardless this looks good.

msonnabaum’s picture

Those 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 a set method here, because this object shouldnt be treated like its a container, it's just a wrapper that obscures the container.

chx’s picture

A consensus is forming so I took #72 and made an issue summary out of it.

Crell’s picture

Issue summary: View changes

Wrote an issue summary.

chx’s picture

Issue summary: View changes

Updated issue summary.

Crell’s picture

Tweaked 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?

Berdir’s picture

I'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:

A fully-qualified namespace and class must have the following structure \\< Vendor Name >\\(< Namespace >\\)*< Class Name >

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

msonnabaum’s picture

Honestly, 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.

jhodgdon’s picture

Regarding #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?

tim.plunkett’s picture

They wouldn't need to (and couldn't) override the class providing the service, they would just swap out the service.

Crell’s picture

jhodgdon: 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.

plach’s picture

Getting a non-namespaced Drupal class to autoload is a solvable implementation detail.

Given 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.

Crell’s picture

I 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. :-)

natted’s picture

A 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.

sun’s picture

The 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:

  1. Drupal can't be a class, unless it's Drupal\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 is drupal_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 turning drupal_container()->get() into Drupal::service() instead of drupal_service(). The only difference between the two would be an include_once vs. spl_autoload_register('wtf').
  2. 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 of Edit::whatever(), you'll arrive at the conclusion that it hits the same problem space.
  3. @Berdir's take in #76 seems to be the most sane + workable: Drupal\Core\Core::whatever() (nicely corresponding to CoreBundle) and Drupal\edit\Edit::whatever() (nicely corresponding to EditBundle).
  4. That proposal is attractive, since it means consistency. Both for core bundles (which we should split out into individual bundles, since by now we know that our current CoreBundle is the implementation of no-idea-of-symfony-bundles-so-just-create-a-single-dumping-ground-for-everything, but in that case, we likely want to keep Core::, since, in terms of DX, core sorta counts as "one giant module"), and, on the other hand, it means consistency for modules. In either case, I've to import Drupal\[Provider]\[Name] into my file, but that's most certainly not the only use statement that will exist in files, so "trying to avoid that" is a misleading/bogus goal in the first place, IMHO.
  5. Likewise, I think that @Dries' suggestion in #69 was off-topic for this issue, since it merely suggests to reduce the problem space, without solving the actual (and still remaining) problem space of abstracted, container-injected services.
  6. In effect, the proposed change will deliver on both fronts: 1) IDE auto-completion. 2) Grep-ability (to some regained extent).

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.

chx’s picture

While Drupal\edit\Edit::whatever() sounds good because after a use it becomes just Edit::whatever(), I would like to suggest Drupal\Core\Drupal::whatever() so that it looks Drupal::whatever() after a use.

Berdir’s picture

\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.

msonnabaum’s picture

Our autoloader totally supports Drupal as a class.

Just change: $prefixes = array(); to $prefixes = array('Drupal' => DRUPAL_ROOT . '/core/lib'); in drupal_classloader().

Crell’s picture

So 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.

cweagans’s picture

Let's just get the framework in place and we can then discuss what else "deserves" to be there piecemeal in parallel.

+1000 to this. I will personally commit to helping people file patches to add other services to this during Core Mentoring Hours.

cweagans’s picture

Status: Needs review » Reviewed & tested by the community

You 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.

Berdir’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/includes/bootstrap.incundefined
@@ -3174,7 +3160,7 @@ function drupal_classloader($class_loader = NULL) {
-    $prefixes = array();
+    $prefixes = array('Drupal' => DRUPAL_ROOT . '/core/lib');

Still would like to know if that negatively impacts performance. Probably depends in which order these things are checked.

+++ b/core/lib/Drupal.phpundefined
@@ -0,0 +1,125 @@
+  public static function db() {

I would say this should be database()

+++ b/core/lib/Drupal.phpundefined
@@ -0,0 +1,125 @@
+  public static function cache($bin = 'cache') {

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.

plach’s picture

Pushed a couple of fixes:

97b6880 Fix coding standard issues.
4d69dd0 Deprecate cache() procedural wrapper.
+++ b/core/lib/Drupal.php
@@ -0,0 +1,125 @@
+ * Contains Drupal.

This should be "Contains \Drupal." but I think it deserves an exception :)

+++ b/core/lib/Drupal.php
@@ -0,0 +1,125 @@
+  public static function db() {
+    return static::service('database');
+  }

Given the amount of times this will probably be called I'd save a method invocation and directly access static::$container.

Crell’s picture

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

Status: Needs review » Needs work

The last submitted patch, 1875086-drupal-container.patch, failed testing.

Crell’s picture

Status: Needs work » Needs review

#93: 1875086-drupal-container.patch queued for re-testing.

msonnabaum’s picture

I 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.

Crell’s picture

OK, renamed db() to database(), dropped the cache utility method, added a lock utility method. Again updated variable_initialize() to demonstrate the syntax change.

Status: Needs review » Needs work

The last submitted patch, 1875086-drupal-container.patch, failed testing.

plach’s picture

Let's try this one. Didn't push this since the latest changes from #97 are missing on the wscci repository.

RobLoach’s picture

Although \Drupal as a class does work, it is better to use an explicit namespace like Drupal\Core\Drupal. This is because if we register the Drupal\Core and Drupal\Component as separate namespaces, classes like Drupal\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:

  1. Use Drupal\Core\Drupal as suggested in #85 and #86 by both chx and Berdir.
  2. I'd be more than happy to re-roll the patch tomorrow
  3. Definitely love the direction here and am very happy people are on board on pushing Dependency Injection and the Container forwards
RobLoach’s picture

Tagging.

msonnabaum’s picture

Rob, 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.

msonnabaum’s picture

Also, 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.

plach’s picture

If we were to change the current approach, Drupal\Core\Service, Service::lock() and Service::get() would look more self-documenting to me.

jhodgdon’s picture

Please... 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.

plach’s picture

Thanks 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:

<?php
Service::cache()->get('key');
Service::lock()->acquire('lock_name');
Service::get('my_contrib_service')->do('stuff');
?>

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.

jhodgdon’s picture

It'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?

tim.plunkett’s picture

Our 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.

chx’s picture

I am fine with \Drupal, when I suggest something else that was on the grounds that the autoloader doesn't support it.

plach’s picture

We are currently using the object returned by drupal_container() as service container. The Drupal class holds a singleton instance of the service container, that is the same object currently returned by drupal_container(). Hence the Drupal object has a slightly different role. The static methods on it are the equivalents of the various non-autloadable cache(), 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:

<?php
ServiceContainer::getCacheService()->get('key');
ServiceContainer::getLockService()->acquire('lock_name');
ServiceContainer::getService('my_contrib_service')->do('stuff');
?>

which would be very verbose and actually less readable to my eyes (the signal to noise ratio is very low IMO).

cweagans’s picture

+1 for just \Drupal. Less keystrokes and it reads very nicely.

plach’s picture

Just 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 :)

effulgentsia’s picture

It 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 a use statement anywhere. There should be very few places where namespaced code needs to do anything on the Drupal class: the patch includes a usage from DrupalKernel and TestBase, and ideally, those should be the only two places where that's needed.

Crell’s picture

New 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.

Status: Needs review » Needs work

The last submitted patch, 1875086-drupal-container.patch, failed testing.

plach’s picture

+++ b/core/modules/user/user.module
@@ -1330,13 +1330,14 @@ function user_login_name_validate($form, &$form_state) {
+  $flood_manager = Drupal::service('flood');

I'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.

catch’s picture

Priority: Major » Critical

Bumping 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.

Crell’s picture

I don't particularly care which variable name convention we use here.

plach’s picture

This should pass tests now and address #113:

c0670cc Standardize on variable name matching service name.
160e2ae Fix contact tests.
5259988 Fix indentation in Drupal class.
0a4cfef Convert the field translation synchronizer service.

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.

Crell’s picture

+++ b/core/modules/translation_entity/translation_entity.module
@@ -868,11 +868,21 @@ function translation_entity_field_info_alter(&$info) {
 /**
+ * Returns an instance of the field translation synchronizer service.
+ *
+ * @return \Drupal\translation_entity\FieldTranslationSynchronizerInterface
+ *   An object implementing the field translation synchronizer interface.
+ */
+function translation_entity_synchronizer() {
+  return Drupal::service('translation_entity.synchronizer');

Erp. No. This is exactly what we're trying to eliminate. No function wrappers. They can't autoload.

plach’s picture

Erp. No. This is exactly what we're trying to eliminate. No function wrappers. They can't autoload.

Yep, 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's lib directory?

Crell’s picture

Status: Needs review » Needs work

Quite 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.

plach’s picture

Here is how things should work according to my understanding of #122.

msonnabaum’s picture

Here'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.

plach’s picture

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.

Personally 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 using use (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.

+++ b/core/modules/user/lib/user.php
@@ -0,0 +1,7 @@
+class user {
+  public static function userData() {
+    return Drupal::service('user.data');
+  }

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.

plach’s picture

+++ b/core/modules/user/lib/user.php
@@ -0,0 +1,7 @@
+class user {
+  public static function userData() {
+    return Drupal::service('user.data');
+  }

Another minor thing: if we go this way, I'd extend the Drupal class so we can directly access static::$container and save a method invocation.

Berdir’s picture

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -457,7 +457,9 @@
     foreach ($moduleFileNames as $module => $filename) {
-      $this->classLoader->registerNamespace("Drupal\\$module", DRUPAL_ROOT . '/' . dirname($filename) . '/lib');
+      $path = DRUPAL_ROOT . '/' . dirname($filename) . '/lib';
+      $this->classLoader->registerNamespace("Drupal\\$module", $path);
+      $this->classLoader->registerPrefix($module, $path);

We 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)" ?

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -99,7 +99,7 @@ function overlay_field_extra_fields() {
-    $account_data = drupal_container()->get('user.data')->get('overlay', $account->id(), 'enabled');
+    $account_data = user::userData()->get('overlay', $account->id(), 'enabled');
     $form['overlay_control'] = array(

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?

plach’s picture

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 :)

I 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.

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

Makes sense. But since it seems we'd better namespace the wrapper class my suggestion still stands :)

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?

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 :)

plach’s picture

@effulgentsia:

Any comment on the latest patches?

webchick’s picture

Note 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. :)

RobLoach’s picture

Overall, 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.

+++ b/core/includes/bootstrap.incundefined
@@ -981,17 +981,18 @@ function variable_initialize($conf = array()) {
-    if (!lock()->acquire($name, 1)) {
+    $lock = Drupal::lock();
+    if (!$lock->acquire($name, 1)) {
       // Another request is building the variable cache.
       // Wait, then re-run this function.
-      lock()->wait($name);
+      $lock->wait($name);
       return variable_initialize($conf);

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.

+++ b/core/includes/bootstrap.incundefined
@@ -3174,7 +3161,7 @@ function drupal_classloader($class_loader = NULL) {
     $prefixes_and_namespaces = require DRUPAL_ROOT . '/core/vendor/composer/autoload_namespaces.php';
-    $prefixes = array();
+    $prefixes = array('Drupal' => DRUPAL_ROOT . '/core/lib');
     $namespaces = array();

See also: #1658720: Use ClassLoader instead of UniversalClassLoader ... That allows the namespace definitions to be in composer.json rather than handled in drupal_classloader() itself.

+++ b/core/includes/bootstrap.incundefined
@@ -3547,10 +3534,13 @@ function drupal_check_memory_limit($required, $memory_limit = NULL) {
  * Get locking layer instance.
  *
+ * @deprecated Use Drupal::lock() instead, or even better have the lock service
+ *   injected into your object.

I like to see the @deprecated tag on global PHP functions. Would love to deprecated more of our global functions in the future.

+++ b/core/includes/install.core.incundefined
@@ -367,7 +367,7 @@ function install_begin_request(&$install_state) {
diff --git a/core/lib/Drupal.php b/core/lib/Drupal.php

diff --git a/core/lib/Drupal.php b/core/lib/Drupal.php

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.

+++ b/core/lib/Drupal.phpundefined
@@ -0,0 +1,99 @@
+class Drupal {
+
+  /**
+   * The currently active container object.
+   *
+   * @var \Symfony\Component\DependencyInjection\ContainerInterface
+   */
+  protected static $container;

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.

+++ b/core/lib/Drupal.phpundefined
@@ -0,0 +1,99 @@
+   */
+  public static function database() {
+    return static::$container->get('database');
+  }
+
+  /**
+   * Returns the locking layer instance.
+   *
+   * @return \Drupal\Core\Lock\LockBackendInterface
+   */
+  public static function lock() {
+    return static::$container->get('lock');

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.

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -457,7 +457,9 @@ protected function getModuleFileNames() {
+      $path = DRUPAL_ROOT . '/' . dirname($filename) . '/lib';
+      $this->classLoader->registerNamespace("Drupal\\$module", $path);
+      $this->classLoader->registerPrefix($module, $path);

This is a change in the design of how a module's lib directory works. Should this be a separate issue?

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -99,7 +99,7 @@ function overlay_field_extra_fields() {
   $account = $form_state['controller']->getEntity($form_state);
   if (user_access('access overlay', $account)) {
-    $account_data = drupal_container()->get('user.data')->get('overlay', $account->id(), 'enabled');
+    $account_data = user::userData()->get('overlay', $account->id(), 'enabled');
     $form['overlay_control'] = array(

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?

  use Drupal\user\User;
  $account_data = User::userData()->get('overlay', $account->id(), 'enabled');

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.

+++ b/core/modules/user/lib/user.phpundefined
@@ -0,0 +1,7 @@
+<?php
+
+class user {
+  public static function userData() {
+    return Drupal::service('user.data');
+  }

Yeah, that should probably just be Drupal\user\User. Some documentation would be good too.

Crell’s picture

Rob: 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.

plach’s picture

What 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.

sun’s picture

Yes. Let's move forward with a helper class for Drupal core services only for now. We can revisit the situation for modules later.

Crell’s picture

#133++

plach’s picture

If there is consensus on #133 then #123 is the patch to review/RTBC.

msonnabaum’s picture

As expressed in #125, my preference is also to not deal with modules for now, so I'm in favor of #123.

Crell’s picture

plach’s picture

Tangentially related: #1935762: Remove entity_load_unchanged() call from FieldTranslationSynchronizer::synchronizeFields() (discussing dependency injection DX for entities there).

Crell’s picture

Someone want to RTBC #123 then? (I don't think I can, since I wrote too much of it.)

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

I'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.

David_Rothstein’s picture

There was this (from #127):

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 :)

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 become Drupal::flood() but that's not too important now since there will be followup issues to do the full set of conversions anyway.

plach’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
19.02 KB
2.35 KB

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

Done.

Also think Drupal::service('flood') should become Drupal::flood() but that's not too important now since there will be followup issues to do the full set of conversions anyway.

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.

quicksketch’s picture

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.

I 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.

Crell’s picture

quicksketch: 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.

webchick’s picture

What 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?

quicksketch’s picture

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.

Perhaps 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".

Crell’s picture

Status: Needs review » Reviewed & tested by the community

quicksketch: 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. :-)

plach’s picture

@quicksketch:

I just don't want that new issue to have someone pointing at this one and saying, "we already made that decision".

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.

plach’s picture

I 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:

+++ b/core/modules/contact/lib/Drupal/contact/MessageFormController.php
@@ -191,7 +191,7 @@ public function save(array $form, array &$form_state) {
-    drupal_container()->get('flood')->register('contact', config('contact.settings')->get('flood.interval'));
+    \Drupal::service('flood')->register('contact', config('contact.settings')->get('flood.interval'));

This could use a @todo as the flood service should be injected.

+++ b/core/modules/system/lib/Drupal/system/Tests/System/FloodTest.php
@@ -30,20 +30,21 @@ function testCleanUp() {
-    drupal_container()->get('flood')->register($name, $window_expired);
+    $flood = \Drupal::service('flood');

We should be doing $this->container->get('flood') here.

If we agree I can do a quick reroll.

msonnabaum’s picture

I wouldn't bother, the issue is unrelated to this one, which is already hard enough to follow.

sun’s picture

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.

Hm. 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.

quicksketch’s picture

Hm. I'd recommend to use this rule of thumb instead: Every service component in Drupal\Core\* should have a dedicated method.

+1 I'd like to see all general-purpose services (even obscure ones) to be available in this way.

Crell’s picture

"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.

Berdir’s picture

Agreed 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.

catch’s picture

I'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.

xjm’s picture

Overall 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 that drupal_container()->get() never did.

+++ b/core/lib/Drupal.phpundefined
@@ -0,0 +1,143 @@
+/**
+ * @file
+ * Contains Drupal.
+ */

lol.

+++ b/core/lib/Drupal.phpundefined
@@ -0,0 +1,143 @@
+ * Static Service Container wrapper.

Nitpick: should start with a verb.

+++ b/core/lib/Drupal.phpundefined
@@ -0,0 +1,143 @@
+ * Generally, code in Drupal should accept its dependencies via either
+ * constructor injection or setter method injection. However, there are cases,
+ * particularly in legacy procedural code, where that is infeasible. This

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.

+++ b/core/lib/Drupal.phpundefined
@@ -0,0 +1,143 @@
+ * @code

This example is very helpful, thanks.

+++ b/core/lib/Drupal.phpundefined
@@ -0,0 +1,143 @@
+ *   // Legacy procedural code.
+ *   function hook_do_stuff() {
+ *     $lock = lock()->acquire('stuff_lock');
+ *     // ...
+ *   }
+ *
+ *   // Correct procedural code.
+ *   function hook_do_stuff() {
+ *     $lock = Drupal::lock()->acquire('stuff_lock');
+ *     // ...
+ *   }

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.

xjm’s picture

Issue summary: View changes

Tweak descriptions a bit.

Dries’s picture

Deciding 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.

Crell’s picture

Regardless 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.

webchick’s picture

Yeah, 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

msonnabaum’s picture

100% agreed with Angie.

catch’s picture

Just 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.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Makes sense.

catch’s picture

Also 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!".

webchick’s picture

Cool, done: #1937600: Determine what services to register in the new Drupal class. Feel free to touch up the issue summary there.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
1.03 KB
18.86 KB

Including 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").

chx’s picture

Status: Needs review » Reviewed & tested by the community

I like this.

plach’s picture

Please note that #157 has not been addressed yet.

webchick’s picture

Title: Improve DX of drupal_container()->get() » Change notice: Improve DX of drupal_container()->get()
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

Since 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.

amanire’s picture

Issue tags: +SprintWeekend2013

Working on the change notice for this with Ehud.

DamienMcKenna’s picture

Issue tags: -SprintWeekend2013

Per 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()

DamienMcKenna’s picture

Related to the change notice, how would the following code be changed (from config.inc)?

  if (drupal_container()->has($context_name)) {
    $context = Drupal::service($context_name);
  }
amanire’s picture

ParisLiakos’s picture

re #172
i think this should do it

if (Drupal::getContainer()->has($context_name)) {
  $context = Drupal::service($context_name);
}
msonnabaum’s picture

Eh, I'm not crazy about pulling the container out like that just to call the "has" method on it. I'd do this instead:


  try {
    $context = Drupal::service($context_name);
  catch (ServiceNotFoundException $e) {
    if (class_exists($context_name) && in_array('Drupal\Core\Config\Context\ContextInterface', class_implements($context_name))) {
      $context = drupal_container()
        ->get('config.context.factory')
        ->get($context_name);
    }
    else {
      throw new ConfigException(sprintf('Unknown config context service or class: %s', $context_name));
    }
  }


DamienMcKenna’s picture

@msonnabaum: Good idea, thanks.

webchick’s picture

Issue tags: +SprintWeekend2013

Restoring lost tag.

podarok’s picture

Title: Change notice: Improve DX of drupal_container()->get() » Improve DX of drupal_container()->get()
Status: Needs work » Fixed
Issue tags: -Needs change record

checked all #173 issues and found all done
Thanks @amanire

This one is fixed.

salvis’s picture

Issue tags: +Needs change record

Re #172: Can we not get a no-throw version, so that we can write something like

if ($context = Drupal::serviceOrNull($context_name)) {
  // ...
}

or

if ($context = Drupal::service($context_name, FALSE)) {
  // ...
}

?

xjm’s picture

Issue tags: -Needs change record

@salvis, to propose that, I'd suggest filing a followup issue.

Status: Fixed » Closed (fixed)

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

salvis’s picture

I've created #1957804: Add a nothrow option to Drupal::service() as suggested by xjm.

yched’s picture

From the issue summary :

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();

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 friend for #1950088: Move FieldInfo class to the DIC)

tim.plunkett’s picture

amateescu’s picture

Note 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.

effulgentsia’s picture

Why?

tim.plunkett’s picture

Not 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.

effulgentsia’s picture

Ah, thanks.

Berdir’s picture

andypost’s picture

yched’s picture

Regarding #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.

yched’s picture

Issue summary: View changes

Fixing small typo.