- Issue created by @renatog
- @renatog opened merge request.
- Status changed to Needs review
about 2 years ago 7:08pm 12 March 2023 - 🇧🇷Brazil renatog Campinas
- Status changed to Closed: won't fix
about 2 years ago 10:27pm 12 March 2023 - 🇦🇺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 10:29am 13 March 2023 - 🇧🇷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 10:35am 13 March 2023 - 🇦🇺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