Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
FILE: /var/www/html/drupal_core/core/modules/jsonapi/src/Controller/EntryPoint.php
-------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------
107 | WARNING | User::load calls should be avoided in classes, use dependency injection instead
Comment | File | Size | Author |
---|---|---|---|
#16 | 3115772-16.patch | 1015 bytes | saurabh.rocksoul |
#16 | interdiff_15-16.txt | 668 bytes | saurabh.rocksoul |
#15 | 3115772-15.patch | 1.46 KB | saurabh.rocksoul |
#15 | interdiff_12-15.txt | 2.02 KB | saurabh.rocksoul |
#12 | interdiff_10-12.txt | 424 bytes | swatichouhan012 |
Comments
Comment #2
swatichouhan012 CreditAttribution: swatichouhan012 at Valuebound for Valuebound commentedComment #3
swatichouhan012 CreditAttribution: swatichouhan012 at Valuebound for Valuebound commentedComment #4
Wim LeersThis would be a great novice patch :)
Why is this marked
9.0.x
?Comment #5
Wim LeersComment #6
saurabh.rocksoulThanks @swatichouhan012 for creating the issue.
Here is the patch for the same. Please review.
Comment #7
swatichouhan012 CreditAttribution: swatichouhan012 at Valuebound for Valuebound commentedThanks @Wim Leers @saurabh.rocksoul to check this issue. @Wim Leers i tagged this with 9.0.x because i checked in latest version should i check this with another one and then reroll ?
Comment #8
Wim LeersThis is updating the constructor, but not its documentation. Could you update that too? :)
In Drupal core, we don't camelcase function arguments. So it should not be
$entityTypeManager
, but$entity_type_manager
.#7 I think it can go in
8.9.x
too — I think it can be committed to both8.9.x
and9.0.x
😊Comment #9
swatichouhan012 CreditAttribution: swatichouhan012 at Valuebound for Valuebound commentedComment #10
swatichouhan012 CreditAttribution: swatichouhan012 at Valuebound for Valuebound commentedI have updated code wrt 8.9.x and 9.0.x , also removed unused use case, kindly review.
Comment #11
Wim LeersThe naming here is incorrect. This should be
$entityTypeManager
. (I know, it's weird and confusing, but all of Drupal does it this way for historical reasons!)The naming here is correct. That's indeed what I asked in #8.2 :)
This is correct too :)
Just one more reroll! 🤞
Comment #12
swatichouhan012 CreditAttribution: swatichouhan012 at Valuebound for Valuebound commented@Wim Leers i have updated patch according comment #11, kindly review.
Comment #13
Wim LeersPerfect! :)
Thank you, and thanks for your patience, @swatichouhan012!
Comment #14
alexpottWe need to account for backwards compatibility - ie. we need to allow passing in a NULL for $entity_type_manager... but this is extending ControllerBase - so we could avoid this and do
$this->entityTypeManager()->getStorage('user')->load()
instead.Also I think we need to think about scope here. Given that you're using a code scanner and coder rule to find this I don;t think a scope of 1 issue per warning is the way to go.
Comment #15
saurabh.rocksoulHey @alexpott, Thanks for pointing out the ControllerBase class. I have chnage the code keeping in mind this class.
Please review.
Comment #16
saurabh.rocksoulForgot to remove a comment from the code.
Uploading new patch. Please review this.
Thanks.
Comment #17
Wim Leers#14:
Oh, hah, didn't realize it already lived on
ControllerBase
😅🙈 Agreed with using that instead. (Now irrelevant, but I disagree with the need for BC forEntryPoint
's constructor: it's a controller owned by the JSON:API module and marked as@internal
.)Marking RTBC for the code changes.
Agreed — but in this case they didn't file dozens of issues for dozens of core modules. So … I think this is okay. But you're used to dealing with this much more often, so I obviously defer to you. Feel free to un-RTBC over this and ask to expand the scope to all of core of course.
Comment #22
xjmOK, that turned out to be very simple. :)
It's a very small cleanup, and usually I would prefer to see this done as a part of the coding standards cleanup initiative, where we replace the various
load()
calls across all of core according to the type of issue. In order to fix core coding standards in a maintainable way, all our coding standards issues should be done on a per-rule basis across all of core, rather than fixing standards in individual modules or files. We should also separate fixes where we need to write new documentation from fixes where we need to correct existing standards. This all should be done as part of #2571965: [meta] Fix PHP coding standards in core. A good place to start is the child issues of #2572645: [Meta] Fix 'Drupal.Commenting.FunctionComment' coding standard.For background information on why we usually will not commit coding standards fixes that aren't scoped in that way, see the core issue scope guidelines, especially the note about coding standards cleanups. That document also includes numerous suggestions for scoping issues including documentation coding standards cleanups.
Contributing to the overall plan above will help ensure that your fixes for core's coding standards remain in core the long term.
So, usually I would mark this as a duplicate of a large issue, but this one time I'll go ahead and commit it, since it's done. And since the change should be purely transparent, it's safe to backport during beta, so backporting as far as 8.9.x.
Thanks! I encourage you to submit complete fixes in the future, rather than one-line fixes.