drupal_container() is deprecated, and all calls in the update module need to be replaced with Drupal::service(), except for where the module_handler service is requested, which needs to be replaced with Drupal::moduleHandler() (see #1957154: Replace calls to drupal_container()->get('module_handler') service with Drupal::moduleHandler())

This task a part of #2001206: Replace drupal_container() with Drupal::service()

Files: 
CommentFileSizeAuthor
#6 drupal-remove-drupal-container-2014037-5.patch1.28 KBhussainweb
PASSED: [[SimpleTest]]: [MySQL] 56,172 pass(es).
[ View ]
#5 drupal-remove-drupal-container-2014037-5.patch0 byteshussainweb
PASSED: [[SimpleTest]]: [MySQL] 58,217 pass(es).
[ View ]
#2 update-2014037-2.patch1.33 KBkgoel
PASSED: [[SimpleTest]]: [MySQL] 55,426 pass(es).
[ View ]

Comments

kgoel’s picture

Assigned:Unassigned» kgoel
kgoel’s picture

Status:Active» Needs review
StatusFileSize
new1.33 KB
PASSED: [[SimpleTest]]: [MySQL] 55,426 pass(es).
[ View ]
dcam’s picture

Status:Needs review» Reviewed & tested by the community

#2 looks good. I didn't find any more uses of drupal_container() in the update module.

webchick’s picture

Status:Reviewed & tested by the community» Needs work

I think those should be Drupal::keyValue() instead.

hussainweb’s picture

Status:Needs work» Needs review
StatusFileSize
new0 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,217 pass(es).
[ View ]

I am attaching a patch altered as per @webchick's recommendations.

I am a little confused about this, though. I read here that it it is preferred to use the service container to instantiate objects. As per the following block:

  // The preferred way: dependency injected code.
  function hook_do_stuff() {
    // Move the actual implementation to a class and instantiate it.
    $instance = new StuffDoingClass(Drupal::lock());
    $instance->doStuff();

    // Or, even better, rely on the service container to avoid hard coding a
    // specific interface implementation, so that the actual logic can be
    // swapped. This might not always make sense, but in general it is a good
    // practice.
    Drupal::service('stuff.doing')->doStuff();
  }

Calling Drupal::keyValue() seems to me to be a direct implementation detail. Of course, I checked the implementation and see that it just calls the container. I am not sure which is the better method. However, like I said, I am using Drupal::keyValue() as per @webchick's recommendation, but I would like someone to throw some light on this.

hussainweb’s picture

StatusFileSize
new1.28 KB
PASSED: [[SimpleTest]]: [MySQL] 56,172 pass(es).
[ View ]

Sorry about the empty patch. Here is the correct one.

Bober’s picture

Status:Needs review» Reviewed & tested by the community

Seems good for me.

alexpott’s picture

Title:Replace drupal_container() with Drupal::service() in the update module» Replace drupal_container() with Drupal::keyValue() in the update module
Status:Reviewed & tested by the community» Fixed

Committed fc35679 and pushed to 8.x. Thanks!

kgoel’s picture

Status:Fixed» Closed (fixed)
alexpott’s picture

Status:Closed (fixed)» Fixed

@kgoel the move to closed(fixed) will happen automatically after 2 weeks without further comments.

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