Only add path prefix if the path does not contain it already

[mxr576]

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Related tickets	#74 (revisit)
Documentation	-
License	MIT

What's in this PR?

This is a revisit of the #74.

Why?

(Just second my comments from the original PR)

If you try to restart a failed API call by calling return $first($request)->wait() in a plugin (ex.: when an Oauth access token expires and you want your API client to automatically resend the same request with a new access token provided by a custom Oauth auth plugin) then the current behaviour of AddPathPlugin corrupts the URL by adding the same API version prefix to the URL again.
What is the exact use case behind adding a path prefix to a path that already has the prefix anyway? I can not imagine any.

Example Usage

Checklist

To Do

[dbu]

hm, i see the use case you have. the change could however lead to other problems, if for whatever reason i have an url that accidentally contains the prefix. i can't think of a really good example, so this contreived example: my prefix should be /a and the request goes to /authors - with this change, the prefix would not be added anymore. we could add a trailing slash for the check, but even then it would be unexpected that prefixing /a in front of /a/weird/url would not work.

is there a way for the plugin to detect if it has already seen "this" request? unfortunately, psr requests are re-created with each change, so keeping an object reference won't help. psr requests don't have attributes that we could use to mark a request as "seen" that are not sent to the server, right? a header would be a problem as it pollutes the request being sent.

[mxr576]

Yes, I do agree this could a possible error flow after this patch. My another idea was to create a hash from "$request->getUri()->getPath()" (so the original path) and use that to identify whether the plugin has already "seen" and modified that path before or not. But this can possibly lead to another unexpected errors, I would need to confirm that.
Also, hashing the $request object just like the Retry plugin does would be useless in this case.

[mxr576]

Actually what the retry plugin does can work in this case too.

[dbu]

this looks good to me, and should avoid the problem without any edge case regressions.

i have one performance optimization suggestion (which we probably should also do on other plugins that use this pattern)

[dbu]

could we make alteredRequests a hashmap of $identifier => true instead? then we could check with array_has_key instead of in_array. for long running processes that send lots of requests, the in_array will become expensive once the list of altered requests grows.

[mxr576]

That came in my mind but IMO the chance of this is really low. Anyway, I updated to code to store identifier => identifier in the array therefore we do not need to check what is the value of an identifier. Storing boolean values in the array would be also misleading because someone may think that $identifier => false means the request has not been altered yet.

[dbu]

thanks a lot for this, looks good to me now!

[Neirda24]

Hi. I have a use case where when developing locally a path is not needed (I use mock servers) and depending on the installation of my project I may need to add a prefix path... So sometimes it is empty and sometimes it is not... At this day it does not allow it to be empty. I thought maybe this PR could solve it ?

[dbu]

@Neirda24 please open a new issue to discuss this topic, as its not related to the pull request. (the good thing though is that it makes me notice we never merged this, will do that now...)

[dbu]

i have added a changelog entry and squashed the commits. merged in e35c5f3

[Nyholm]

Why would you do return $first($request)->wait()??

I understand $response = $first($request)->wait() or return $first($request). But return $first($request)->wait() will definitely break stuff.

[dbu]

oh, indeed, did not realize that. but from the context of the description (oauth) i assume the request was waited for to get the response and pull the oauth token out of that response.

on oauth specifically, we recently discussed a similar case and said the best implementation would be to not make that a plugin but a authentication that is given to the existing AuthenticationPlugin, and inject a separate client instance to the oauth authentication. that client does at least not need the authentication, and possibly also not some other plugins.