-
Notifications
You must be signed in to change notification settings - Fork 56
Request URI Manipulations #163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
c29bfde
to
7df95c9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should add a usage example for each plugin, not only the BaseUriPlugin. but i can do that later once this PR is merged - the AddHostPlugin doc was completely missing before, so this is already an improvement.
|
||
$plugin = new BaseUriPlugin(UriFactoryDiscovery::find()->createUri('https://domain.com:8000/api'), [ | ||
// Always replace the host, even if this one is provided on the sent request. Available for AddHostPlugin. | ||
'replace' => 'true', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
afaik the value here should be boolean true, not string
|
||
Request URI manipulations can be done thanks to several plugins: | ||
|
||
* ``AddHostPlugin``: Sets default host, scheme and port of each request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
depending on configuration, this can force the host or set it to a default value. maybe "Set host, scheme and port. Depending on configuration, the host is overwritten in every request or only set if not yet defined in the request."
Request URI manipulations can be done thanks to several plugins: | ||
|
||
* ``AddHostPlugin``: Sets default host, scheme and port of each request. | ||
* ``AddPathPlugin``: Sets default base path of each request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not a default but unconditionally applied: "Prefix the request path with a path, leaving the host information untouched."
|
||
* ``AddHostPlugin``: Sets default host, scheme and port of each request. | ||
* ``AddPathPlugin``: Sets default base path of each request. | ||
* ``BaseUriPlugin``: Is a combination of ``AddHostPlugin`` and ``AddPathPlugin`` using the ``UriInterface`` class. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets leave away the mention of UriInterface, that is just an implementation detail
Thank you for this PR. Could you fix the issues David commented? I would be happy to improve the documentation with this PR. |
Hello @Nyholm. I was on vacation. I'll work on it today or next week! 👍 |
7df95c9
to
364f596
Compare
All requested changes are done @dbu. This PR is ready for review and merge. |
Thanks @soullivaneuh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
found one formatting error, otherwise its all fine
Request URI manipulations can be done thanks to several plugins: | ||
|
||
* ``AddHostPlugin``: Set host, scheme and port. Depending on configuration, | ||
the host is overwritten in every request or only set if not yet defined in the request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you want to have multiline in a bullet list, you need to indent the second line by two spaces to align with the ` above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks to be related to the failing job.
Ah, build is failing. Can you please check? https://travis-ci.org/php-http/documentation/builds/173191035#L333 |
364f596
to
ea803b0
Compare
It should be OK now. |
Build passed! 🍾 |
Closes #162
Related source PR: php-http/client-common#48