Generalized access token format

[lookyman]

Replacement for #873

[simonhamp]

We could avoid a BC break if getResponseString was a new method that called convertToJWT in AccessTokenTrait.php. In case any implementors are relying on convertToJWT's existence.

[lookyman]

We still need to call that new method from all places that currently call the old one though. Which means adding it to the access token interface, which means BC break if someone doesn't use the trait and just implements the interface.. But sure, I can do that.

[simonhamp]

I agree, but a BC break for some is better than a BC break for all IMHO :)

Thanks for changing that.

[frankdejonge]

My two cents on the matter. The getResponseString is a very technical description. It might be better to name it something along the lines of convertToAccessToken with a string return type. This is a better description than "get string response". The current convertToJwt could be seen as an implementation detail of that function.

[Sephster]

Great suggestion. Thanks @frankdejonge

[Sephster]

As @frankdejonge suggested, it would be good to change this to convertToAccessToken

[lookyman]

I'll update it this evening.

[Sephster]

I updated this this morning @lookyman - hope that was ok but thought I'd save you the effort as it was a quick change.

[Sephster]

I think this should be removed from the interface as people who don't want to use JWTs for their access token's will have no need for this. This is going to be a breaking change which is why I've flagged this change for version 8.

[lookyman]

Done.

[Sephster]

I've removed the native type hints for now as I'd prefer these were implemented project wide instead of in sections of the code.

I think this is pretty close to being able to be merged in but I'm unsure about the new method name now I've reviewed it again.

In our code we now can have implementations as follows:

$this->accessToken->convertToAccessToken($this->privateKey);

This seems strange to me due to the naming we have used. Why would an access token object need to be converted to an access token? Its name implies it already is one. I'm worried that the current name of the function isn't clear enough.

I have been racking my brain trying to think of a more appropriate name. I had thought we could possibly use __toString() but it doesn't accept an argument meaning we would need to set the private key in advance which is probably more trouble than its worth.

The best name I've come up with at the moment is simple build, which would align it with the JWT libraries builder class. I think that it would make more sense to call the function this so the code would then read as follows, which to me, seems more natural/intuitive.

$this->accessToken->build($this->privateKey);

If you have any thoughts on this @lookyman - or anyone else for that matter, I would very much appreciate a second set of eyes for this proposed adjustment

[Sephster]

I've also changed this to merge into branch 8.0.0 which I created today

[lookyman]

I don't really care about the method's name, I am fine either way.

I would however love to have it without the argument, and set the key in advance, to further separate it from any hardcoded implementation. But I understand if you don't want to go in that direction right now.

[Sephster]

Hmmm. As far as I can tell, we set the private key for the response type but it is just passed to the access token. It might be more appropriate to set it on the access token as that is the entity that is using it. We can easily do this in the issueAccessToken function.

We could then use a __toString() magic method and do away with the instance on the response type if I haven't missed anything.

Do you want to have a go at implementing this @lookyman? If not, I'm happy to make the changes tomorrow. Cheers

[Sephster]

I've updated the code @lookyman to use __toString() and set the private key in the access token for encryption. Let me know what you think. Cheers.

[lookyman]

Thank you, I love it.

Review for old version

[Sephster]

Good stuff. I will merge it in now :) Cheers for all your efforts with this