Save all tokens to the session

kristiaanvandeneynde → changed the visibility of the branch 3.x to hidden.

MR looks greats! Lets merge this?

To refresh the token, it is essential to have access to the OpenID client used to obtain the initial tokens, as it must also be utilized for refreshing them. I would suggest also storing the client id in the OpenID Connect session.

e.g. https://git.drupalcode.org/project/openid_connect/-/merge_requests/131/d...

Not sure I can follow. We're using the MR from this issue on a project and it's working fine in its current state. We simply always use the same client throughout our application. Also it seems like your MR is tagged against a different issue so perhaps we should keep this issue and its MR small in size? It's far easier to review and commit smaller MRs than big ones with multiple goals.

Will set to NR but feel free to RTBC again if you agree.

I agree with keeping MRs small in size. The point I want to emphasize is that the OpenID Connect session needs client context: in theory, you can have multiple clients active & connecting to the same Drupal account—for example, when using combinations of social clients like Facebook and Google.

To implement anything generic inside the OpenID Connect module with these stored tokens, we need the context of their origin. That’s the main reason I suggested including that addition in this issue. That said, I agree that this context isn’t really needed when using only one client and working with code within a custom module.

I’d love to hear your opinion on this.

I'd say there's a good chance there are many websites out there that only use one provider. For them, this MR is sufficient.

Delaying it to add more info that then needs tests to see how well it behaves when using multiple clients alongside each other seems unnecessary. I'd rather we focus this issue on expanding what can go into a session and then another issue on investigating how well the session currently behaves with multiple clients and if we can or even need to store the client ID in said session.

The issue is about "Save all tokens", so let's keep it like that? A Client ID is not a token, even though I understand why you brought it up.

Alright, I'll keep the 'client ID' session addition request in the other issue so that this one can be merged.

I added unit tests to the new methods and I renamed two of the methods to match the other save/retrieve token patterns:

- retrieveTokenExpire was renamed to retrieveExpireToken
- saveTokenExpire was renamed to saveExpireToken

I rebased the code with the 3.x version to get the tests validating Drupal 10 and 11. If all tests pass, I'll get this merged.

All tests passed after the rebase. I went ahead and merged this.

Thanks everyone!

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