Make HTTP transporter setup in registry more flexible to not throw discovery exception when no HTTP request is even being attempted

[felixarntz]

This partially will fix WordPress/wp-ai-client#41 - specifically, when a provider is registered before an HTTP client has been configured in the PSR-17 discovery mechanism.

This is probably not sufficient by itself as a proper fix for that issue, but it's already a partial fix, which means a workaround will no longer be needed. And since this library is not meant to be only used with WordPress, it makes sense to optimize for general flexibility:

• The problem is that currently, whenever someone tries to register a provider, the default registry is initialized.
• And since the HTTP transporter is set as part of that, we run into an exception if no 3P code has provided an HTTP client yet. This is bad DX because there's no point in throwing an exception at this point, given that no HTTP request is even being attempted.

This PR fixes that, by dynamically attempting to set the default HTTP transporter when no (other) HTTP transporter has already been set, and doing that in a way that is forgiving in terms of the discovery exception.

cc @martin-helmich @LukasFritzeDev

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @dgrieser.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Unlinked contributors: dgrieser.

Co-authored-by: felixarntz <flixos90@git.wordpress.org>
Co-authored-by: JasonTheAdams <jason_the_adams@git.wordpress.org>
Co-authored-by: swissspidy <swissspidy@git.wordpress.org>
Co-authored-by: LukasFritzeDev <lukasfritzedev@git.wordpress.org>
Co-authored-by: martin-helmich <mhelmich@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[JasonTheAdams]

Really interesting, @felixarntz! If I'm following, this comes down to the fact that the WP AI Client doesn't connect the WP_AI_Client_Discovery_Strategy until init fires? So if someone were to register a provider prior to this point and calls AIClient::defaultRegistry(), it will throw a RealNotFoundException inside HttpTransportFactory::createTransport()? Gotta love a good race condition. 😄

I mean, really, the discovery system can be hooked up as soon as possible. It doesn't require that anything in WordPress itself is loaded at that time. I could see providers wanting to be registered (and even usable) prior to init, so that may be worth considering. I saw your comment in the other Issue that you think there should be a distinct hook for registering providers, which I think I agree with.

As a note, while the WP AI Client does load in init, if used as a plugin, it can be used as a package, as in AI Experiments. That plugin, then, is the one deciding when to call AI_Client::init(). So if we move things to load earlier, we'll need to do it in both places.

If I'm tracking with all that, the goal of this PR is to at least make it so that when a provider is registered prior to this point it still works. This done is by suppressing the NotFoundException, which is necessary because we really don't have any other way of knowing if an HTTP system ought to have been loaded yet. Things would work later because whenever ProviderRegistery::setHttpTransporter() is finally called, it sets it for all providers already set.

My only concern with this is the remaining race condition of someone trying to use AiClient::prompt() when a provider has been registered by the transporter has not been successfully set. We're sort of shuffling things around. That may be fair, but I wanted to at least raise the point.

Am I tracking with all this?

[felixarntz]

@JasonTheAdams I think you're right on all accounts. But I'd like us to look at this from a less WordPress-centric angle too. While we can shuffle things around in the WP AI Client, and maybe set up the HTTP client very early in the WordPress bootstrap cycle, I think there's still no point in throwing an HTTP discovery exception when a provider is registered. So this PR is mostly about being more "forgiving", or less aggressive.

An HTTP client and HTTP transporter only need to be set up once any AI HTTP request is made (e.g. via AiClient::prompt()). At that point, it should throw the exception. But before any of that happens, it really doesn't matter, so throwing an exception can unnecessarily throw a wrench in things.

So to your last point, I'm not sure I follow what your concern is with AiClient::prompt() - given at that point the lack of an HTTP transporter or client should indeed throw an exception. And I don't think this PR does anything that would jeopardize that?

[JasonTheAdams]

Got it. I'm tracking with what you mean. The think the nested try...catch is unnecessary, then. The RuntimeException catch only seems necessary because we're using the WithHttpTransporterTrait, but I don't think that trait really fits here. We're purposely considering the transporter as something nullable in this class, trusting it will eventually come.

I think if we drop the use of that trait and make it a nullable property, then we can clean that up. The only other place the getter is used is in bindModelDependencies, and if we consider that a fair point of failure, then we can throw an exception there with a more meaningful exception/description.

[felixarntz]

Hmm, but why not use the trait? Can you clarify what you think is the problem with it?

[JasonTheAdams]

Hmm, but why not use the trait? Can you clarify what you think is the problem with it?

The problem is that the trait assumes the transporter is non-nullable, which is why we're having to catch the exception for the getter. In the case of the ProviderRegistry we're saying that's not the case — null is completely fine until a certain point. The trait is doing very little for us, by design doesn't introduce a contract, and is making the code less clean, not more. By removing it we can drop the first level try...catch and just check if the property is null or not. We can do the same later in bindModelDependencies and throw a more meaningful exception if it is null.

Make sense?

[felixarntz]

@JasonTheAdams

The trait is doing very little for us, by design doesn't introduce a contract, and is making the code less clean, not more.

I'm not sure I agree. Why is catching an exception a bad thing to do here? The trait still takes away responsibility of having the property, setter, and getter. It's all super basic functionality, but I'd argue the trait simply reduces the amount of code we have to (re-)write.

[JasonTheAdams]

@felixarntz I think we just disagree, then. Hahah! But it's not a hill I'd die on.

Personally, I think the trait is not matching the actual intentions of the class — namely, that a nullable property is valid. I don't even think we need the getter, either, because it's only used internally. So I think we're applying something to reduce code that isn't actually a proper fit, it just feels close enough. The exception catching becomes a code smell, not that catching exceptions is bad in any way.

I'm approving because I've made my point and am fine either way. 😄