Add BaseUriPlugin support. ISSUE-235 #240

llaakkkk · 2017-12-19T11:14:21Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Related tickets	-
Documentation	php-http/documentation#226
License	MIT

What's in this PR?

Add BaseUriPlugin support

Why?

#235

Example Usage

http://docs.php-http.org/en/latest/plugins/request-uri-manipulations.html

Checklist

Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix
Added info about base_uri to documentation

dbu

thank you, looks good!

can you add a documentation pull request and update the CHANGELOG.md file?

(btw, you can edit the pull request to make the description less confusing: remove the example as there is not much to example, remove todos that do not apply etc)

joelwurtz · 2017-12-19T12:03:48Z

DependencyInjection/HttplugExtension.php

@@ -203,6 +203,13 @@ private function configurePluginByName($name, Definition $definition, array $con
                $definition->replaceArgument(1, [
                    'replace' => $config['replace'],
                ]);
+            case 'base_uri':
+                $baseService = $serviceId.'.host_uri';


Shouldn't that be base_uri ?

llaakkkk · 2017-12-19T12:06:11Z

@dbu yes, I will add.

joelwurtz · 2017-12-19T12:06:42Z

DependencyInjection/Configuration.php

+                    ->addDefaultsIfNotSet()
+                    ->info('Set scheme, host and port in the request URI.')
+                    ->children()
+                        ->scalarNode('host')


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

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

joelwurtz · 2017-12-19T12:07:30Z

DependencyInjection/Configuration.php

+                    ->info('Set scheme, host and port in the request URI.')
+                    ->children()
+                        ->scalarNode('host')
+                            ->info('Host name including protocol and optionally the port number, e.g. https://api.local:8000')


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

joelwurtz · 2017-12-19T12:13:37Z

Thanks for this, can you add also a test in https://github.com/php-http/HttplugBundle/blob/master/Tests/Unit/DependencyInjection/ConfigurationTest.php

(Just adding this plugin into the full.yml/xml/php and check if it's equal into the existing test should be enough IMO)

Nyholm

Looks good. Just a few minor changes.

Good work!

Nyholm · 2017-12-19T16:09:31Z

DependencyInjection/Configuration.php

+                ->arrayNode('base_uri')
+                    ->canBeEnabled()
+                    ->addDefaultsIfNotSet()
+                    ->info('Set scheme, host and port in the request URI.')


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

Nyholm · 2017-12-19T16:10:40Z

DependencyInjection/Configuration.php

+                    ->addDefaultsIfNotSet()
+                    ->info('Set scheme, host and port in the request URI.')
+                    ->children()
+                        ->scalarNode('host')


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

llaakkkk · 2017-12-28T00:23:22Z

@joelwurtz can you please describe more about this

(Just addingthis plugin into the full.yml/xml/php and check if it's equal into the existing test should be enough IMO)

llaakkkk · 2017-12-28T00:27:12Z

@dbu I also added changes to CHANGELOG.md and PR doc. Can you please review it?

dbu

(Just addingthis plugin into the full.yml/xml/php and check if it's equal into the existing test should be enough IMO)

look at the failing tests: there is a unit test called HttplugExtensionTest that checks default and full configuration. here we should make sure that base_uri is supported by adding it to the fixture files for full configuration. (and something seems to go wrong atm, we get an undefined index error - i think its the missing break at the end of the add_host section)

can you please also do a pull request against the documentation to describe how the plugin is configured, and edit the description of this PR to add the link to that PR?

please also add something to the CHANGELOG in a new section "1.9.0 (unreleased)" to say that base_uri support has been added? and finally, because this is a new feature, please change the branch-alias in composer.json to 1.9-dev.

dbu · 2017-12-28T10:03:23Z

DependencyInjection/HttplugExtension.php

@@ -203,6 +203,13 @@ private function configurePluginByName($name, Definition $definition, array $con
                $definition->replaceArgument(1, [
                    'replace' => $config['replace'],
                ]);
+            case 'base_uri':
+                $baseService = $serviceId.'.base_uri';
+                $this->createUri($container, $baseService, $config['uri']);


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)

dbu · 2017-12-28T10:08:00Z

DependencyInjection/HttplugExtension.php

@@ -203,6 +203,13 @@ private function configurePluginByName($name, Definition $definition, array $con
                $definition->replaceArgument(1, [
                    'replace' => $config['replace'],
                ]);


missing break

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

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

dbu · 2017-12-28T10:13:11Z

CHANGELOG.md

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


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

Changes

…sing break

dbu

Thanks, the code looks good to me now!

Can you please add this to the full configuration test? see full.yml|xml|php and the configuration test that uses those files.

HttplugBundle/Tests/Resources/Fixtures/config/full.yml

Line 19 in 7564416

add_host:

and please do a pull request to https://github.com/php-http/documentation/blob/master/integrations/symfony-full-configuration.rst to add the example, so that people can discover this functionality.

llaakkkk · 2018-02-11T19:53:05Z

Added this to full test configuration. And add info to documentation and create new PR.php-http/documentation#226

dbu

you need to add base_uri to full.xml and full.php as well, and adjust Tests/Unit/DependencyInjection/ConfigurationTest.php to expect the base_uri configuration (see the failing test on travis-ci)

dbu · 2018-02-12T08:21:15Z

CHANGELOG.md

@@ -2,6 +2,9 @@

 The change log describes what is "Added", "Removed", "Changed" or "Fixed" between each release.

+## 1.9.0 (unreleased) - 2017-01-12 
+- Configured the `BaseUriPlugin` per client, under the `base_uri` configuration key.


i'd say: "Allow to configure ..."

Nyholm

Awesome. Thank you for this PR

dbu

Thanks!

Nyholm · 2018-02-13T15:27:12Z

I really apprichiate the fix and all the reviews.

llaakkkk · 2018-02-13T15:28:11Z

Guys!! Thank you too for support and help and task!!))

Add BaseUriPlugin support. ISSUE-235

c51720a

dbu reviewed Dec 19, 2017

View reviewed changes

joelwurtz reviewed Dec 19, 2017

View reviewed changes

Nyholm reviewed Dec 19, 2017

View reviewed changes

Add fixes after review: fixed code, add info to CHANGELOG.md

f7aa143

dbu reviewed Dec 28, 2017

View reviewed changes

llaakkkk added 5 commits January 12, 2018 16:07

Merge pull request #1 from php-http/master

d44c8d5

Changes

Merge branch 'master' into feature/addingBaseUriPluginSUpport

601953c

Add new info to CHANGELOG.md with version 1.9.0

b1257a0

Add new info to CHANGELOG.md with version 1.9.0-unreleased and ad mis…

75ab4b3

…sing break

Fixed break style

e22007b

dbu reviewed Jan 15, 2018

View reviewed changes

Added configuration to tests

cbde5ea

llaakkkk mentioned this pull request Feb 11, 2018

Added information about base_uri using to symfony-full-configuration.rst php-http/documentation#226

Merged

dbu requested changes Feb 12, 2018

View reviewed changes

llaakkkk added 2 commits February 13, 2018 12:32

Added test configuration

3b3629c

Added config base_uri to full.xml

45380b2

Nyholm approved these changes Feb 13, 2018

View reviewed changes

dbu approved these changes Feb 13, 2018

View reviewed changes

dbu merged commit 5ea9284 into php-http:master Feb 13, 2018

Add BaseUriPlugin support. ISSUE-235 #240

Add BaseUriPlugin support. ISSUE-235 #240

Uh oh!

Conversation

llaakkkk commented Dec 19, 2017 • edited by dbu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's in this PR?

Why?

Example Usage

Checklist

Uh oh!

dbu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

llaakkkk commented Dec 19, 2017

Uh oh!

joelwurtz Dec 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelwurtz commented Dec 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nyholm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

llaakkkk commented Dec 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llaakkkk commented Dec 28, 2017

Uh oh!

dbu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu left a comment

Choose a reason for hiding this comment

Uh oh!

llaakkkk commented Feb 11, 2018

Uh oh!

dbu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nyholm left a comment

Choose a reason for hiding this comment

Uh oh!

dbu left a comment

Choose a reason for hiding this comment

Uh oh!

Nyholm commented Feb 13, 2018

Uh oh!

llaakkkk commented Feb 13, 2018

Uh oh!

Uh oh!

llaakkkk commented Dec 19, 2017 •

edited by dbu

Loading

joelwurtz Dec 19, 2017 •

edited

Loading

joelwurtz commented Dec 19, 2017 •

edited

Loading

llaakkkk commented Dec 28, 2017 •

edited

Loading