Skip to content

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

Merged
merged 1 commit into from
Nov 4, 2016

Conversation

soullivaneuh
Copy link
Contributor

Closes #162

Related source PR: php-http/client-common#48

Copy link
Contributor

@dbu dbu left a 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',
Copy link
Contributor

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.
Copy link
Contributor

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.
Copy link
Contributor

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.
Copy link
Contributor

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

@Nyholm
Copy link
Member

Nyholm commented Oct 25, 2016

Thank you for this PR. Could you fix the issues David commented? I would be happy to improve the documentation with this PR.

@soullivaneuh
Copy link
Contributor Author

Hello @Nyholm. I was on vacation. I'll work on it today or next week! 👍

@soullivaneuh soullivaneuh force-pushed the request-uri-manipulations branch from 7df95c9 to 364f596 Compare November 4, 2016 08:57
@soullivaneuh
Copy link
Contributor Author

All requested changes are done @dbu. This PR is ready for review and merge.

@sagikazarmark
Copy link
Member

Thanks @soullivaneuh

Copy link
Contributor

@dbu dbu left a 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.
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@sagikazarmark
Copy link
Member

Ah, build is failing. Can you please check? https://travis-ci.org/php-http/documentation/builds/173191035#L333

@soullivaneuh soullivaneuh force-pushed the request-uri-manipulations branch from 364f596 to ea803b0 Compare November 4, 2016 09:28
@soullivaneuh
Copy link
Contributor Author

It should be OK now.

@soullivaneuh
Copy link
Contributor Author

Build passed! 🍾

@dbu dbu merged commit 8ca71b5 into php-http:master Nov 4, 2016
@soullivaneuh soullivaneuh deleted the request-uri-manipulations branch November 4, 2016 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants