Closes #3456738
Co-authored-by: Julian Pustkuchen <3110-Anybody@users.noreply.drupalcode.org>
Merge request reports
Activity
11 class UserAuthDecorator implements UserAuthInterface { 12 13 /** 14 * Constructs a UserAuthDecorator object. 15 * 16 * @param \Drupal\user\UserAuthInterface|\Drupal\user\UserAuthenticationInterface $inner 17 * The inner User.Auth service. 18 */ 19 public function __construct(protected UserAuthInterface|UserAuthenticationInterface $inner) { 20 } 21 22 /** 23 * {@inheritdoc} 24 */ 25 public function authenticate($username, #[\SensitiveParameter] $password) { 26 return $this->inner->authenticate($username, $password); -
Speaking of phpstan would have discovered the original issue:
UserAuthenticationInterface doesn't have this method. This only works because the core implementation of UserAuthenticationInterface actually also implements UserAuthInterface for BC reasons.
What I mean with that is that this decorator should only accept UserAuthInterface in its constructor. It will fail anyway once we remove the BC layer and it more accurately reflects how such an existing decorator in contrib/custom modules would look like.
Another missed win for PHPStan (which ironically is the exact issue core made and I repeated)
Bigger problem. What if someone decorates and only implements UserAuthenticationInterface?
UserAuthInterface->UserAuthenticationInterface->UserAuthInterface&UserAuthInterface (this is the core inner service) or vice versa where the outer service only implements UserAuthenticationInterface?
Will Symfony error out in that case during the container build ? (I honestly don’t even understand how it is working now, I thought decorators had to implement all interfaces of a service being decorated) and is that even what we want ? What is the future impact on being able to be compatible with D11 and D12 at same time?
Seems like we almost need UserAuthenticationInterface to extend UserAuthInterface (with the intention that in D12 that parent relationship will be removed and implementers will have a stray method to clean up)
-
This is a test module, it doesn't matter for this.
Yes, for a contrib implementation, it could be an issue, but there's only so much that core can do about that. There are many scenarios that aren't going to work, like module A replacing the service with the old interface, and module B injecting only the new UserAuthenticationInterface. But honestly, getting an error in such a situation is better than it silently not working. If core would provide BC with a new service, then email authentication would just silently not work then for the login stuff that module B is doing. Either way, one of those modules will need to be adjusted/patched then in that scenario.
Code has been merged, closing.
- 11.x: e015f80827
- 11.0.x: 8a748ac031
- 10.4.x: 8f08ecc0e1
- 10.3.x fee6f4e082
mentioned in merge request !8534 (closed)