Skip to content

[DI] Bug with sharing of non-public service #23979

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.

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
ghost opened this issue Aug 24, 2017 · 11 comments
Closed

[DI] Bug with sharing of non-public service #23979

ghost opened this issue Aug 24, 2017 · 11 comments

Comments

@ghost
Copy link

ghost commented Aug 24, 2017

Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no
Symfony version 3.3.6
PHP version 7.1

If service declared as non-public it stops to be shared, means for every consumer of such a service a new one will be instantiated. Explicit shared: true makes no difference. With public: true works as expected (shared by default).

Behaviour has been noticed for compiled ContainerBuilder, but not dumped.

@ghost ghost changed the title [DI] Bug with sharing of not public service [DI] Bug with sharing of non-public service Aug 24, 2017
@Nek-
Copy link
Contributor

Nek- commented Aug 25, 2017

[edit] see the next comment, the issue exists. #23979 (comment)
Hello,

Here is a reproducer that show there is no problem. I used Symfony 3.3.6 to make my test.

$ composer show
symfony/symfony                      v3.3.6  The Symfony PHP framework
<?php

namespace GlobalNamespace\Foobar {
    class NewLine {
        public function __construct()
        {
            echo "I'm new line and i'm used everywhere";
            $this->hello();
        }

        public function hello()
        {
            echo "\n";
        }
    }
}

namespace GlobalNamespace\Foo {
    class Stof {

        private $nl;

        public function __construct(\GlobalNamespace\Foobar\Newline $newline)
        {
            $this->nl = $newline;
        }

        public function hello()
        {
            echo "You missed something line 150, please change it !";
            $this->nl->hello();
        }
    }
}

namespace GlobalNamespace\Bar {
    class Fabpot {

        private $nl;

        public function __construct(\GlobalNamespace\Foobar\Newline $newline)
        {
            $this->nl = $newline;
        }

        public function hello()
        {
            echo "[MERGED] Thanks for your contribution !";
            $this->nl->hello();
        }
    }
}

namespace GlobalNamespace {

    require 'vendor/autoload.php';

    use GlobalNamespace\Bar\Fabpot;
    use GlobalNamespace\Foo\Stof;
    use GlobalNamespace\Foobar\NewLine;
    use Symfony\Component\DependencyInjection\ContainerBuilder;


    $container = new ContainerBuilder();

    $container->register('nl', NewLine::class)->setPublic(false);
    $container->autowire('stof', Stof::class)->setPublic(true);
    $container->autowire('fabpot', Fabpot::class)->setPublic(true);

    $container->compile();

    $container->get('stof')->hello();
    $container->get('fabpot')->hello();
}

Output:

$ php test.php
I'm new line and i'm used everywhere
You missed something line 150, please change it !
[MERGED] Thanks for your contribution !

(expected: only one call to "new line constructor"... And it is !)

Status: Reviewed

@Nek-
Copy link
Contributor

Nek- commented Aug 25, 2017

I found a bug anyway. Let's take the previous code with the following usage:

    $container = new ContainerBuilder();

    $container->register('nl', NewLine::class)->setPublic(false)->setShared(false);
    $container->autowire('stof', Stof::class)->setPublic(true);
    $container->autowire('fabpot', Fabpot::class)->setPublic(true);

    $container->compile();

    $container->get('stof')->hello();
    $container->get('fabpot')->hello();

Output:

$ php test.php
I'm new line and i'm used everywhere
You missed something line 150, please change it !
[MERGED] Thanks for your contribution !

Also, the spl hash is the same. Of course. But I asked that the instance is not shared. Here is the issue. If I set it public the issue is the same, but it works when I call it from the outside:

    $container = new ContainerBuilder();

    $container->register('nl', NewLine::class)->setPublic(true)->setShared(false);
    $container->autowire('stof', Stof::class)->setPublic(true);
    $container->autowire('fabpot', Fabpot::class)->setPublic(true);

    $container->compile();

    $container->get('stof')->hello();   // instanciates "NewLine"
    $container->get('fabpot')->hello(); // reuse the existing instance
    $container->get('nl')->hello();     // instanciates "NewLine" again

@ghost
Copy link
Author

ghost commented Aug 25, 2017

Cannot reproduce it with an "artificial" test, works fine, but with real project there is still an issue. I going to continue to work with it.

