Discovered as part of #2935947: Comply with code standards

HttpExceptionNormalizer and others should type hint to AccountInterface, not AccountProxyInterface.

CommentFileSizeAuthor
#5 2936185-5.patch5.47 KBgabesullice

Comments

gabesullice created an issue. See original summary.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

I am missing the why here. Can you elaborate?

gabesullice’s picture

gabesullice
Primary language English
commented
Assigned: Unassigned » gabesullice
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

My guess: because interface AccountProxyInterface extends AccountInterface {…} — i.e. AccountInterface is the base interface. Unless JSON API calls any of the methods that AccountProxyInterface layers on top, we should just typehint to the base interface.

gabesullice’s picture

gabesullice
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new 2936185-5.patch5.47 KB

@Wim Leers, yep. And I admit it's a knee-jerk reaction that I've built up working (and testing) a lot access related code that isn't obviously valuable in this module. Just a best-practice earned from experience.

The practical reason is this:

  1. AccountProxy is the only class that implements AccountProxyInterface.
  2. But both AccountProxyInterface and UserInterface implement AccountInterface.
  3. Therefore, when you use AccountInterface, you can either pass the account you have or a user entity... which is the whole point of interfaces in the first place :)
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

#5 is correct. And this would definitely also come up when we try to move this module into Drupal core!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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