Update composer.json for Symfony 4. #211

dbrumann · 2017-10-22T10:10:28Z

Q	A
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Related tickets	---
Documentation	---
License	MIT

What's in this PR?

Update Symfony-related dependencies to allow 4.x-versions to make this bundle work in Symfony 4.0 applications. This PR requires php-http/stopwatch-plugin#4 to work.

Nyholm

Thank you.

I think you also need to add the following to composer.json:

"minimum-stability": "dev",
"prefer-stable": true,

..else it will not take the dev dependencies.

Nyholm · 2017-10-22T16:31:04Z

.travis.yml

@@ -33,6 +33,8 @@ matrix:
          # Test against LTS versions
        - php: 7.0
          env: SYMFONY_VERSION=2.8.*
+        - php: 7.1
+          env: SYMFONY_VERSION=4.0.*


Write version 3.4 here instead. Then add another entry for 4.0 above the comment say "Test against LTS versions"

soullivaneuh · 2017-10-25T11:25:48Z

@Nyholm It seems to be already done: https://github.com/php-http/HttplugBundle/pull/211/files#diff-b5d0ee8c97c7abd7e3fa29b9a27d1780R59

dbu · 2017-11-13T10:09:22Z

can you please rebase this on master? i just merged #216 which should fix the fatal error.

dbu · 2017-11-13T10:11:47Z

and we need to figure out if we simply need to make those services public that we get warnings for in symfony 3.4, or if we can wire things so as to not need to get those service from the container but inject them instead.

fbourigault · 2017-11-13T12:43:38Z

All services the container complains about have to be public to keep backward compatibility!

dbu · 2017-11-13T20:28:50Z

All services the container complains about have to be public to keep backward compatibility!

they only become inaccessible in symfony 4, so thats no BC break of us. discusssion is in #218, it looks like most of them are explicitly set to private...

fbourigault · 2017-11-14T22:11:53Z

But I think we want to support Symfony 2.8 -> 4.0 in next minor release?

xabbuh · 2017-11-22T11:50:50Z

@dbrumann Any updates? Will you be able to work on this? :)

dbrumann · 2017-11-22T11:51:44Z

Very funny @xabbuh. I will update this PR on Friday

dbu · 2017-11-22T14:37:37Z

support Symfony 2.8 - 4.0 in next minor release?

unless there is a big problem popping up with that yes, i would want to support all those.

when the user updates to symfony 4, services that have been public only by accident will become private, which is imo a BC break of symfony itself, not of this bundle, and thus seems acceptable. we should nonetheless list the services in the changelog to help the users with upgrading.

xabbuh · 2017-11-24T13:54:14Z

composer.json

@@ -42,13 +42,13 @@
        "php-http/buzz-adapter": "^0.3",
        "php-http/mock-client": "^1.0",
        "symfony/phpunit-bridge": "^3.2",


What about allowing 4.0 here too?

dbu · 2017-11-24T23:08:49Z

i think the version 4 can't install because one of our dependencies does not support it. do you see which one it is?

the 3.4 build has warnings about accessing private services. i think the httplug clients should be public. for the rest we have to see, we maybe also need to change some tests to not try to get the service from the container in this way.

Nyholm · 2017-11-25T07:00:49Z

@dbrumann Do you mind if I try to make this work?

dbrumann · 2017-11-25T09:28:23Z

@Nyholm Not at all. Since prophecy and phpspec merged the Symfony 4 updates I should be able to make php-http/stopwatch-plugin pass now and then hopefully this PR as well, but feel free to add commits. I will refrain from doing any changes requiring a force push.

Nyholm · 2017-11-25T10:43:10Z

Much cleaner, thank you!

Nyholm · 2017-11-25T10:47:30Z

I think you are done with this PR. You are blocked by #221. When that PR is merged you just have to rebase and everything will be green

Nyholm · 2017-11-27T07:46:41Z

Hey @dbrumann. If you do a final rebase on master then you should be just fine. I've updated .travis.yml to test all proper symfony versions.

dbu

thanks a lot @dbrumann ! i think we should make httplug.client (and httplug.async_client?) public as well. for the other point about plugins i am ok if we merge as is but think we should then try to change that in a separate PR so they can be completely wired in the container rather than need to be public.

dbu · 2017-11-27T10:07:57Z

Tests/Functional/ServiceInstantiationTest.php

