Allow the client plugin to instantiate a different provider

Note, I was able to verify that the Azure provider works fine with the changed return type that I patched in locally as a quickfix (still on 8.x-3.x, so the patch is not directly applicable to 4.x)

  public function getProvider(): AbstractProvider {
    return new Azure(
      [
        'clientId' => $this->getClientId(),
        'clientSecret' => $this->getClientSecret(),
        'redirectUri' => $this->getRedirectUri(),
        'urlAuthorize' => $this->getAuthorizationUri(),
        'urlAccessToken' => $this->getTokenUri(),
        'urlResourceOwnerDetails' => $this->getResourceUri(),
        'scopes' => $this->getScopes(),
        'scopeSeparator' => $this->getScopeSeparator(),
        'defaultEndPointVersion' => '2.0',
        'tenant' => 'XYZ',
      ],
      $this->getCollaborators()
    );
  }

The endpoint version and tenant is important to specify, it then does not use the provided authorize and token URLs at all but dynamically fetches that through the openid meta API of microsoft, so could skip that in the definition, but I just copied that part and was too lazy to test that.

Here is a patch against 4.x. Since this method is on the interface, this is a bit tricky with BC, but since it's a base class, it apparently seems to work just fine, see also https://3v4l.org/Kd0dT. classes and subclasses can define a more specific return type.

What a great catch @Berdir

AbstractProvider isn't an interface but it's as close as this library provides so I should have thought of that from the beginning.

With regard to BC, I agree that since the current interface return type is a child of your proposed interface return type, it should be fine. Also we can further note it on the release notes.

Okay, I'm looking at your patch more closely. I think all we need to change is the return type in Oauth2ClientPluginBase since GenericProvider is an instance of AbstractProvider.

As a small test, I opened the code in phpStorm, changed the method in Oauth2ClientPluginBase to read

public function getProvider(): AbstractProvider {
    return new GenericProvider(
      [
        'clientId' => $this->getClientId(),
        'clientSecret' => $this->getClientSecret(),
        'redirectUri' => $this->getRedirectUri(),
        'urlAuthorize' => $this->getAuthorizationUri(),
        'urlAccessToken' => $this->getTokenUri(),
        'urlResourceOwnerDetails' => $this->getResourceUri(),
        'scopes' => $this->getScopes(),
        'scopeSeparator' => $this->getScopeSeparator(),
      ],
      $this->getCollaborators()
    );
  }

And then inserted an override in one of the example plugin classes with a signature like so:

 public function getProvider(): GenericProvider

And phpStorm code analysis is happy with that given the inhertance. This would allow a plugin such as yours to extend the base class and use an AbstractProvider but existing plugins that might be overriding this and already have a signature that matches the current base class would not be disturbed.

Does this sound right to you?

That's what my patch does, no?

For that to work, the type hint must be removed on the interface. If you keep it there, then you get a fatal error, see https://3v4l.org/43cFO. The rules for the interface work differently.

And if you ever want to add it back to the interface as AbstractProvider, then all implementations must do that, so I'm already now changing all test implementations to do that. But existing implementations that keep using GenericProvider will work combined with the interface change: https://3v4l.org/gOdGZ.

That's a cool site for discussing code! As I read your patch, you propose to remove the AbstractProvider from the interface. It's late in the day for me so maybe I'm missing something but I don't think we need to do that. Rather we can change the return type in the base class to match like this: https://3v4l.org/E1l6t

Also copying here as a canonical record of the discussion:

class AbstractProvider {
    
}

class GenericProvider extends AbstractProvider {
    
}

interface Oauth2ClientPluginInterface {
    
    public function getProvider(): AbstractProvider;
}

class OAuth2ClientPluginBase implements Oauth2ClientPluginInterface {
    public function getProvider(): AbstractProvider {
        
    }
}

class Oauuth2Plugin extends OAuth2ClientPluginBase  {
    public function getProvider(): GenericProvider {
        
    }
}

You are correct, that is indeed valid, I somehow assumed it isn't, so yeah, that doesn't need to be removed.

Note: If you select include EOL php versions, you can see that neither option works on PHP 7.3 and older, but that's fine as you require 8.1 for this major version.

Here's an updated patch with the single line change

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