@Nek-
Copy link
Contributor

Nek- commented Aug 26, 2017

I did it for you @mikemeier :). See my last post. The issue is real.

@ro0NL
Copy link
Contributor

ro0NL commented Aug 30, 2017

@Nek- isnt that autowiring kicking in? Creating a new NewLine service, as it doesnt know about the nl service 🤔

I'd say no issue then :)

@Nek-
Copy link
Contributor

Nek- commented Aug 30, 2017

@ro0NL the autowiring doesn't instantiate many times the service as expected. (As I understand you describe the inverse case, which would make eventually sense but would still remain an issue)

The case here in a nutshell:

  1. I register NewLine as public service that should not be shared
  2. I call 2 services autowired that should generate 2 instantiations of NewLine service as it's not shared, but it fails and instantiate only once
  3. I get the NewLine service with the container directly and get the expected behavior: a new instance

The last bunch of code I posted exposes this issue.

@stof
Copy link
Member

stof commented Aug 30, 2017

@Nek- your autowiring issue is that your non-shared service is not registered for the class name as id. So it is not the one used by the autowiring (actually, it may be the one being used due to the BC layer, as you use 3.3 and not master. We may have a weird thing in the BC layer). The autowiring may be creating a new service for Newline (and services created there are shared).

@stof
Copy link
Member

stof commented Aug 30, 2017

@Nek- can you dump $container->getCompiler()->getLog() after the compilation of the container in your test ?

@Nek-
Copy link
Contributor

Nek- commented Aug 30, 2017

Here is the output:

$ php test.php
[New instance NewLine] I'm new line and i'm used everywhere
You missed something line 150, please change it !
[MERGED] Thanks for your contribution !
[New instance NewLine] I'm new line and i'm used everywhere

array(3) {
  [0]=>
  string(143) "Symfony\Component\DependencyInjection\Compiler\AutowirePass: Type "GlobalNamespace\Foobar\Newline" has been auto-registered for service "stof"."
  [1]=>
  string(147) "Symfony\Component\DependencyInjection\Compiler\RemovePrivateAliasesPass: Removed service "Psr\Container\ContainerInterface"; reason: private alias."
  [2]=>
  string(171) "Symfony\Component\DependencyInjection\Compiler\RemovePrivateAliasesPass: Removed service "Symfony\Component\DependencyInjection\ContainerInterface"; reason: private alias."
}
Click me to see the code that generates this output
<?php

namespace GlobalNamespace\Foobar {
    class NewLine {
        public function __construct()
        {
            echo "[New instance NewLine] I'm new line and i'm used everywhere";
            $this->hello();
        }

        public function hello()
        {
            echo "\n";
        }
    }
}

namespace GlobalNamespace\Foo {
    class Stof {

        private $nl;

        public function __construct(\GlobalNamespace\Foobar\Newline $newline)
        {
            $this->nl = $newline;
        }

        public function hello()
        {
            echo "You missed something line 150, please change it !";
            $this->nl->hello();
        }
    }
}

namespace GlobalNamespace\Bar {
    class Fabpot {

        private $nl;

        public function __construct(\GlobalNamespace\Foobar\Newline $newline)
        {
            $this->nl = $newline;
        }

        public function hello()
        {
            echo "[MERGED] Thanks for your contribution !";
            $this->nl->hello();
        }
    }
}

namespace GlobalNamespace {

    require 'vendor/autoload.php';

    use GlobalNamespace\Bar\Fabpot;
    use GlobalNamespace\Foo\Stof;
    use GlobalNamespace\Foobar\NewLine;
    use Symfony\Component\DependencyInjection\ContainerBuilder;


    $container = new ContainerBuilder();

    $container->register('nl', NewLine::class)->setPublic(true)->setShared(false);
    $container->autowire('stof', Stof::class)->setPublic(true);
    $container->autowire('fabpot', Fabpot::class)->setPublic(true);

    $container->compile();

    $container->get('stof')->hello();
    $container->get('fabpot')->hello();
    $container->get('nl')->hello();

    var_dump($container->getCompiler()->getLog());
}

@nicolas-grekas
Copy link
Member

#24671 to the rescue? Can you try setting the parameter? And upgrade to beta2 of course.

@nicolas-grekas
Copy link
Member

Closing as this should be fixed, as suggested. Please report back if not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants