Problem/Motivation

I have noticed that some classes that extend from ControllerBase still have `entity_type.manager` (or similar) injected but later they are not used, as the function provided by the parent class is used instead.
therefore, we can avoid inject them.

In addition, the construct params and variables can be declared directly, so we can use this issue to review them.

Remaining tasks

code it, push it, review it and merge :)

Issue fork group-3494986

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

gorkagr created an issue. See original summary.

gorkagr’s picture

I am not sure if you have also planned to do smth in the constructors and declare the variables directly in the params:

smth like
original:


  /**
   * The private store factory.
   *
   * @var \Drupal\Core\TempStore\PrivateTempStoreFactory
   */
  protected $privateTempStoreFactory;

  /**
   * The renderer.
   *
   * @var \Drupal\Core\Render\RendererInterface
   */
  protected $renderer;

  /**
   * Constructs a new GroupController.
   *
   * @param \Drupal\Core\TempStore\PrivateTempStoreFactory $temp_store_factory
   *   The private store factory.
   * @param \Drupal\Core\Render\RendererInterface $renderer
   *   The renderer.
   */
  public function __construct(PrivateTempStoreFactory $temp_store_factory, RendererInterface $renderer) {
    $this->privateTempStoreFactory = $temp_store_factory;
    $this->renderer = $renderer;
  }

idea:


  /**
   * Constructs a new GroupController.
   *
   * @param \Drupal\Core\TempStore\PrivateTempStoreFactory $temp_store_factory
   *   The private store factory.
   * @param \Drupal\Core\Render\RendererInterface $renderer
   *   The renderer.
   */
  public function __construct(
    protected PrivateTempStoreFactory $privateTempStoreFactory,
    protected RendererInterface $renderer,
  ) {
  }

But then i assume that should be done to all files that have a __construct(), so all of them are unified

gorkagr’s picture

Status: Needs work » Needs review
kristiaanvandeneynde’s picture

Uh yeah, property promotion and the omission of constructor docblocks is also on my todo list

kristiaanvandeneynde’s picture

Status: Needs review » Closed (won't fix)

Actually, we should still inject them because if we call the parent method without injecting them, they will use \Drupal::service() instead. We should absolutely avoid using that where we can.

Closing as won't fix as I've just upgraded all constructors to use property promotion where possible instead. See #3506163: Drop constructor documentation and use property promotion where possible