Enable plugins to override the url_options in authorize method

Why not use the alter hook in this case: https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Extension!ModuleH... ?

+1 for the RTBC,
Nice to have update to api.php file with example, a lot of people are new to event handler here.

No offense, but introducing event handlers to the module for one hook while others don't support them? - I'd suggest sticking to the hooks for the current version of the module, and introducing events when the API gets an overhaul (e.g. to make the module Drupal 9 compliant).

Alter hooks are still a viable and often used method to allow for altering data in Drupal 8.

For your additional query parameter: For this use case, I suggest creating a custom client plugin for your IdP. Then you can implement the
OpenIDConnectClientInterface::authorize() method and add your custom query parameters to the authorization request. No need for an additional hook here. Or did I miss something?

The best we can do in D8 in terms of DX is to support both hooks and events (preferring events over hooks in terms of execution order).

I'd therefore recommend to go ahead with this patch, but add a regular alter hook, too. #BestOfBothWorlds #DX

Attached patch adds the regular alter hook to make both worlds happy.

We can amend the other hooks with events in a separate issue.

@sun: Thanks for the revised patch. However, my concerns where more related to whether it makes much sense to introduce the event dispatcher for a single event only, whereas the module provides hooks only everywhere else.

That's why my idea is to provide this additional hook now, and then implement the event dispatcher as alternative for ALL existing hooks of the module. Targeted for the next beta version.

So modules extending openid_connect can choose between subscribing to events, or sticking to hooks until they will be deprecated for the entire module. This avoids a too messy API implementation, where modules may be required to use hooks and events.

Created a general task to add event support for all hooks. TBD for the next beta release.

Re-roll against 8.x-1.x...

An update to the general purpose of this issue:

As a (new) co-maintainer of the module, I vote against adding an event/hook trigger within the plugin and will discuss this matter with sanduhrs before we make a final decision.

My reasoning:

The pluggable implementation of the OpenID Connect module allows for adding support for other IdPs than the ones that are shipped with the module. Instead of using hooks or events, third-party modules can and should provide their own plugins to support additional IdPs or implement special needs as different URL query parameters. This can easily done by overriding the authorize method of the base client.

We can't influence, how these plugins are written. We can only provide generic methods and helpers from within our module's service classes that reasonably support the general flow of OpenID Connect authorization and user property mapping. These generic methods allow for alterations by using our hooks and potentially soon events.

Adding a hook/event dispatch within the plugin means, that all custom plugins have to trigger these hooks/events, so that other modules can rely on them. Unfortunately, triggering the hook/event within such third-party plugin may be overseen, forgotten, or intentionally left out. Which in turn means, that to ensuring the event/hook is fired, we'd have to make all plugins depend in a private method of the base plugin class that would be responsible for dispatching this event. Or we would open up a potential source of hard-to-track errors where a hook/event provided/documented by our module will never be fired.

Both are scenarios, I'd pretty much like to avoid and suggest creating an own plugin for special use-cases instead.

Given the above, I'll set this certain issue back to "Needs work" till we find a better solution. It's also very likely that it will be closed as "Won't fix" in the near future.

@mikejw: For your additional FormState parameter, please create a separate issue, if you still require it.

This would change the signature of the plugin authorize methods, and therefore is a breaking change to all current plugin implementations. We should decide in a dedicated issue of whether and when it will be done.

To support our and the community's decision, please add enough information, of why this change would be to the benefit of other users of the module (e.g. describe your use case to let us evaluate of whether it is a common issue that can be addressed). You may also try out the issue summary template for your next issue reported.

Another re-roll, keeping up with 8.x-1.x. Sorry for the noise.

Not sure why these patches are needing constant re-rolls :(

I arrived here since I did not want to override the entire method just to change the URL options in my custom plugin.

I would suggest adding a protected helper function in the base class which we can override. This would make it possible to override a "core" one as well, just by adding a custom plugin that extends it, and overrides this method. Much cleaner.

Patch attached.

I think that @eiriksm's approach here is indeed the best. If you need to alter the URL options, the plugins should be able to override that.

Since the patch in #16 is only refactoring existing code, I'm setting it to RTBC.