Replace Term::loadMultiple to $this->entityTypeManager

Created on 12 March 2023, about 2 years ago
Updated 13 March 2023, about 2 years ago

Problem/Motivation

Testing the HM I verified that on HmTaxonomyController there is a call to Term::loadMultiple($tids) which works, but it can be improved to use dependency injection. So DrupalBest practice is returning this error: Term::loadMultiple calls should be avoided in classes, use dependency injection instead

Steps to reproduce

Run DrupalBest Practice standard on phpcs on HmTaxonomyController

Proposed resolution

We can use the same solution of #3104298: Term::load calls should be avoided in classes .
On that case we already have the EntityTypeManager as dependency on this controller so we can just use $this->entityTypeManager->getStorage('taxonomy_term')->loadMultiple($tids); to fix this

Remaining tasks

User interface changes

API changes

Data model changes

Feature request
Status

Closed: won't fix

Version

3.0

Component

Code

Created by

🇧🇷Brazil renatog Campinas

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @renatog
  • @renatog opened merge request.
  • Status changed to Needs review about 2 years ago
  • Status changed to Closed: won't fix about 2 years ago
  • 🇦🇺Australia mingsong 🇦🇺

    Term:loadMultiple is working fine in Drupal 10.
    Reference:
    https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...

  • Status changed to Needs review about 2 years ago
  • 🇧🇷Brazil renatog Campinas

    Hello @Mingsong. Sorry I expressed wrongly

    I know that the “loadMultiple()” works fine. My suggestion is just to use dependency injection instead of Term::loadMultiple you know?

    Following this recommendation:

    Term::loadMultiple calls should be avoided in classes, use dependency injection instead

    With the fix on my PR we’re using $this-> and we continue with loadMultiple. I tested and the result continue the same. But now with dependency injection 😊

  • Status changed to Closed: won't fix about 2 years ago
  • 🇦🇺Australia mingsong 🇦🇺

    Term::loadMultiple calls should be avoided in classes, use dependency
    injection instead.

    Where is above comes from?

    It is working well meaning no need to change.

    That is even used in the core as exactly the same way. Should Drupal core replace it as it is “good practice“?

  • 🇧🇷Brazil renatog Campinas

    Where is above comes from?

    To reproduce we can run:
    phpcs --standard=DrupalPractice src/Controller/HmTaxonomyController.php

    Result:

    FILE: hierarchy_manager/src/Controller/HmTaxonomyController.php
    ---------------------------------------------------------------------------------------------------------
    FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
    ---------------------------------------------------------------------------------------------------------
     307 | WARNING | Term::loadMultiple calls should be avoided in classes, use dependency injection instead
    ---------------------------------------------------------------------------------------------------------
    

    About this example from Drupal core it's a commit from ~4 years ago. So I'd say that the fact that have one example on Drupal core don't mean that is the "ideal". Drupal core also have issues tracked to solve this kind of issues. For example: 🌱 [meta] Fix PHP coding standards in core Active

    For this reason the #3 fixed at least in our side, you know?

  • 🇦🇺Australia mingsong 🇦🇺

    I did understand you meant the report generated by phpcs. If you used phpcs to scan Drupal core, you also would see many warnings against Drupal core. Would you raise issues to Drupal core?
    At this stage, I am not interested in making this change.

  • 🇧🇷Brazil renatog Campinas

    Would you raise issues to Drupal core?

    Issues reported in core at 🌱 [meta] Fix PHP coding standards in core Active

    At this stage, I am not interested in making this change

    Ok then :)

    Thanks @Mingsong

Production build 0.71.5 2024