Enhance exception API for response exceptions from invalid values by including concrete field name

[felixarntz]

Follow up to #78: This brings ResponseException::fromInvalidData more in line with ResponseException::fromMissingData, making the exception messages more helpful by including the concrete response field name where the problem occurred.

[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.

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

Co-authored-by: felixarntz <[email protected]>
Co-authored-by: JasonTheAdams <[email protected]>

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

[JasonTheAdams]

I like the modification in the exception, @felixarntz! I'm a bit 🤔 about one specific implementation of it.

[JasonTheAdams]

I'm not crazy about requiring this parameter that's strictly used for the exception. This method otherwise isn't coupled to the concept that there's an index somehow associated with the choice. If we feel the index is really important (and the $choiceData isn't sufficient), I'd rather throw the exception where the loop is happening — even if it means catching this exception in the loop and throwing a new one.

[felixarntz]

That's totally fair. I'll take another look later to see how this could be restructured.

[felixarntz]

@JasonTheAdams Looking into this, I'm not sure what's the best alternative - everything ends up being hacky somehow.

You could do something where the inner methods (without awareness of $index) simply pass the innermost field name (e.g. message instead of choices[{$index}].message), and then the outer method catches it and runs a regular expression on $e->getMessage() to replace "([A-Za-z0-9_]+)" with "choices[{$index}].$1", but that's hacky because it is based on the specific message shape of the ResponseException class. - I don't love it.

Other ideas I thought about is to store the API name and field name as separate properties in ResponseException so you can read them from the caught instance, but then you also still need to somehow extract the part of the message that's dynamic and doesn't just come from the API name and field name. Not a great solution either.

Let me know if you have other ideas.

Otherwise, I think the current approach of passing something to the inner methods is the most straightforward. An idea that might alleviate your concern would be that we don't just pass the index, but the full "prefix" identifier of the current context that is being parsed, e.g. instead of $index (integer) we would pass choices[{$index}] (string). This not only clarifies the purpose of why this is passed, it also avoids the problem of the inner methods having some awareness of their context (e.g. right now the methods must know that the outer field is called choices, which is not ideal) - by passing the context down in its entirety the responsibility remains entirely with the outer method, and as such, in a single place within the class.

WDYT?

[JasonTheAdams]

Hmm, I'm starting to back down on this one. I admit I didn't notice the first time that this is a protected method (and could maybe even be private). If this is just an internal method, then I don't think passing the index is a big deal. It feels a little odd since it's just used for exceptions, but that would be more strange if it was public.

[felixarntz]

OK, sounds good. I think it still needs to be protected so that custom tools in e.g. a specific provider can be supported (by overriding some of these methods in a child class), but yeah the risk should be rather low. Also as long as we're in such an early version, for such low-level semi-internal APIs we can still make breaking changes if we later find there's a problem.

LMK if this works for you.

[JasonTheAdams]

LGTM!