Skip to content

Update composer.json for Symfony 4. #211

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

Closed
wants to merge 6 commits into from
Closed

Update composer.json for Symfony 4. #211

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
16aa017
Update composer.json for Symfony 4.
Oct 22, 2017
f6edf65
Explicitly make services not marked private as public.
Nov 24, 2017
87094b3
Fix service visibility in tests.
Nov 24, 2017
db0b255
Make replacement for httplug.client.default public.
Nov 24, 2017
693cefb
Fix service visibility.
Nov 27, 2017
6beb3f0
Make main aliases public.
Nov 28, 2017
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
4 changes: 3 additions & 1 deletion DependencyInjection/HttplugExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use Http\Message\Authentication\Wsse;
use Psr\Http\Message\UriInterface;
use Symfony\Component\Config\FileLocator;
use Symfony\Component\DependencyInjection\Alias;
use Symfony\Component\DependencyInjection\ChildDefinition;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Definition;
Expand Down Expand Up @@ -103,7 +104,7 @@ private function configureClients(ContainerBuilder $container, array $config)
// If we do not have a client named 'default'
if (!isset($config['clients']['default'])) {
// Alias the first client to httplug.client.default
$container->setAlias('httplug.client.default', 'httplug.client.'.$first);
$container->setAlias('httplug.client.default', new Alias('httplug.client.'.$first, true));
}
}
}
Expand Down Expand Up @@ -297,6 +298,7 @@ private function configureClient(ContainerBuilder $container, $clientName, array

$container
->register($serviceId, PluginClient::class)
->setPublic(true)
->setFactory([new Reference(PluginClientFactory::class), 'createClient'])
->addArgument(new Reference($serviceId.'.client'))
->addArgument(
Expand Down
18 changes: 9 additions & 9 deletions Resources/config/services.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,47 +4,47 @@
xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd">

<services>
<service id="httplug.strategy" class="Http\HttplugBundle\Discovery\ConfiguredClientsStrategy">
<service id="httplug.strategy" class="Http\HttplugBundle\Discovery\ConfiguredClientsStrategy" public="true">
<argument type="service" id="httplug.auto_discovery.auto_discovered_client" on-invalid="null"/>
<argument type="service" id="httplug.auto_discovery.auto_discovered_async" on-invalid="null"/>
<tag name="kernel.event_subscriber"/>
</service>

<service id="httplug.auto_discovery.auto_discovered_client" class="Http\Client\HttpClient">
<service id="httplug.auto_discovery.auto_discovered_client" class="Http\Client\HttpClient" public="true">
<factory class="Http\Discovery\HttpClientDiscovery" method="find" />
</service>

<service id="httplug.auto_discovery.auto_discovered_async" class="Http\Client\HttpAsyncClient">
<service id="httplug.auto_discovery.auto_discovered_async" class="Http\Client\HttpAsyncClient" public="true">
<factory class="Http\Discovery\HttpAsyncClientDiscovery" method="find" />
</service>

<!-- Discovery with autowiring support for Symfony 3.3+ -->
<service id="httplug.message_factory.default" class="Http\Message\MessageFactory">
<service id="httplug.message_factory.default" class="Http\Message\MessageFactory" public="true">
<factory class="Http\Discovery\MessageFactoryDiscovery" method="find" />
</service>
<service id="Http\Message\MessageFactory" alias="httplug.message_factory" public="false" />
<service id="Http\Message\RequestFactory" alias="httplug.message_factory" public="false" />
<service id="Http\Message\ResponseFactory" alias="httplug.message_factory" public="false" />

<service id="httplug.stream_factory.default" class="Http\Message\StreamFactory">
<service id="httplug.stream_factory.default" class="Http\Message\StreamFactory" public="true">
<factory class="Http\Discovery\StreamFactoryDiscovery" method="find" />
</service>
<service id="Http\Message\StreamFactory" alias="httplug.stream_factory" public="false" />

<service id="httplug.uri_factory.default" class="Http\Message\UriFactory">
<service id="httplug.uri_factory.default" class="Http\Message\UriFactory" public="true">
<factory class="Http\Discovery\UriFactoryDiscovery" method="find" />
</service>
<service id="Http\Message\UriFactory" alias="httplug.uri_factory" public="false" />

<service id="httplug.async_client.default" class="Http\Client\HttpAsyncClient">
<service id="httplug.async_client.default" class="Http\Client\HttpAsyncClient" public="true">
<factory class="Http\Discovery\HttpAsyncClientDiscovery" method="find" />
</service>
<service id="Http\Client\HttpAsyncClient" alias="httplug.async_client.default" public="false" />

<service id="httplug.client.default" class="Http\Client\HttpClient">
<service id="httplug.client.default" class="Http\Client\HttpClient" public="true">
<factory class="Http\Discovery\HttpClientDiscovery" method="find" />
</service>
<service id="Http\Client\HttpClient" alias="httplug.client" public="false" />
<service id="Http\Client\HttpClient" alias="httplug.client" public="true" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

hm, is this what we need to make public? this is just the alias to enable autowiring. this is not defining httplug.client

Copy link
Member

Choose a reason for hiding this comment

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

I agree. These aliases shouldn't be public (in Symfony, they are private too).


<!-- PluginClientFactory -->
<service id="Http\Client\Common\PluginClientFactory" class="Http\Client\Common\PluginClientFactory" public="false" />
Expand Down
3 changes: 3 additions & 0 deletions Tests/Resources/app/config/config_test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,6 @@ httplug:
services:
app.http.plugin.custom:
class: Http\Client\Common\Plugin\RedirectPlugin
public: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do the plugins need to be public? that smells like an issue in how we wire them into the plugin-client, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't just because test assertions are written against the compiled container instead of the container builder?

Copy link
Collaborator

Choose a reason for hiding this comment

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

indeed, seems the problem is in ServiceInstantiationTest::testProfilingShouldNotChangeServiceReference where we get the container from the kernel and then assert that nothing messes with the plugin service. i don't know if there is an easy solution for this - maybe simply add a comment here to avoid confusion? # plugin services usually do not need to be public. this one is made public so that we can do functional tests on the service

Copy link
Member

Choose a reason for hiding this comment

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

That's basically the approach we still need to document with now "private by default" services in Symfony 4 (see symfony/symfony-docs#8097). In this case, we do not need to work around with an alias.

Copy link
Author

Choose a reason for hiding this comment

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

Are there any preferences as to whether I should use a test alias or just add a comment? I personally prefer the comment. Making this public for tests because we want to get it from the container feels more honest to me.

Copy link
Member

Choose a reason for hiding this comment

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

The alias IMO is only needed when the service would be present outside the test too (which is not the case here).

Copy link
Collaborator

Choose a reason for hiding this comment

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

agreed, lets just add a comment

# plugin services usually do not need to be public.
# this one is made public so that we can do functional tests on the service
24 changes: 12 additions & 12 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,14 @@
"php-http/cache-plugin": "^1.4",
"php-http/logger-plugin": "^1.0",
"php-http/stopwatch-plugin": "^1.0",
"symfony/options-resolver": "^2.8 || ^3.0",
"symfony/event-dispatcher": "^2.8 || ^3.0",
"symfony/framework-bundle": "^2.8 || ^3.0",
"symfony/options-resolver": "^2.8 || ^3.0 || ^4.0",
"symfony/event-dispatcher": "^2.8 || ^3.0 || ^4.0",
"symfony/framework-bundle": "^2.8 || ^3.0 || ^4.0",
"php-http/message": "^1.4",
"php-http/discovery": "^1.0",
"twig/twig": "^1.18 || ^2.0",
"symfony/asset": "^2.8 || ^3.0",
"symfony/dependency-injection": "^2.8.3 || ^3.0.3"
"symfony/asset": "^2.8 || ^3.0 || ^4.0",
"symfony/dependency-injection": "^2.8.3 || ^3.0.3 || ^4.0"
},
"require-dev": {
"phpunit/php-token-stream": "^1.1.8",
Expand All @@ -41,13 +41,13 @@
"php-http/buzz-adapter": "^0.3",
"php-http/mock-client": "^1.0",
"symfony/phpunit-bridge": "^3.3 || ^4.0",
"symfony/twig-bundle": "^2.8 || ^3.0",
"symfony/twig-bridge": "^2.8 || ^3.0",
"symfony/web-profiler-bundle": "^2.8 || ^3.0",
"symfony/finder": "^2.7 || ^3.0",
"symfony/cache": "^3.1",
"symfony/browser-kit": "^2.8 || ^3.0",
"symfony/dom-crawler": "^2.8 || ^3.0",
"symfony/twig-bundle": "^2.8 || ^3.0 || ^4.0",
"symfony/twig-bridge": "^2.8 || ^3.0 || ^4.0",
"symfony/web-profiler-bundle": "^2.8 || ^3.0 || ^4.0",
"symfony/finder": "^2.7 || ^3.0 || ^4.0",
"symfony/cache": "^3.1 || ^4.0",
"symfony/browser-kit": "^2.8 || ^3.0 || ^4.0",
"symfony/dom-crawler": "^2.8 || ^3.0 || ^4.0",
"polishsymfonycommunity/symfony-mocker-container": "^1.0",
"matthiasnoback/symfony-dependency-injection-test": "^1.1 || ^2.0",
"nyholm/nsa": "^1.1"
Expand Down