Skip to content

Add BaseUriPlugin support. ISSUE-235 #240

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.

Sign up for GitHub

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 10 commits into from
Feb 13, 2018
Merged

Add BaseUriPlugin support. ISSUE-235 #240

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c51720a
Add BaseUriPlugin support. ISSUE-235
llaakkkk Dec 19, 2017
f7aa143
Add fixes after review: fixed code, add info to CHANGELOG.md
llaakkkk Dec 28, 2017
d44c8d5
Merge pull request #1 from php-http/master
llaakkkk Jan 12, 2018
601953c
Merge branch 'master' into feature/addingBaseUriPluginSUpport
llaakkkk Jan 12, 2018
b1257a0
Add new info to CHANGELOG.md with version 1.9.0
llaakkkk Jan 12, 2018
75ab4b3
Add new info to CHANGELOG.md with version 1.9.0-unreleased and ad mis…
llaakkkk Jan 15, 2018
e22007b
Fixed break style
llaakkkk Jan 15, 2018
cbde5ea
Added configuration to tests
llaakkkk Feb 11, 2018
3b3629c
Added test configuration
llaakkkk Feb 13, 2018
45380b2
Added config base_uri to full.xml
llaakkkk Feb 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ The change log describes what is "Added", "Removed", "Changed" or "Fixed" betwee
- Any third party library using `Http\Client\Common\PluginClientFactory` to create `Http\Client\Common\PluginClient`
instances now gets zero config profiling.
- `Http\HttplugBundle\Collector\Collector::reset()`
- BaseUriPlugin support
Copy link
Collaborator

@dbu dbu Dec 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah sorry, you did add to the changelog. however, 1.8 has meanwhile been released. please rebase on master and then move this to a new section for 1.9. lets say something like "Configure the BaseUriPlugin per client, under the base_uri configuration key."


### Changed

Expand Down
16 changes: 16 additions & 0 deletions DependencyInjection/Configuration.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,22 @@ private function createClientPluginNode()
->end()
->end()
->end()
->arrayNode('base_uri')
->canBeEnabled()
->addDefaultsIfNotSet()
->info('Set a base URI to the request.')
->children()
->scalarNode('uri')
->info('Base Uri including protocol, optionally the port number and prepend path, e.g. https://api.local:8000/api')
->isRequired()
->cannotBeEmpty()
->end()
->scalarNode('replace')
->info('Whether to replace the host if request already specifies one')
->defaultValue(false)
->end()
->end()
->end()
->arrayNode('header_append')
->canBeEnabled()
->info('Append headers to the request. If the header already exists the value will be appended to the current value.')
Expand Down
7 changes: 7 additions & 0 deletions DependencyInjection/HttplugExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,13 @@ private function configurePluginByName($name, Definition $definition, array $con
$definition->replaceArgument(1, [
'replace' => $config['replace'],
]);
Copy link
Collaborator

@dbu dbu Dec 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing break

Copy link
Collaborator

@dbu dbu Jan 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you still need to add a break; here, its the reason why the tests fail

Copy link
Contributor Author

@llaakkkk llaakkkk Jan 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbu I fixed all review, can you please review my PR?

case 'base_uri':
$baseService = $serviceId.'.base_uri';
$this->createUri($container, $baseService, $config['uri']);
Copy link
Collaborator

@dbu dbu Dec 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we s/$baseService/$baseUriService/ and refactor the section above (add_host) to call its variable s/$uriService/$hostUriService/? i would prefer consistent naming and being specific helps here (otherwise we have twice $uriService which is confusing too)

$definition->replaceArgument(0, new Reference($uriService));
$definition->replaceArgument(1, [
'replace' => $config['replace'],
]);

break;
case 'header_append':
Expand Down
4 changes: 4 additions & 0 deletions Resources/config/plugins.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@
<argument/>
<argument/>
</service>
<service id="httplug.plugin.base_uri" class="Http\Client\Common\Plugin\BaseUriPlugin" public="false" abstract="true">
<argument/>
<argument/>
</service>
<service id="httplug.plugin.header_append" class="Http\Client\Common\Plugin\HeaderAppendPlugin" public="false" abstract="true">
<argument/>
</service>
Expand Down