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

[FrameworkBundle] Fix test-container on kernel reboot, revert to returning the real container from Client::getContainer() #27501

Merged
merged 1 commit into from Jun 6, 2018

Conversation

Projects
None yet
8 participants
@nicolas-grekas
Member

nicolas-grekas commented Jun 5, 2018

Q A
Branch? 4.1
Bug fix? yes
New feature? -
BC breaks? yes
Deprecations? no
Tests pass? yes
Fixed tickets #27494
License MIT
Doc PR -

By making Client::getContainer() return the new test container, we broke BC, as spotted in linked issue.

Always use static::$container in your tests instead.

While reverting to returning the real container, I noticed we have a serious design issue in the way the test container currently works: because the kernel can be rebooted, we cannot inject the container directly, but have to go through the kernel all the time. Fixing this forces doing a BC break on the constructor of TestContainer. Since this is a new class and since it's mostly internal, I think we should do it now. I've marked the class as internal to further strengthen this.

private function getPublicContainer()
{
$this->kernel->boot();

This comment has been minimized.

@stof

stof Jun 5, 2018

Member

Haven't we made a change in 4.1 implying that booting an already booted kernel is not a no-op ? That's would be a bad side-effect then here.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 5, 2018

Member

I reverted the side-effect so this should be OK, but to be safe and strict, I replaced this call by an exception when the container is called while the kernel is not booted.

@stof

This comment has been minimized.

Member

stof commented Jun 5, 2018

Btw, this also solves the issue we faced in #27037 as the only way to access private services now is through a new API (so any 3.4 deprecation is an actual one)

[FrameworkBundle] Fix test-container on kernel reboot, revert to retu…
…rning the real container from Client::getContainer()
@arderyp

This comment has been minimized.

Contributor

arderyp commented Jun 6, 2018

sounds good! Is there going to be accompanying documentation regarding what you (@nicolas-grekas) mentioned about static::$container and what you (@stof) mentioned about the "new API" for accessing the test container? I'm not sure I understand fully at the moment.

@pamil

pamil approved these changes Jun 6, 2018

Looks good to me, fixes the BC break and works with Sylius :)

@fabpot

fabpot approved these changes Jun 6, 2018

@fabpot

This comment has been minimized.

Member

fabpot commented Jun 6, 2018

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 6764d4e into symfony:4.1 Jun 6, 2018

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Jun 6, 2018

bug #27501 [FrameworkBundle] Fix test-container on kernel reboot, rev…
…ert to returning the real container from Client::getContainer() (nicolas-grekas)

This PR was merged into the 4.1 branch.

Discussion
----------

[FrameworkBundle] Fix test-container on kernel reboot, revert to returning the real container from Client::getContainer()

| Q             | A
| ------------- | ---
| Branch?       | 4.1
| Bug fix?      | yes
| New feature?  | -
| BC breaks?    | yes
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #27494
| License       | MIT
| Doc PR        | -

By making `Client::getContainer()` return the new test container, we broke BC, as spotted in linked issue.

Always use `static::$container` in your tests instead.

While reverting to returning the real container, I noticed we have a serious design issue in the way the test container currently works: because the kernel can be rebooted, we cannot inject the container directly, but have to go through the kernel all the time. Fixing this forces doing a BC break on the constructor of `TestContainer`. Since this is a new class and since it's mostly internal, I think we should do it now. I've marked the class as internal to further strengthen this.

Commits
-------

6764d4e [FrameworkBundle] Fix test-container on kernel reboot, revert to returning the real container from Client::getContainer()
@stof

This comment has been minimized.

Member

stof commented Jun 6, 2018

@arderyp the PR introducing the test container made it available in 2 different ways: static::$container (which is totally new in 4.1) and $client->getContainer() (changing the API), while static::$kernel->getContainer() was still returning the public container.
After this PR, only the new static::$container API exposes the test container.

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:client-real-container branch Jun 6, 2018

@arderyp

This comment has been minimized.

Contributor

arderyp commented Jun 6, 2018

awesome! So, for clarity's sake,

// on S3.4 WebTestCase extension
$this->client = static::createClient();
$this->container = $this->client->getContainer();

should be replaced with:

// on S4.x WebTestCase extension
$this->client = static::createClient();
$this->container = static::$container;

Is that right?

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 6, 2018

This line is not needed: $this->container = static::$container;, as there is only one $container property per class, static or not, its the same here.

@arderyp

This comment has been minimized.

Contributor

arderyp commented Jun 6, 2018

So no need to createClient() at all? Nice, thanks for the clarification!

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 6, 2018

Yes you need it! I only told about the 2nd line.

@arderyp

This comment has been minimized.

Contributor

arderyp commented Jun 6, 2018

Sorry, I'm an idiot... read too quickly, I understand now. Thanks again.

@fabpot fabpot referenced this pull request Jun 25, 2018

Merged

Release v4.1.1 #27706

@jifer

This comment has been minimized.

My friend. Changes made in this file (or commit) made your post invalid:
https://symfony.com/blog/new-in-symfony-4-1-simpler-service-testing
All private services (including Doctrine\ORM\EntityManagerInterface) can't be fetched anymore. All our unit test are down.

Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: The "Doctrine\ORM\EntityManagerInterface" service or alias has been removed or inlined when the container was compiled. You should either make it public, or stop using the container directly and use dependency injection instead.

This comment has been minimized.

Member

xabbuh replied Jun 27, 2018

Please open a new issue if you think that you found a bug. Comments on commits are likely to get lost.

This comment has been minimized.

Member

stof replied Jun 27, 2018

Well, as we reverted part of the change here, the blog post may indeed need to be updated to show only the way we kept.

This comment has been minimized.

jifer replied Jun 27, 2018

I did so. On first go I thought my problem is solved, but unfortunately "new way" doesn't work for me
#27741

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