@@ -24,17 +24,17 @@ public function testHttpClient()
    {
        static::bootKernel();
        $container = static::$kernel->getContainer();
-        $this->assertTrue($container->has('httplug.client'));
-        $client = $container->get('httplug.client');


i think we should make the httplug.client service public too, it seems correct to me to reference it by that name.

According to the current configuration it is configured as private. I can restore this test, but then I would also change the public=false in the service configuration to make clear that this id can be used.

dbu · 2017-11-27T10:08:40Z

Tests/Resources/app/config/config_test.yml

@@ -26,3 +26,4 @@ httplug:
 services:
    app.http.plugin.custom:
        class: Http\Client\Common\Plugin\RedirectPlugin
+        public: true


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

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

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

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.

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.

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

agreed, lets just add a comment

dbu · 2017-11-28T16:53:03Z

Resources/config/services.xml

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


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

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

dbu · 2017-11-28T16:57:35Z

i looked at the code and don't see where the service httplug.client is defined. its neither in services.xml nor in the HttplugExtension, it seems

Nyholm · 2017-11-28T17:57:02Z

The httplug.client definition is super old code..

It is defined here: https://github.com/php-http/HttplugBundle/blame/v1.7.1/DependencyInjection/HttplugExtension.php#L53

Nyholm

As a second thought.. Why do we need any service to be public?

One should never do $this->container->get($service).

Do we specify them public here to allow the tests to pass?

fbourigault · 2017-11-28T21:20:43Z

I think we should follow the Symfony 4 trend and let those service became private in Symfony 4. Users will get deprecation notices if getting those services using ContainerInterface::get.

For the test, could we rewrite it to not write assertion on compiled container? If no adding a comment where the public=true is set would be great!

fbourigault · 2017-11-28T22:46:37Z

Could you rebase again to get #225 and really run tests against Symfony 4?

Nyholm · 2017-11-29T18:30:45Z

Thank you @dbrumann. This was merged via #228.

Nyholm requested changes Oct 22, 2017

View reviewed changes

dbu mentioned this pull request Nov 13, 2017

Add reset method to collector #216

Merged

xabbuh reviewed Nov 24, 2017

View reviewed changes

Nyholm approved these changes Nov 25, 2017

View reviewed changes

fbourigault added this to the 1.8.0 milestone Nov 27, 2017

dbu requested changes Nov 27, 2017

View reviewed changes

Denis Brumann added 5 commits November 27, 2017 23:03

Update composer.json for Symfony 4.

16aa017

Explicitly make services not marked private as public.

f6edf65

Fix service visibility in tests.

87094b3

Make replacement for httplug.client.default public.

db0b255

Fix service visibility.

693cefb

dbu reviewed Nov 28, 2017

View reviewed changes

Make main aliases public.

6beb3f0

Nyholm reviewed Nov 28, 2017

View reviewed changes

fbourigault mentioned this pull request Nov 28, 2017

Add Symfony 4 support #228

Merged

Nyholm closed this in #228 Nov 29, 2017

dbrumann deleted the update/symfony4 branch November 30, 2017 02:16

Update composer.json for Symfony 4. #211

Update composer.json for Symfony 4. #211

Uh oh!

Conversation

dbrumann commented Oct 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's in this PR?

Uh oh!

Nyholm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soullivaneuh commented Oct 25, 2017

Uh oh!

dbu commented Nov 13, 2017

Uh oh!

dbu commented Nov 13, 2017

Uh oh!

fbourigault commented Nov 13, 2017

Uh oh!

dbu commented Nov 13, 2017

Uh oh!

fbourigault commented Nov 14, 2017

Uh oh!

xabbuh commented Nov 22, 2017

Uh oh!

dbrumann commented Nov 22, 2017

Uh oh!

dbu commented Nov 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Nov 24, 2017

Uh oh!

Nyholm commented Nov 25, 2017

Uh oh!

dbrumann commented Nov 25, 2017

Uh oh!

Nyholm commented Nov 25, 2017

Uh oh!

Nyholm commented Nov 25, 2017

Uh oh!

Nyholm commented Nov 27, 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!

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 commented Nov 28, 2017

Uh oh!

Nyholm commented Nov 28, 2017

Uh oh!

Nyholm left a comment

Choose a reason for hiding this comment

Uh oh!

fbourigault commented Nov 28, 2017

Uh oh!

fbourigault commented Nov 28, 2017

dbrumann commented Oct 22, 2017 •

edited

Loading