Over in #1875086: Improve DX of drupal_container()->get(), we had a lively discussion about the DX impact of things like this:

drupal_container()->get('flood')->register($event_name);

and changed them to things like this:

Drupal::flood()->register($event_name);

However, in the later comments (from about #144 on) had some contentions around this part of the proposal:

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

Suggestions (and concerns) can basically be summarized as:

- Decide on a case-by-case basis (How? What criteria? How will this be easily communicated to "end developers" so they know what to expect?)

- Apply a standard rule that just chucks everything in /includes or /Drupal/Core into a method on the class (It can't be this simple though; there are hundreds of things registered as services and we need a way to differentiate a "major" service from a "minor" one that's never intended to be used as a public-facing API)

Proposed solution

All public API's get a method on the Drupal class. The initial patch in this issue adds a bunch of those and also converts some existing calls to those new methods. The idea is to a) minimize conflicts between the follow-up conversions and provide some examples, so that the follow-ups can be done as Novice issues. It does not mean to be a complete list, additional methods can be added any time later.

Remaining tasks

(Fill follow-up issues as necessary and update change notice(s) according to the following lists)

Added as a method and already converted as an example (can be considered done)

* Drupal::queue()
* Drupal::httpClient()
* Drupal::keyValue()

Added as a method but not yet converted/removed the old function (Needs follow-up issue per method if not yet existing to convert existing calls for the eventually existing old function/drupal_container() and removal of the eventually existing function)

* #1957142: Replace config() with Drupal::config()
* #1947720: Use Drupal::state() to replace state()
* #1957148: Replace entity_query() with Drupal::entityQuery()
* #1957152: Replace calls to the flood service with Drupal::flood()
* #1957154: Replace calls to drupal_container()->get('module_handler') service with Drupal::moduleHandler()
* #1957158: Replace typed_data() with Drupal::typedData()

Not (yet) added (Needs an issue to discuss/add if not yet existing)

* url generator (#1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence)
* request (Requested to be left out by @Crell, @msonnabaum disagreed (I think?) in IRC, let's fight! in #1957156: Add a request() method to \Drupal)
* plugin managers (#1950670: Decide what methods should be added for plugin managers to \Drupal)

I'm not adding any services that are not supposed to be called directly/manually (like event listeners, compiler passes, route access checks, ...). Also haven't added module specific services yet. We have not yet discussed what to do with that after the initial discussion in the original issue.

Once those specific cases are done, #1938334: META: Replace uses of drupal_container() can be unpostponed to get rid of the remaining calls, which should only be a few remaining ones. And we can always add more methods if we find more useful ones later on.

Follow-up Issues

See remaining tasks.

Files: 
CommentFileSizeAuthor
#51 drupal-service-1937600-51.patch16.95 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 53,866 pass(es).
[ View ]
#51 interdiff.txt2.72 KBParisLiakos
#47 drupal-service-1937600-48.patch16.56 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 54,055 pass(es).
[ View ]
#41 drupal-service-1937600-41.patch16.56 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 53,385 pass(es).
[ View ]
#35 drupal-service-1937600-35.patch16.3 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 53,295 pass(es), 12 fail(s), and 0 exception(s).
[ View ]
#35 drupal-service-1937600-35-interdiff.txt396 bytesBerdir
#25 drupal-service-1937600-25.patch16.48 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 53,053 pass(es).
[ View ]
#25 drupal-service-1937600-25-interdiff.txt1.15 KBBerdir
#23 drupal-service-1937600-23.patch16.23 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 53,005 pass(es), 13 fail(s), and 0 exception(s).
[ View ]
#23 drupal-service-1937600-23-interdiff.txt6.1 KBBerdir
#20 drupal-service-1937600-20.patch19.68 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 52,928 pass(es), 13 fail(s), and 0 exception(s).
[ View ]
#18 drupal-service-1937600-17.patch8.55 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 52,930 pass(es).
[ View ]

Comments

I'll start the discussion by saying that this comment by Crell made me just about spit coke out all over my keyboard:

We're going to have to exercise good contextually-relevant judgment. I'm sure we can manage that.

No, we can't manage that. :) As evidenced by many hundreds of points in Drupal history. :D We are a "pattern applying machine" as a community. Also, just because core developers who understand the system deeply can manage contextually-relevant judgment *for them* doesn't mean that that judgment will mean anything to "end developers" who are going to be completely befuddled as to why Database is in there but not Lock, simply because a group of 8 people on an issue decided that wasn't useful to most people.

So I really think we need to go with option b) and come up with a rule. But I don't know technically how to do that. Names of directories in Drupal/Core? Two service containers: an "API" one and a "service" level one, similar to how we differentiate config from content entities?

Should we introduce an additional section in mymodule.api.php (or something) which documents what services are introduced to the Container by the module? Having a bunch of class wrappers around the Container is fine, but I'd definitely like to see more documentation for each service provided.

Well, that won't work because in many cases (like all of the ones in there currently?) these aren't exposed by modules at all, but by /Drupal/Core/ classes.

However, +1000 for better docs. But probably a better "centralized" place for this is on the Drupal methods themselves, no?

If we make use of public vs. private services on the Container, we can have the container tell us what services are available. We just need to get around to copying that utility from Symfony. Hand-duplicating that is asking for people to forget things.

Webchick: I was being deliberately sarcastic there as a challenge, because quite frankly I believe we *should* be capable of exercising good judgment rather than "blindly do the same to ALL THE THINGS". Encouraging core devs to ignorantly apply a pattern rather than thinking about if it makes sense breeds core devs who can only ignorantly apply a pattern rather than thinking about what makes sense.

We may well end up deciding that what makes sense comes very close to (but doesn't exactly match) "one service per directory in \Drupal\Core". Certainly there's an argument for that. But we shouldn't start there. We as a community like to talk. So, let's talk intelligently.

Meaning, I'm not opposed to Lock getting its own legacy wrapper method. I am opposed to it getting its own wrapper method "because it's there". And remember, this applies to hooks and code that's not been converted to a service yet. Injected object is still the preferred approach where possible.

Status:Postponed» Active

Ok, that just got committed, so marking back to active.

Wrt flood, I use that in Honeypot.module, as well as other modules not on drupal.org besides, and see value in having it in the wrapper. I think more modules should be using flood control to track things like spammers and potential DDoS situations.

"And for a service that is obscure, rarely used, or not intended to be used as a public API:"

I'm not sure when "rarely used" made it into this list actually (the original version and the original text in the issue summary did not have that part). I think "rarely used" is the problematic part here.

I like the way Berdir put it too: "We shouldn't decide based on how often something is used but if it's a public service or not."

That does require contextually-relevant judgment; however, if you're writing code, whether or not you intend it to be a public API is something pretty fundamental you should be thinking about anyway. I see this as very similar to putting an underscore in front of a function name to indicate your intention that it be private. People make that choice all the time.

So:

  • Drupal::serviceName() if the service is written with the intention of being a public API.
  • Drupal::service('service.name') if it's not.

That seems pretty simple to me. The only thing that bothers me about it is that even in the case of a service that is not intended as a public API, you still might want to document it for the benefit of someone reading your code. And there doesn't seem to be a great way to go from Drupal::service('service.name') to the place where that documentation actually lives.

+100 for #7. Don't know what to do about that last part though. :\

So, from the list in #1875086-154: Improve DX of drupal_container()->get(), what about this list to get started. The names can be changed and discussed of course.

* Drupal::config($name)
* Drupal::queue($queue)
* Drupal::keyValue($collection)
* Drupal::keyValueExpirable($collection) (not yet in the container/list but I have two patches that add it and use it)
* Drupal::settings()
* Drupal::state()
* Drupal::pathAliasManager() (the cached one of course)
* Drupal::path()
* Drupal::cache() (this is not yet really in the container, actually)
* Drupal::request()
* Drupal::httpClient()
* Drupal::typedData()
* Drupal::entityQuery()
* Drupal::flood()
* Drupal::moduleHandler()

There might be more, but we can always add more. So if we more or less agree on that list and that we should do it (which I think we do as we committed the other issue), then we can probably make this a meta task and open separate issues for the specific cases. Where can also discuss the name and so on.

Injected object is still the preferred approach where possible.

I'd like to re-iterate what Crell said here. Injecting the objects correctly avoids this problem entirely. I understand we won't be able to accomplish injection for all our services by launch, but implementing it benefits us so much more than what is proposed here.

Not saying we shouldn't do this, however, I'm just saying that having objects given to you directly is nicer than having to ask for them.

In #9, what's the difference between ::path() and ::pathAliasManager()?

It's a different service, path CRUD vs. path alias stuff. Can't combine that into one here :)

I chose path() over pathCrud() as the second IMHO sounds strange but as I said, we can discuss that in specific issues how they should be called exactly.

Hm. Yeah, those should be split, but the names are confusing. ::path() almost suggests the system_path property. However, with the work in #1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence I'd actually suggest we don't want to expose path alias manager but instead only expose the generator. People should be making their links through that, once Kat's patch goes in.

Is ::request() intended to be the current request object? While I know that's common, I'd almost suggest we don't expose that specifically to discourage its use. The more code we have that's request-agnostic, the more easily we'll be able to break code up and unit test it (and run it through drush without weird contortions).

I can't speak to the KV system as I don't know it well enough. Otherwise the list seems reasonable overall.

Sure, we can leave out both of those or at least delay to see how often it will be used later on. I think KV makes sense, it's not used that often yet but I think it's a public, generic and useful API. For example, I have an issue that drops cache_update in favor of multiple key value collections both expirable and not.

What we should do however is to rename $pathaliasmanager and $pathaliasmanager.cached to $pathaliasmanager.uncached and $pathaliasmanager. Maybe even make the uncached one private, I don't think there is a reason to access it from the outside at all? ((has nothing to do with this issue)

So, my suggestion would be to turn this into a [meta] conversion issue, maybe someone can pick one of the methods and provide an example patch and then we can make most of the other ones as novice issues?

Status:Active» Needs review
StatusFileSize
new8.55 KB
PASSED: [[SimpleTest]]: [MySQL] 52,930 pass(es).
[ View ]

Here is an initial patch. Added all the services that I listed above except path stuff and request to Drupal, because there's otherwise a large potential for conflicts there. Used the docblocks of existing functions when possible.

Also converted all calls to keyvalue as an example. Not sure if we want to do the other conversions here as well. Some are quite simple (flood), others will be big (state/config).

Should we then mark functions like config(), state() etc. as deprecated?

+++ b/core/lib/Drupal.phpundefined
@@ -140,4 +140,121 @@ public static function lock() {
+  public static function entityQuery($entity_type, $conjunction = 'AND') {
+    return static::$container->get('entity.query')->get($entity_type, $conjunction);
+  }

This is actually a nice win for the DX.

StatusFileSize
new19.68 KB
FAILED: [[SimpleTest]]: [MySQL] 52,928 pass(es), 13 fail(s), and 0 exception(s).
[ View ]

Did we agree on using @deprecated now or is that still open?

I guess we can also do it all in this issue but might want to wait until after the sprint and the RTBC queue is lower.

Converted flood and queue for now.

Status:Needs review» Needs work

The last submitted patch, drupal-service-1937600-20.patch, failed testing.

Status:Needs work» Needs review

There are pending patches that move around contact module's use of flood already and plan to make them injected, so let's not touch those here for now. cf #1938390: Convert contact_site_page and contact_person_page to a new-style Controller

+++ b/core/lib/Drupal.php
@@ -140,4 +140,121 @@ public static function lock() {
+   * @code config('book.admin') @endcode will return a configuration object in

Code block needs to be updated.

+++ b/core/lib/Drupal.php
@@ -140,4 +140,121 @@ public static function lock() {
+   * Instantiates and statically caches the correct class for a queue.

Description needs updating. I think all of these wrappers should probably just be a "returns" statement.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/fetcher/DefaultFetcher.php
@@ -30,7 +30,7 @@ class DefaultFetcher implements FetcherInterface {
   function fetch(Feed $feed) {
-    $request = drupal_container()->get('http_default_client')->get($feed->url->value);
+    $request = \Drupal::httpClient()->get($feed->url->value);

Please mark this with a @todo to make injected once we resolve how to inject plugins.

+++ b/core/modules/contact/contact.pages.inc
@@ -92,7 +92,7 @@ function contact_flood_control() {
-  if (!Drupal::service('flood')->isAllowed('contact', $limit, $interval)) {
+  if (!Drupal::flood()->isAllowed('contact', $limit, $interval)) {

Please don't touch this, as it will conflict with another issue as above. :-)

StatusFileSize
new6.1 KB
new16.23 KB
FAILED: [[SimpleTest]]: [MySQL] 53,005 pass(es), 13 fail(s), and 0 exception(s).
[ View ]

Ok, reverted the flood conversions, fixed the comments and also added typedData(). It makes sense to start with a useful list to avoid unecessary conflicts but I think we can always add more as we find useful ones. The various managers might be useful for example, maybe a generic pluginManager() one if all follow the same service naming pattern.

Anything left to do before we can get this in as a first step?

Status:Needs review» Needs work

The last submitted patch, drupal-service-1937600-23.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.15 KB
new16.48 KB
PASSED: [[SimpleTest]]: [MySQL] 53,053 pass(es).
[ View ]

For starters, I need to fix the tests ;)

+++ b/core/lib/Drupal.phpundefined
@@ -140,4 +140,135 @@ public static function lock() {
+  public static function config($name) {
...
+  public static function queue($name, $reliable = FALSE) {
...
+  public static function keyValue($collection) {
...
+  public static function state() {
...
+  public static function httpClient() {
...
+  public static function entityQuery($entity_type, $conjunction = 'AND') {
...
+  public static function flood() {
...
+  public static function moduleHandler() {
...
+  function typedData() {

Here's the complete list of services this patch adds.

+++ b/core/includes/update.incundefined
@@ -1237,7 +1237,7 @@ function update_retrieve_dependencies() {
-  foreach (drupal_container()->get('keyvalue')->get('system.schema')->getAll() as $module => $schema) {
+  foreach (Drupal::keyValue('system.schema')->getAll() as $module => $schema) {
+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/fetcher/DefaultFetcher.phpundefined
@@ -30,7 +30,8 @@ class DefaultFetcher implements FetcherInterface {
-    $request = drupal_container()->get('http_default_client')->get($feed->url->value);
+    // @todo: Inject the http client.
+    $request = \Drupal::httpClient()->get($feed->url->value);
+++ b/core/modules/update/lib/Drupal/update/Tests/UpdateCoreTest.phpundefined
@@ -201,7 +201,7 @@ function testFetchTasks() {
-    $queue = queue('update_fetch_tasks');
+    $queue = \Drupal::queue('update_fetch_tasks');

The patch apparently converts httpClient(), keyValue(), and queue(), but not the rest, so we'll want followups for the others (which will touch lots of files and break patches). Also, does that @todo need/have a followup?

+++ b/core/lib/Drupal.phpundefined
@@ -140,4 +140,135 @@ public static function lock() {
+  /**
+   * Returns the flood instance.
+   *
+   * @return \Drupal\Core\Flood\FloodInterface
+   */
+  public static function flood() {
+    return static::$container->get('flood');
+  }

Do we actually want a method for this one? I thought this was our example of "not used enough so just use Drupal::service()".

+++ b/core/lib/Drupal.phpundefined
@@ -140,4 +140,135 @@ public static function lock() {
+  function typedData() {

Should have a "public static" (the other methods on the class do).

flood() comes down to whether or not we want to use that as a way to encourage more people to use it. Personally I don't think so, because we want to encourage people to use it as an injected service. Currently there's only 2 uses in core, I think, one of which is about to get turned into an injected service by one of the WSCCI-conversion patches. It's definitely one of the less-likely core services to expose this way.

I've also been toying with allowing an arbitrary URL to get flood-protected via a route setting, but I don't know how feasible that is yet.

Actually, I think httpClient is probably another one that is unlikely to be needed. How often are we going to be making Guzzle requests directly from a hook? Page callbacks are turning into controllers and those are injectable (work is in progress as we speak), so in any of those you can get it injected.

moduleHandler makes sense. The generator should probably get added. (It's there now, even if we're only sort of using it.)

Drupal::flood() might not really be required, depends on the rules on which we add or not add things. There was quite a discussion in the previous issue about that already, it's surprisingly quite in here at the moment compared to that ;) I'm not sure if adding Drupal::flood() really hurts the injection-adoption (not sure if that's a word :p), I think it might actually raise awareness of the API which is a good thing as @catch pointed out in the other issue.

I actually think that httpClient() is a relatively often used one. Sure, most usages in core should become injected at some point, but I doubt that's going to happen for most of them in 8.x. See http://drupal.org/project/issues/drupal?text=guzzle, we have usages in batch callbacks, xml-rpc api functions (I very much doubt that anyone is going to convert that into a service), lots of tests and so on. And, I've written a bunch of conversions using drupal_container() and it was way harder than it's supposed to be because I always had to look up the classes instead of being able to rely on autocomplete.

The @todo indirectly has a follow-up with #1863816: Allow plugins to have services injected. I assume once there is an aggrement on how that should happen, that issue will result in follow-ups to convert our existing plugins.

Will create the follow-ups and re-roll.

So far no one has said they disagree with #7... does anyone? And also, the next comment shows that 100 people agree with it - although they are all named webchick :)

If that's the case, then the answer is simple: yes to both Drupal::flood() and Drupal::httpClient().

And also, the next comment shows that 100 people agree with it - although they are all named webchick :)

LOL :D

Yeah, I don't get the contention around saying yes to both. Or at least, to counter it, I've no idea how you'd explain to a new Drupal developer why public facing API A (database) was made available as a first-class service, but public facing API B (lock/httpClient) was not. They're going to expect consistency, and that all of these various low-level public facing APIs are treated the same.

Okay, but we just need to update the use of flood in the original patch then, and find a different example for the change notification etc. :)

flood() comes down to whether or not we want to use that as a way to encourage more people to use it. Personally I don't think so, because we want to encourage people to use it as an injected service.

D8 will still have procedural code and everything that is necessary in a service will be just as necessary outside of them. Trying to say, "you don't really need that unless you're a service" is a complete fallacy at this point.

I agree with @David_Rothstein in #30 and #7. This isn't about how obscure or how frequently an API is used, it's whether it's a public API or not.

If that's the case, then the answer is simple: yes to both Drupal::flood() and Drupal::httpClient().

+1

Although it's clear that I'm in the "more is better" camp, I was definitely wrong above in saying that all services should be aliased like this. If something is internal that has no purpose outside it's particular situation then it's not helpful to alias it. Not because it's obscure but because it's just plain useless outside a particular scope.

@catch created #1947720: Use Drupal::state() to replace state(), will create more followups later.

Issue summary:View changes

Updated issue summary.

StatusFileSize
new396 bytes
new16.3 KB
FAILED: [[SimpleTest]]: [MySQL] 53,295 pass(es), 12 fail(s), and 0 exception(s).
[ View ]

I thought this was our example of "not used enough so just use Drupal::service()".

I think the conclusion from the recent discussion here is that there might not be such an example. I would just state something like "In procedural code, use the provide methods like Drupal::flood(). Note most of these methods are simple wrappers and all services can also be accessed with Drupal::service('flood')" or something like that. There could be use cases for that, if e.g. the service name is dynamic but most if not all things that are interesting for procedural code will use a method.

I am not quite sure how to proceed. Attaching a re-rolled patch, see also the "Proposed solution" part that I added to the issue summary. This is not a complete list and IMHO doesn't need to be. For example, I left out the plugin managers. I'm not sure if we want a global one for those as a starting point (and enforce a plugin.manager.$something naming pattern for those, like we enforce it for cache now, there is no common interface for additional methods that they have however) and other things that I haven't thought of yet.

Not sure what kind of follow-up's I should create exactly. We already have #1938334: META: Replace uses of drupal_container() for those that directly access drupal_container(), maybe an additional one for each function that we have right now and will replace with a Drupal::$method()? i don't think it makes sense to split it up per module/file as most will basically be a single search & replace (+ some manual fixes for those that are currently in classes).

maybe an additional one for each function that we have right now and will replace with a Drupal::$method()?

Yep, let's do that next after this. To spare confusion, though, let's convert the Drupal::service('flood') from #1875086: Improve DX of drupal_container()->get() to Drupal::flood() here in this issue?

i don't think it makes sense to split it up per module/file as most will basically be a single search & replace (+ some manual fixes for those that are currently in classes

This really depends. A "simple search-and-replace" is easy to roll, but if it is a big patch and touches a lot of files, it's much harder to review and more likely to break other patches. See for example #500866: [META] remove t() from assert message, which stalled for years until we broke it up per-module. Plus, this may also allow us to get rid of some procedural wrappers we added to get around the previous DX issue. So maybe better to go over it in chunks.

Let's also file a specific followup for the plugin managers if we haven't already because they crop up a lot.

Issue summary:View changes

Updated issue summary.

- Added a follow-up for the plugin managers
- Drupal::service('flood') has not been converted (actually, it has been reverted) because of the issue linked in #22 which converts it to injected dependencies in a controller.

Status:Needs review» Needs work

The last submitted patch, drupal-service-1937600-35.patch, failed testing.

Status:Needs work» Needs review

We're only talking about adding Drupal::fooBarService() methods for services added in /includes and /Drupal/Core, not for services provided by modules ?

E.g;
the field_info_field(), field_info_instances()... functions are currently wrappers around methods provided by the FieldInfo class. #1950088: Move FieldInfo class to the DIC will move that class to a service in the container, and remove the wrapper functions.
Will this be accessed through Drupal::fieldInfo(), or Drupal::service('field.info') ?

The idea in the original issue was that modules would add their own class with static methods, so you could do Field::fieldInfo() and Field::instanceInfo().

As you can see in the discussion in the original issue, no agreement was found where that class should live exactly, my suggestion was and still is \Drupal\field\Field as that just works and doesn't need any additional locations to look for classes.

StatusFileSize
new16.56 KB
PASSED: [[SimpleTest]]: [MySQL] 53,385 pass(es).
[ View ]

Grr, was rebasing an old version. This should fix the tests again, interdiff is the same as #25.

Also, in response to @28, talked with @katbailey about a generator method and it will get added in the generator issue.

Status:Needs review» Reviewed & tested by the community

The current patch looks great to me.

IMHO, we should stop spending so much time on debating whether usage of a particular wrapper should be "encouraged" or "discouraged". That's a fruitless discussion for D8 either way. Premature and preemptive discouragements and omissions/removals will only cause a bad DX for developers that happen to need a service in a situation we didn't imagine. I'd rather suggest to add more than less. It's not like this would cost us anything.

It actually appears as if there are follow-up issues for doing actual conversions, and the current patch seems to be relatively complete, so let's move forward?

Issue tags:+Needs followup

As requested, here's the list of services/methods added/discussed in this issue, in three groups.

Added as a method and already converted as an example (can be considered done)

* Drupal::queue()
* Drupal::httpClient()
* Drupal::keyValue()

Added as a method but not yet converted/removed the old function (Needs follow-up issue per method if not yet existing to convert existing calls for the eventually existing old function/drupal_container() and removal of the eventually existing function)

* Drupal::config()
* Drupal::state() (#1947720: Use Drupal::state() to replace state())
* Drupal::entityQuery()
* Drupal::flood() (conversion issue should possibly be postponed on #1938390: Convert contact_site_page and contact_person_page to a new-style Controller)
* Drupal::moduleHandler()
* Drupal::typedData()

Not (yet) added (Needs an issue to discuss/add if not yet existing)

* url generator (#1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence)
* request (Requested to be left out by @Crell, @msonnabaum disagreed (I think?) in IRC, let's fight! in a follow-up issue)
* plugin managers (#1950670: Decide what methods should be added for plugin managers to \Drupal)

I'm not adding any services that are not supposed to be called directly/manually (like event listeners, compiler passes, route access checks, ...). Also haven't added module specific services yet. We have not yet discussed what to do with that after the initial discussion in the original issue.

Once those specific cases are done, #1938334: META: Replace uses of drupal_container() can be unpostponed to get rid of the remaining calls, which should only be a few remaining ones. And we can always add more methods if we find more useful ones later on.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Status:Reviewed & tested by the community» Needs work

needs a re-roll...

error: patch failed: core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/fetcher/DefaultFetcher.php:31
error: core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/fetcher/DefaultFetcher.php: patch does not apply

Status:Needs work» Needs review
StatusFileSize
new16.56 KB
PASSED: [[SimpleTest]]: [MySQL] 54,055 pass(es).
[ View ]

due to aggregator processors and parses to plugins commit

Status:Needs review» Reviewed & tested by the community

Re-roll looks good if testbot agrees.

We will find out in about 30 mins, but looks good to go :)

Status:Reviewed & tested by the community» Needs work

Some very minor inconsistencies with leading slashes in comments... lets clean this up since all the doc blocks are together now...

+++ b/core/lib/Drupal.phpundefined
@@ -167,4 +167,135 @@ public static function lock() {
+   * @return Drupal\Core\Config\Config

needs a leading \

+++ b/core/lib/Drupal.phpundefined
@@ -167,4 +167,135 @@ public static function lock() {
+   *   defined. Defaults to Drupal\Core\Queue\System, a reliable backend using

Needs a leading slash

+++ b/core/lib/Drupal.phpundefined
@@ -167,4 +167,135 @@ public static function lock() {
+   *   Drupal\Core\Queue\System.

Needs a leading slash

+++ b/core/lib/Drupal.phpundefined
@@ -167,4 +167,135 @@ public static function lock() {
+   * @return Drupal\Core\Queue\QueueInterface

Needs a leading slash

+++ b/core/lib/Drupal.phpundefined
@@ -167,4 +167,135 @@ public static function lock() {
+   * @return Drupal\Core\KeyValueStore\KeyValueStoreInterface

Needs a leading slash

+++ b/core/lib/Drupal.phpundefined
@@ -167,4 +167,135 @@ public static function lock() {
+   * @return Guzzle\Http\ClientInterface

Needs a leading slash

Status:Needs work» Needs review
StatusFileSize
new2.72 KB
new16.95 KB
PASSED: [[SimpleTest]]: [MySQL] 53,866 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs followup

Yep, that looks better, sorry for not catching that before. Also removing the needs followup tag, I think we got them all.

Title:Determine what services to register in the new Drupal classChange notice: Determine what services to register in the new Drupal class
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

Committed 7aca995 and pushed to 8.x. Thanks!

We need to update the same change notices as #1875086-173: Improve DX of drupal_container()->get()

Title:Change notice: Determine what services to register in the new Drupal classDetermine what services to register in the new Drupal class
Status:Active» Fixed

Updated those as much as I already could.

Also opened two more follow-ups:
- #1961542: Add Drupal::transliteration() and replace calls to transliteration service with it
- #1961548: [Policy, no patch] Decide how the module-specific services are made available as static methods (similar to \Drupal::$service())

What I didn't find was an change notice about the introduction of the Drupal class. http://drupal.org/node/1539454 should probably be extend a bit and like to that class on api.drupal.org and explain it with a sentence or two?

Status:Fixed» Closed (fixed)

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

Issue tags:-Needs change record

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

Issue summary:View changes

Updated issue summary.