Make it possible to create public httplug clients

[ruudk]

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

What's in this PR?

Added public: true/false to the Configuration. It is used to make the created HTTPlug client public or private.

Why?

All HTTPlug clients that are created are private by default, which is a good thing. But sometimes, you just need a public HTTPlug client.

Checklist

• Updated CHANGELOG.md to describe new feature
• Documentation pull request created

[Nyholm]

Hm. I agree to this PR. We discussed it before and we said that the user should always want a private service since that is the best practice.

However, I know people tried to access them as they were public.. I don’t know if we ever talked about making it configurable, but I believe that would be awesome for all users.

👍

[dbu]

i agree with this. i'll check the doc PR next to see if we properly say that you SHOULD use private services and DI, but if you have a case where you want to make it public, you can.

config tests fail because a new option is there so the default config needs to be adjusted.

it would be nice to also have a test making sure that normally the client service is private but if we set public: true it indeed is set to public. that one should go into the HttplugExtensionTest

[ruudk]

I added the tests and updated the documentation PR.

[xabbuh]

The test failures seem to be related to these changes. Also I think inside configureClient() we need to update the decorations happening at the end too.

[ruudk]

@xabbuh Good catch. It now also makes the decorated services public. Also added tests for this.

I think the tests are failing because Symfony changed from default public services to default private services. I do added the setPublic call:

HttplugBundle/DependencyInjection/HttplugExtension.php

Lines 326 to 330 in 83ae982

	$container
	->register($serviceId, PluginClient::class)
	->setPublic($arguments['public'])
	->setFactory([new Reference(PluginClientFactory::class), 'createClient'])
	->addArgument(new Reference($serviceId.'.client'))

That means that existing services will break that rely on the service to be public, on older Symfony installations.

What would be the best way to solve this?

Maybe we should make the default value of public dependent on the Symfony version?

[ruudk]

I made some changes to the test to check the Symfony version. I made the public configuration option null by default. That way, we know if the user actually tried to make the service public or not. It's always used in the decorated services (since they are private by default) and only used when it's a bool for the client.

The default null can be fixed in a new major version.

[dbu]

good solution! agree with how you do this.

this (implicitly) allows to set visibility to false for symfony < 3.4 be setting public to false - but i think we don't even need to add a test for that, its just until we can drop support for legacy versions of symfony.

[ruudk]

@xabbuh Tests are now fixed and green ✅ 🤗

[dbu]

thanks for this contribution @ruudk !

[ruudk]

Thanks for merging @dbu. When will it get released?

[dbu]

i am updating the changelog in #281, once that is good we can tag the release

[dbu]

done: https://github.com/php-http/HttplugBundle/releases/tag/1.12.0