Replace drupal_container() with injected services in the rest module

Guess i'll give this a try.

@DmitryDrozdik: Ok thanks. Just trying to do a small change and get it green so i get a feel for the API change.

Also @DmitryDrozdik does #1957154: Replace calls to drupal_container()->get('module_handler') service with Drupal::moduleHandler() have to be committed first before this issue in order for tests to get passed?

#1: 2011122-deprecate-drupal_service-in-rest-1.patch queued for re-testing.

The patch referenced from #3 is in, so this can move forward.

+++ b/core/modules/rest/rest.module
@@ -10,8 +10,8 @@
+  if (Drupal::service()->has('plugin.manager.rest')) {

Why do we need the conditional? If this module is enabled, that service should be there...

+++ b/core/modules/rest/rest.module
@@ -10,8 +10,8 @@
+    $manager = Drupal::service()->get('plugin.manager.rest');

This should be Drupal::service('plugin.manager.rest')

The test failed because of line 13 in rest.module.

  if (Drupal::service()->has('plugin.manager.rest')) {
    $manager = Drupal::service('plugin.manager.rest');
  ...
  }

Of course, one thought is to use Drupal::getContainer() instead of Drupal::service() (which is invalid). However, the documentation says that this should be avoided.

The next approach I can think of is to remove the call to has() completely, something like this:

  $manager = Drupal::service('plugin.manager.rest', ContainerInterface::NULL_ON_INVALID_REFERENCE);
  if ($manager) {
  ...
  }

I am not sure about the approach, though. Any better ideas?

Here is the patch with the fix suggested in #11.

#14: rest-2011122-14.patch queued for re-testing.

It appears that the error is here:

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/ResourceBase.php
@@ -68,7 +68,7 @@ public function routes() {
         case 'HEAD':
           // Restrict GET and HEAD requests to the media type specified in the
           // HTTP Accept headers.
-          $formats = drupal_container()->getParameter('serializer.formats');
+          $formats = \Drupal::service()->getParameter('serializer.formats');
           foreach ($formats as $format_name) {
             // Expose one route per available format.
             //$format_route = new Route($route->getPattern(), $route->getDefaults(), $route->getRequirements());

Drupal::service() expects a parameter. I am not sure how to support this particular case. Any ideas?

If you actually need the container itself, you call \Drupal::getContainer()->getParameter().

That said... Plugins can now have values injected with a create() method so we really really should inject the format list properly rather than calling \Drupal.

@Crell: I think that means adding a parameter to the constructor, right? I am not very sure if this counts as an API change. If it is an API change, then is it necessary to put it past the API freeze?

I am trying out the change from #20.

That said... Plugins can now have values injected with a create() method so we really really should inject the format list properly rather than calling \Drupal.

Add the following in rest/lib/Drupal/rest/Plugin/ResourceBase.php

public static function create(ContainerInterface $container) {
return new static($container->getParameter('serializer.formats'));
}

I don't think it is the matter of overriding the create() method. It seems that PluginBase::create() method already takes care of passing the container. Besides, passing in the serializer.formats list might just break the contract (the constructor should probably expect a ContainerInterface).

I think it is a matter of saving the container in the constructor and using the saved container instead of calling \Drupal::getContainer(). Am I on the right track here?

Ok, I found something. I am rewriting the patch based on Change Record: Plugins can receive injected dependencies from the container.

Here goes nothing... I am still not sure if this counts as an API change. If it does, and if its too late for that, I think patch in #21 still applies and passes all tests.

I guess I must define somewhere that these plugins will now need ContainerInterface parameter too. The change record didn't mention anything about it and I thought it would be picked up.

Any tips?

We absolutely do not save the container here. We only need to save the formats property. The constructor is not something that would ever be accessed outside of a unit test (at least we don't support doing so), so it's not an API change to inject it properly.

Thanks @Crell,

Can you tell me if my approach in #26 was at least in the right direction? I am looking into the code to see how to work with it. I guess that instead of saving the ContainerInterface, I will just save the formats list, but the problem is that I am being passed NULL for $container. How should I proceed?

Came up with this after analysis. I hope it works.

Wow, that was embarrassing. I changed the class but didn't update the namespace imports. Fixing it now...

There was some progress, so I made the change with injecting serializer.formats instead of the container. Let's see how this goes.

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/views/display/RestExport.php
@@ -75,9 +75,8 @@ class RestExport extends PathPluginBase {
     parent::initDisplay($view, $display, $options);
 
-    $container = drupal_container();
-    $negotiation = $container->get('content_negotiation');
-    $request = $container->get('request');
+    $negotiation = \Drupal::service('content_negotiation');
+    $request = \Drupal::request();
 
     $request_content_type = $negotiation->getContentType($request);

Looking at the Views plugin inheritance tree I can see that it leads back to ContainerFactoryPluginBase, so we can implement a create() method here as well that pulls the content negotiation from the container?

Made changes as per #36. I was torn between using $container::Request(); and $container->Request();. I just resorted to using $container->get('request') in the create() method. Lets see what happens.

It looks like this should fix the problem. The test result says that the function definition for RestExport::create() is not compatible with ContainerFactoryPluginInterface::create(), but it is. I think it is only the missing reference, though I was expecting a different error in that case.

Excellent!

I think that we should be changing ResourcePluginManager to use DefaultPluginManager.

Here goes nothing. I used examples from other modules. I hope it works.

I think this should fix it.

I see the problem, really silly of me. I am still unsure about one part of the change, but I think it should work.

  cache.rest:
    class: Drupal\Core\Cache\CacheBackendInterface
    tags:
      - { name: cache.bin }
    factory_method: get
    factory_service: cache_factory
    arguments: [rest]

This is in rest.services.yml and is necessary because we are passing a @cache.rest to our plugin manager. I see the same usage in block.services.yml and field.services.yml.

The earlier error (in #44) was due to swapped arguments. I just fixed that in this patch and the block above.

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/Type/ResourcePluginManager.php
@@ -8,32 +8,37 @@
+    parent::__construct('rest/resource', $namespaces);
     $this->discovery = new AlterDecorator($this->discovery, 'rest_resource');
     $this->discovery = new CacheDecorator($this->discovery, 'rest');
 
-    $this->factory = new ReflectionFactory($this->discovery);
+    $this->setCacheBackend($cache_backend, $language_manager, 'field_formatter_types');
+    $this->alterInfo($module_handler, 'field_formatter_info');

Field formatter info? Why? And are the decorators above still needed?

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/views/display/RestExport.php
@@ -70,16 +73,59 @@ class RestExport extends PathPluginBase {
   /**
+   * The content negotiation library.
+   *
+   * @var \Drupal\Core\ContentNegotiation
+   */
+  protected $content_negotiation;

class properties should use lowerCamel naming, see https://drupal.org/node/608152#naming

I am not too sure about the additional decorators. These were the only decorators not present in DefaultPluginManager::__construct() and I thought I should not change that here.

About the other two calls: I didn't pay attention when copying them from FormatterPluginManager. I don't know if they are necessary, although it looks like they are. I have changed them appropriately.

I have also changed the class property name as mentioned in #47.

Fixing more bugs...

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/ResourceBase.php
@@ -7,14 +7,51 @@
+  /**
+   * The available serialization formats.
+   *
+   * @var array
+   */
+  protected $serializer_formats = array();

and lowerCmale here too, please :-)

I don't know why I am making mistakes like this. I put all the use statements in another file and resubmitted the patch. Silly me.

And I changed the property name to camel case as indicated in #51. I hope there are no others. :)

Well, 5 errors is certainly better than 400+ errors. It seems that the Drupal\Core\Plugin\ContainerFactoryPluginBase has been removed recently. I will try searching for it and replace it appropriately.

Found it. Drupal\Core\Plugin\ContainerFactoryPluginBase has been removed in #2033447: Remove obsolete ContainerFactoryPluginBase and I am bringing the changes in here.

Hmmm... looks like Drupal\Core\Plugin\ContainerFactoryPluginBase did more than just the one abstract method. I have to recreate that method in ResourceBase, I think.

I am putting back extends PluginBase on ResourceBase as that class defines the properties and the methods and looks like the appropriate match here. That was the eventual class implementation in hierarchy when using ContainerFactoryPluginBase too, so with that latter becoming an interface now, we can revert to PluginBase.

This is a lot more patches than I thought. :-)

And we're back to green, gentlemen. Finally!

Please look at the approach when reviewing. The last patch before #58 to pass was #39. We changed the ResourcePluginManager to use DefaultPluginManager as per #41 and a few property names were changed from #47 and #51. As part of the changes, we found that ContainerFactoryPluginBase has since been removed since the patch in #39 (from #2033447: Remove obsolete ContainerFactoryPluginBase). I implemented the new method as per the change record: Allow plugins to have services injected.

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/ResourceBase.php
@@ -8,13 +8,51 @@
 
 /**
  * Common base class for resource plugins.
  */
-abstract class ResourceBase extends PluginBase implements ResourceInterface {
+abstract class ResourceBase extends PluginBase implements ContainerFactoryPluginInterface, ResourceInterface {

So now REST plugins need to have a create() method, otherwise there will be PHP fatal errors if a plugin only implements the ResourceInterface? That means the Resource Interface itself must extend from ContainerFactoryPluginInterface.

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/Type/ResourcePluginManager.php
@@ -8,32 +8,40 @@
+class ResourcePluginManager extends DefaultPluginManager {
...
+    parent::__construct('rest/resource', $namespaces);
     $this->discovery = new AlterDecorator($this->discovery, 'rest_resource');
     $this->discovery = new CacheDecorator($this->discovery, 'rest');
 
-    $this->factory = new ReflectionFactory($this->discovery);
+    $this->setCacheBackend($cache_backend, $language_manager, 'rest_plugins');
+    $this->alterInfo($module_handler, 'rest');

So the AlterDecorator is not needed anymore. And for alterInfo() we should pass 'rest_resource'.

Same for the CacheDecorator, that is also not needed anymore, because DefaultPluginManager has built-in caching.

Please copy the ResourcePluginManager stuff from #2047593: Inject dependencies into EntityDerivative derivative class, looks good there.

Thanks @klausi, I am traveling until Wednesday. I will resume working on this then.

It looks like it needs a reroll. So, I am not attempting any major changes as suggested in #62 yet. I am just rerolling and putting in suggestions from #61 for now. If it turns green, I will look at #2047593: Inject dependencies into EntityDerivative derivative class and work the changes.

The reroll were from conflicts in #2043379: Allow plugins to be discovered in any directory and #1972116: Add REST test coverage for nodes. The second issue was fairly straight-forward as it was only a one line change. The first was a little tricky but ultimately, I see I had to change this line: parent::__construct('rest/resource', $namespaces); to this: parent::__construct('Plugin/rest/resource', $namespaces); in core/modules/rest/lib/Drupal/rest/Plugin/Type/ResourcePluginManager.php.

This looks good to me now for replacing drupal_container(), injecting other dependencies should be handled in #2047593: Inject dependencies into EntityDerivative derivative class.

Committed 5031030 and pushed to 8.x. Thanks!

Fixed code indentation during commit...

diff --git a/core/modules/rest/rest.module b/core/modules/rest/rest.module
index 9f01f6a..d0e0551 100644
--- a/core/modules/rest/rest.module
+++ b/core/modules/rest/rest.module
@@ -12,12 +12,12 @@ function rest_permission() {
   $permissions = array();
   $manager = Drupal::service('plugin.manager.rest');
   $resources = config('rest.settings')->get('resources');
-    if ($resources && $enabled = array_intersect_key($manager->getDefinitions(), $resources)) {
-      foreach ($enabled as $key => $resource) {
-        $plugin = $manager->getInstance(array('id' => $key));
-        $permissions = array_merge($permissions, $plugin->permissions());
-      }
+  if ($resources && $enabled = array_intersect_key($manager->getDefinitions(), $resources)) {
+    foreach ($enabled as $key => $resource) {
+      $plugin = $manager->getInstance(array('id' => $key));
+      $permissions = array_merge($permissions, $plugin->permissions());
     }
+  }
   return $permissions;
 }