Problem/Motivation

Current user is being called from the static service container but could be injected instead as ActionBase implements ContainerFactoryPluginInterface

  /**
   * {@inheritdoc}
   */
  public function executeMultiple(array $entities) {
    $this->tempStoreFactory->get('user_user_operations_cancel')->set(\Drupal::currentUser()->id(), $entities);
  }

Proposed resolution

Inject the container instead.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task
Issue priority Not critical
Unfrozen changes Unfrozen because it only injects the current user into the plugin
Non prioritized changes The goal of this change is adding the current user service to the ActionBase plugin that is currently using the tempstore of the user, so currentUser would be a fairly common use case
Disruption Contributed/custom modules that implement an ActionBase. The change to implement is trivial, non disruptive
CommentFileSizeAuthor
#1 2400159-container_action_base.patch4.78 KBpcambra
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pcambra’s picture

pcambra
he/him
Primary language Spanish
Location Asturies
CreditAttribution: pcambra commented
Status: Active » Needs review
Issue tags: +Quick fix
FileSize
2400159-container_action_base.patch4.78 KB

Status: Needs review » Needs work

The last submitted patch, 1: 2400159-container_action_base.patch, failed testing.

pcambra’s picture

pcambra
he/him
Primary language Spanish
Location Asturies
CreditAttribution: pcambra commented
Status: Needs work » Needs review
jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs review » Reviewed & tested by the community

Thanks for the patch and issue.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

This issue is a minor task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.

We need to decide whether injection of dependencies is beta permissible or not - this one is especially interesting since it is a base class so any contrib modules that implement it will break.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Don't we want to use the account switcher here as well?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

https://www.drupal.org/node/2377441

pcambra’s picture

pcambra
he/him
Primary language Spanish
Location Asturies
CreditAttribution: pcambra commented
Issue summary: View changes
pcambra’s picture

pcambra
he/him
Primary language Spanish
Location Asturies
CreditAttribution: pcambra commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs issue summary update

As discussed with @dawehner on IRC, there's no user switching on this change so no need to use that bit.

Added the beta evaluation table and setting back to RTBC

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Yeah +1

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Category: Task » Bug report
Status: Reviewed & tested by the community » Fixed

Plugins should be properly injected with their services where possible. Committed b8b30d3 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed b8b30d3 on 8.0.x 
    Issue #2400159 by pcambra: Use the container for current user on all...

Status: Fixed » Closed (fixed)

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