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 1 commit
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
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 scheme, host and port in the request URI.')
Copy link
Member

Choose a reason for hiding this comment

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

Maybe change this to "Set a bast URI to the request-"

->children()
->scalarNode('host')
Copy link
Member

@joelwurtz joelwurtz Dec 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It should be uri and not host IMO, BaseUriPlugin can preprend a path, so someone should be able to use : https://my-site/prepend-path

Copy link
Member

Choose a reason for hiding this comment

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

Yes, Also update here: https://github.com/php-http/HttplugBundle/pull/240/files#diff-18f3f26a96b967c0a5a3e13086d8367dR208

->info('Host name including protocol and optionally the port number, e.g. https://api.local:8000')
Copy link
Member

Choose a reason for hiding this comment

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

Same here, we should talk about being able to have a path

->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

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

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

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.'.host_uri';
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't that be base_uri ?

$this->createUri($container, $baseService, $config['host']);
$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