8000 Symfony 2.8.10 no longer allows the ContainerBuilder as base class · Issue #19858 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Symfony 2.8.10 no longer allows the ContainerBuilder as base class #19858

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

Jump to bottom
Closed
RonRademaker opened this issue Sep 5, 2016 · 10 comments
Closed

Symfony 2.8.10 no longer allows the ContainerBuilder as base class #19858

RonRademaker opened this issue Sep 5, 2016 · 10 comments
Labels
BC Break Bug DependencyInjection Status: Needs Review

Comments

@RonRademaker
Copy link

We use a custom DIC based on ContainerBuilder in one of our projects. Our Kernel overwrites the getContainerBaseClass in order to make sure this DIC is used instead of Container. As of Symfony 2.8.10 this is broken due to symfony/dependency-injection@e0eeba7#diff-7f4e44bf5b03bfea768fbd537f88c20bR147

Reproduction code:

composer.json

{
    "require": {
        "symfony/dependency-injection": "2.8.10",
        "symfony/http-kernel": "2.8.10",
        "symfony/config": "2.8.10"
    },
    "autoload": {
        "classmap": [
            "src/"
        ]
    }
}

src/Kernel.php

<?php

use Symfony\Component\Config\Loader\LoaderInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\HttpKernel\Kernel as BaseKernel;

class Kernel extends BaseKernel
{
    public function registerBundles()
    {
        return [];
    }

    public function registerContainerConfiguration(LoaderInterface $loader)
    {

    }

    protected function getContainerBaseClass()
    {
        return ContainerBuilder::class;
    }

}

src/issue.php

<?php

require 'vendor/autoload.php';

$kernel = new Kernel('test', true);
$kernel->boot();

Result:

PHP Fatal error:  Uncaught exception 'Symfony\Component\DependencyInjection\Exception\BadMethodCallException' with message 'Setting service "kernel" on a frozen container is not allowed.' in vendor/symfony/dependency-injection/ContainerBuilder.php:396
Stack trace:
#0 vendor/symfony/http-kernel/Kernel.php(520): Symfony\Component\DependencyInjection\ContainerBuilder->set('kernel', Object(Kernel))
#1 vendor/symfony/http-kernel/Kernel.php(133): Symfony\Component\HttpKernel\Kernel->initializeContainer()
#2 src/issue.php(6): Symfony\Component\HttpKernel\Kernel->boot()
#3 {main}
  thrown in vendor/symfony/dependency-injection/ContainerBuilder.php on line 396

Fatal error: Uncaught exception 'Symfony\Component\DependencyInjection\Exception\BadMethodCallException' with message 'Setting service "kernel" on a frozen container is not allowed.' in vendor/symfony/dependency-injection/ContainerBuilder.php:396
Stack trace:
#0 vendor/symfony/http-kernel/Kernel.php(520): Symfony\Component\DependencyInjection\ContainerBuilder->set('kernel', Object(Kernel))
#1 vendor/symfony/http-kernel/Kernel.php(133): Symfony\Component\HttpKernel\Kernel->initializeContainer()
#2 src/issue.php(6): Symfony\Component\HttpKernel\Kernel->boot()
#3 {main}
  thrown in vendor/symfony/dependency-injection/ContainerBuilder.php on line 396

The solution could be to extend from Container instead of ContainerBuilder, but at this time I don't yet know it that has any other consequences.
@nicolas-grekas
Copy link
Member

Relates to #19704

@allflame
Copy link
Contributor
allflame commented Sep 5, 2016 •
edited
Loading

@nicolas-grekas Told you that was a BC break.
I was pretty sure that somewhere it was used in a way it was not supposed to be
I'm pretty sure this should not be possible by design (of frozen container)
$this->container = new $class(); $this->container->set('kernel', $this);

@allflame
Copy link
Contributor
allflame commented Sep 5, 2016 •
edited
Loading

@nicolas-grekas I gave it a bit more thought and what I suspect that the core problem is PhpDumper losing information about "synthetic" services in the dumped container so there is now way even in default Symfony scenario to check whether calls to Container::set(through dumped container) are bad or not (e.g. only synthetic services should be allowed and only once per service). Another possibility is that it is a mix of conceptions - isFrozen() was initially designed to check "whether it's possible to register parameters" but over time changed to "whether it's possible to register services too".
@RonRademaker Regarding your problem: I think it should be safe for you just to extend Container, given you overriden only getContainerBaseClass in Kernel with FCN of your own implementation, but i think this is hardly the case.

@RonRademaker
Copy link
Author

@RonRademaker Regarding your problem: I think it should be safe for you just to extend Container, given you overriden only getContainerBaseClass in Kernel with FCN of your own implementation, but i think this is hardly the case.

Indeed we've overridden some more in the Kernel, what cases could lead to problems? Our changes are related to the locations of log and cache files, the bundles to include, the default services file to load and the name of the cached container.

@allflame
Copy link
Contributor
allflame commented Sep 5, 2016

@RonRademaker it seems the getContainerBaseClass function is responsible only for the "extends ..." part in the file, produced by PhpDumper (apart from method mapping to avoid parsing errors).
From what you've wrote nothing has impact on container code, so I suggest you change to extends Container instead of extends ContainerBuilder and at least Symfony part of you application won't differ. What is the impact on your own code I don't know.

@nicolas-grekas
Copy link
Member

Let's revert

@nicolas-grekas
Copy link
Member

@RonRademaker can you confirm your issue is fixed and everything works fine after using Container instead of ContainerBuilder? Because I looked again at the code, and you fall into a corner case that shouldn't really work, even if it did "by chance" in your case...

@Nicofuma
8000 Copy link
Contributor
Nicofuma commented Sep 6, 2016

I had the exact same issue with all my synthetic services and using Container fixed it yes

@RonRademaker
Copy link
Author

@nicolas-grekas I haven't been able to fully test, because our use case is a framework that runs many sites and I can't risk breaking all. So, some others will have to look into this as well.

@nicolas-grekas nicolas-grekas mentioned this issue Sep 6, 2016
Merged
@nicolas-grekas
Copy link
Member

Instead of reverting #19704, I propose to fix the issue another way: #19870

fabpot added a commit that referenced this issue Sep 6, 2016
@fabpot
bug #19870 [DI] Fix setting synthetic services on ContainerBuilder (n…
42b2641
< 6333 /div> 
…icolas-grekas)

This PR was merged into the 2.7 branch.

Discussion
----------

[DI] Fix setting synthetic services on ContainerBuilder

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| Tests pass?   | yes
| Fixed tickets | #19858
| License       | MIT

`ContainerBuilder` doesn't allow setting a service while it's frozen and has no corresponding definition.
Yet, the base `Container` doesn't have this limitation.

See linked issue for some context.

Commits
-------

42244f2 [DI] Fix setting synthetic services on ContainerBuilder
@fabpot fabpot closed this as completed Sep 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
BC Break Bug DependencyInjection Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@fabpot @javiereguiluz @nicolas-grekas @Nicofuma @RonRademaker @allflame @carsonbot
0