Add AddPathPlugin always_prepend option #74

fbourigault · 2017-06-06T16:00:16Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Related tickets	N/A
Documentation	TODO
License	MIT

This allow to make the AddPathPlugin smart. By smart I mean to not prepend the configured URI path to the request URI if already present. This behavior is disabled by default to keep code BC.

The BaseUriPlugin now has a third optional constructor argument to configure the underlying AddPathPlugin.

This is required for https://github.com/m4tthumphrey/php-gitlab-api which has a base URI set to https://example.gitlab.com/api/v3 but is also used to follow pagination Link headers uri which already have the base URI configured.

To do

Documentation.
Changelog.

fbourigault · 2017-06-07T04:39:12Z

I'm open to suggestions for an alternative to the smart name!

dbu

looks good to me, apart from my style nitpicks. i am not super happy with "smart" but can't think of a better name. i can live with "smart" :-)

dbu · 2017-06-07T10:13:51Z

src/Plugin/AddPathPlugin.php

     * @param UriInterface $uri
+     * @param array        $config {
+     *


please remove the extra blank line

Looks like StyleCI wants this extra blank line.

dbu · 2017-06-07T10:15:48Z

src/Plugin/AddPathPlugin.php

     * @param UriInterface $uri
+     * @param array        $config {
+     *
+     *     @var bool $smart True will prepend path only if request path does not start with path to add.


i am somehow confused by the wording. maybe "Set to true to only prepend the path only if the request path does not start with that path already."?

dbu · 2017-06-07T10:17:22Z

src/Plugin/AddPathPlugin.php

+        $prepend = $this->uri->getPath();
+        $path = $request->getUri()->getPath();
+
+        if (!($this->smart && substr($path, 0, strlen($prepend)) === $prepend)) {


can we rewrite this to if (!$this->smart || !substr(...))? i feel that is more readable.

dbu · 2017-06-07T10:17:54Z

CHANGELOG.md

@@ -2,6 +2,11 @@

 ## Unreleased

+### Added
+
+- Add the `$smart` option to `AddPathPlugin` to prepend path only when required.


lets write this 'smart' instead of $smart

fbourigault · 2017-06-07T10:23:12Z

About the smart name. We can also reverse the boolean meaning and so the behavior (and of course change the default value). Maybe it could help to find a better name.

EDIT: what about always_prepend with true as default value?

Nyholm · 2017-06-07T11:34:28Z

what about always_prepend with true as default value?

I like that better.

Nyholm · 2017-06-07T11:38:30Z

src/Plugin/AddPathPlugin.php

+        $prepend = $this->uri->getPath();
+        $path = $request->getUri()->getPath();
+
+        if (!$this->smart || substr($path, 0, strlen($prepend)) !== $prepend) {


This only consider the case where $prepend is the beginning of $path. Is that the only case we want?

In your case on Gitlab, you do not want to prepend if a absolute URL is used. IMHO, that logic should only be in BaseUriPlugin.

I did this way to be more consistant with AddHostPlugin which owns the replace logic.

To get back to my use case, it's about using Link header href attributes. As of https://tools.ietf.org/html/rfc5988 §5.1 this IRI can be relative so I'm not sure that in a future release gitlab will send relative IRIs.

But maybe I'm wrong with the client design and the API path (/api/v3) should be hardcoded in the abstract api class and so, I just need to use the AddHostPlugin as the user will only configure it's gitlab instance domain.

But maybe I'm wrong with the client design and the API path (/api/v3) should be hardcoded in the abstract api class

Yeah, maybe.. :/

Or in every endpoint..

Though. This PR is still useful.

sagikazarmark · 2017-06-08T09:09:26Z

How about preserve_uri defaulting to false?

sagikazarmark · 2017-06-08T09:12:06Z

Hm, rereading the initial comment not sure what we prepend here. Always prepend sounds a little weird to me. prepend/preserve_path/uri?

Nyholm · 2017-06-08T09:14:32Z

The AddPath plugin prepends the current path with a predefined value.

The idea is that if the predefined value is "/foo/bar" then we should not prepend that to an url like "/foo/bar/whatever" since it already contains "/foo/bar".

fbourigault · 2017-06-08T11:07:30Z

If prepend is an issue, I can name the option always_add. Didn't know that prepend was a neologism!

sagikazarmark · 2017-06-08T15:14:56Z

No, my issue is with always. Despite the word, it does not express the conditions when it matters. Of course we can document it properly, but very likely the next time I meet the config value I will just not remember what it was and look it up again.

Don't know if there is a better name for it though.

Nyholm · 2017-06-08T15:16:34Z

Something between “always_prepend” and “prepend_even_if_uri_start_with_prefix” would be good. Regards Tobias Nyholm

…

On 8 juni 2017, at 17:14, Márk Sági-Kazár ***@***.***> wrote: No, my issue is with always. Despite the word, it does not express the conditions when it matters. Of course we can document it properly, but very likely the next time I meet the config value I will just not remember what it was and look it up again. Don't know if there is a better name for it though. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#74 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABN1RgRvgKk9WapV7jvKlNbBMTM69EcPks5sCA_wgaJpZM4NxjxN>.

sagikazarmark · 2017-06-08T15:17:11Z

Exactly 😂

fbourigault · 2017-06-08T15:29:00Z

Fabien listen to propositions.

Nyholm · 2017-07-06T11:56:42Z

Name it prepend_even_if_uri_start_with_prefix

fbourigault · 2017-07-06T12:00:46Z

Closing because no longer required for https://github.com/m4tthumphrey/php-gitlab-api.

mxr576 · 2018-05-02T13:58:02Z

@sagikazarmark @Nyholm Can we re-open this issue? If you try to restart a failed API call by calling $first($request) 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.

mxr576 · 2018-05-02T15:36:09Z

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.

fbourigault mentioned this pull request Jun 6, 2017

Decouple from Buzz GitLabPHP/Client#191

Merged

6 tasks

dbu reviewed Jun 7, 2017

View reviewed changes

Nyholm reviewed Jun 7, 2017

View reviewed changes

add AddPathPlugin alwaysPreprend option

491665e

fbourigault changed the title ~~Add AddPathPlugin smart option~~ Add AddPathPlugin always_prepend option Jun 8, 2017

fbourigault closed this Jul 6, 2017

fbourigault deleted the smart-add-path-plugin branch July 6, 2017 12:00

mxr576 mentioned this pull request May 3, 2018

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AddPathPlugin always_prepend option #74

Add AddPathPlugin always_prepend option #74

fbourigault commented Jun 6, 2017 •

edited

Loading

fbourigault commented Jun 7, 2017

dbu left a comment

dbu Jun 7, 2017

fbourigault Jun 7, 2017

dbu Jun 7, 2017

dbu Jun 7, 2017

dbu Jun 7, 2017

fbourigault commented Jun 7, 2017 •

edited

Loading

Nyholm commented Jun 7, 2017

Nyholm Jun 7, 2017

fbourigault Jun 7, 2017 •

edited

Loading

fbourigault Jun 7, 2017

Nyholm Jun 8, 2017

sagikazarmark commented Jun 8, 2017

sagikazarmark commented Jun 8, 2017

Nyholm commented Jun 8, 2017

fbourigault commented Jun 8, 2017

sagikazarmark commented Jun 8, 2017

Nyholm commented Jun 8, 2017 via email

sagikazarmark commented Jun 8, 2017

fbourigault commented Jun 8, 2017

Nyholm commented Jul 6, 2017

fbourigault commented Jul 6, 2017

mxr576 commented May 2, 2018 •

edited

Loading

mxr576 commented May 2, 2018 •

edited

Loading

Add AddPathPlugin always_prepend option #74

Add AddPathPlugin always_prepend option #74

Conversation

fbourigault commented Jun 6, 2017 • edited Loading

To do

fbourigault commented Jun 7, 2017

dbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbourigault commented Jun 7, 2017 • edited Loading

Nyholm commented Jun 7, 2017

Choose a reason for hiding this comment

fbourigault Jun 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagikazarmark commented Jun 8, 2017

sagikazarmark commented Jun 8, 2017

Nyholm commented Jun 8, 2017

fbourigault commented Jun 8, 2017

sagikazarmark commented Jun 8, 2017

Nyholm commented Jun 8, 2017 via email

sagikazarmark commented Jun 8, 2017

fbourigault commented Jun 8, 2017

Nyholm commented Jul 6, 2017

fbourigault commented Jul 6, 2017

mxr576 commented May 2, 2018 • edited Loading

mxr576 commented May 2, 2018 • edited Loading

fbourigault commented Jun 6, 2017 •

edited

Loading

fbourigault commented Jun 7, 2017 •

edited

Loading

fbourigault Jun 7, 2017 •

edited

Loading

mxr576 commented May 2, 2018 •

edited

Loading

mxr576 commented May 2, 2018 •

edited

Loading