[Resource Owners] Ineffective way to add a custom Resource Owner

[Taluu]

Looking at the code (even on 0.4), I think the way custom resource owners are handled in a really inefficient way.

Indeed, it is currently working through only aliases, without any checks on them ("how to ensure they implement the right interface ?" is the main question I'm thinking of right now, as I don't have any contracts on it), and also in a really hazardous way (declaring a new alias, with a specific name, and interpoling things from it)... when a tag (and a compiler pass) should be way more appropriate, such as <tag name="hwi.oauth.resource_owner" name="XYZ" /> for example.

And that also applies even more if I want to add some options to this owner, as does the provided resource owners ; it it impossible to do so, as the config doesn't allow any option handling when the service parameter is used. As there is an interface, it should be able to see that is it configurable through options, and thus be able to change some things through a conifigureOptions method... Which is set in the AbstractResourceOwner class.

I think a huge refacto is needed here, on the Resource Owner handling at least, on the DependencyInjection part.

So, the points are the following ;

• Check that the ResourceOwnerInterface is implemented when using a custom resource owner service
• Base the resource owners detection on a tag rather than a fixed set of known resource owners and a wonky alias handler
• Do not forbid to add options or other parameters when using a service rather than a built-in owner (this is linked to the previous point)

then the resource owner mechanism should really be easily extensible

[stof]

OK, I started to review the PR, and some points in them are about points you suggested here, which will be true for any other way you try to achieve it:

Check that the ResourceOwnerInterface is implemented when using a custom resource owner service

The DI extension cannot access other services. So the validation cannot be done att this point (we could validate it later, bbut most bundles don't do it, relying on the PHP exceptions thrown later anyway because of typehint to keep things simpler)

Do not forbid to add options or other parameters when using a service rather than a built-in owner (this is linked to the previous point)

This means that your bundle will alter the definition of a service defined elsewhere, and without knowing how this service looks like (because it is a service defined by someone depending on the HWI bundle, which is not known, not a service coming from a dependency of the bundle). This looks bad because it forces to make particular assumptions about how this service looks like, removing flexibility for the user. It also means you will try to overwrite some config set by the user of your bundle, which is the wrong priority. This kind of behavior is not consistent at all with the way Symfony deals with service configuration.

[Taluu]

I see where you're getting at on the typehints, and even if this is indeed not rigorous, it is still a valid point of view, and should also only happen on development, which is fine I guess.

The dependency injection is already broken on the compiler pass which works on the aliased services though, as it assumes that a setName exists... If your custom resource owner extends the AbstractResourceOwner, your service declaration must duplicate the AbstractResourceOwner (or the oauth1's one in my case) declaration, which I think is also kind of bad.

I may have went the wrong way (this is really plausible), but I think the handling through an alias is not that great, and also the blocking of the possiblity of having more options for a service. How to inject them is indeed the right question though...

[stof]

The actual question is whether the resource owner should actually be aware of the name used to reference it.

[Taluu]

I see where you're getting at on the typehints, and even if this is indeed not rigorous, it is still a valid point of view, and should also only happen on development, which is fine I guess.

The dependency injection is already broken on the compiler pass which works on the aliased services though, as it assumes that a setName exists... If your custom resource owner extends the AbstractResourceOwner, your service declaration must duplicate the AbstractResourceOwner (or the oauth1's one in my case) declaration, which I think is also kind of bad.

I still think the handling through an alias is not that great (and too limited), as is the absence of a possibilty to easily configure more options for a service which is really blocking. How to inject them is indeed the right question though...

And I agree for the name, I don't think the resource owner should be aware of it, as the Definition in Symfony's DI component are not aware of their given name. Maybe a type (google for google for example) would be ok I guess ?

edit : reopening, didn't want to close it before

[github-actions[bot]]

Message to comment on stale issues. If none provided, will not mark issues stale

[github-actions[bot]]

This issue was closed because it has been stalled for 5 days with no activity.