Skip to content

[FrameworkBundle] WebTestCase KernelBrowser::getContainer null return type #31202

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

Merged

Conversation

Simperfit
Copy link
Contributor

@Simperfit Simperfit commented Apr 23, 2019

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

As @stof suggested in the #25920 I started deprecating the behaviour of returning null when the container is non booted / null.

If this is not wanted we should close both the PR and the issue ;).

@nicolas-grekas nicolas-grekas added this to the next milestone Apr 23, 2019
@Simperfit Simperfit force-pushed the dx/get-container-should-not-return-null branch 2 times, most recently from 3d11b16 to b7e5040 Compare April 26, 2019 07:47
@Simperfit Simperfit force-pushed the dx/get-container-should-not-return-null branch from b7e5040 to 6e010c3 Compare April 26, 2019 08:09
@derrabus
Copy link
Member

Wouldn't it make sense to move that deprecation into the kernel? 🤔

@Simperfit Simperfit force-pushed the dx/get-container-should-not-return-null branch from 6e010c3 to 4bc8b7a Compare April 29, 2019 05:51
@Simperfit
Copy link
Contributor Author

PR Updated.

@Simperfit Simperfit force-pushed the dx/get-container-should-not-return-null branch 2 times, most recently from 5e143c1 to 54428ce Compare April 29, 2019 07:46
@Simperfit
Copy link
Contributor Author

There are alot of deprecation with this being deprecated directly in the Kernel, I see 2 way of fixing this:

  • marking all test as expecting this deprecation ( :( )
  • Fixing all the test in this PR, but it seems like modify more than 90 test.

WDYT ?

@ro0NL
Copy link
Contributor

ro0NL commented Apr 30, 2019

Fixing all the test in this PR, but it seems like modify more than 90 test.

is there a legit case that relies on a nulled container? We should check before shipping :)

in general i think tests should be fixed yes, unless it tests a behavior that becomes irrelevant in 5.0; then it's a legacy test.

@xabbuh
Copy link
Member

xabbuh commented Apr 30, 2019

The deprecations seem to be triggered by the TestContainer that we use which calls the getContainer() method of the Kernel class even after the kernel was shut down when the reset() method is called.

@Simperfit Simperfit force-pushed the dx/get-container-should-not-return-null branch from d6e58b3 to bd6c8b0 Compare May 12, 2019 06:33
@Simperfit
Copy link
Contributor Author

Status: Needs Review

@Simperfit Simperfit force-pushed the dx/get-container-should-not-return-null branch 2 times, most recently from a522f61 to 1b67876 Compare May 18, 2019 06:31
@Simperfit
Copy link
Contributor Author

Test fixed !

Copy link
Member

@xabbuh xabbuh left a comment

Choose a reason for hiding this comment

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

with minor comment

@Simperfit Simperfit force-pushed the dx/get-container-should-not-return-null branch 2 times, most recently from 7a2b34e to 50eda4b Compare August 9, 2019 11:48
@Simperfit
Copy link
Contributor Author

PR Rebased

@nicolas-grekas nicolas-grekas force-pushed the dx/get-container-should-not-return-null branch from 50eda4b to e169e1a Compare September 27, 2019 10:06
@nicolas-grekas
Copy link
Member

PR rebased, ready.

@fabpot
Copy link
Member

fabpot commented Sep 27, 2019

Thank you @Simperfit.

fabpot added a commit that referenced this pull request Sep 27, 2019
…ner null return type (Simperfit)

This PR was merged into the 4.4 branch.

Discussion
----------

[FrameworkBundle] WebTestCase KernelBrowser::getContainer null return type

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | yes <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #25920  <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        |  <!-- required for new features -->

<!--
Write a short README entry for your feature/bugfix here (replace this comment block.)
This will help people understand your PR and can be used as a start of the Doc PR.
Additionally:
 - Bug fixes must be submitted against the lowest branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against the master branch.
-->

As @stof suggested in the #25920 I started deprecating the behaviour of returning null when the container is non booted / null.

If this is not wanted we should close both the PR and the issue ;).

Commits
-------

e169e1a [FrameworkBundle] WebTestCase KernelBrowser::getContainer null return type
@fabpot fabpot merged commit e169e1a into symfony:4.4 Sep 27, 2019
nicolas-grekas added a commit that referenced this pull request Oct 23, 2019
…eal one instead (nicolas-grekas)

This PR was merged into the 4.4 branch.

Discussion
----------

[FrameworkBundle] Don't reset the test container but the real one instead

| Q             | A
| ------------- | ---
| Branch?       | 4.4 for features / 3.4 or 4.3 for bug fixes <!-- see below -->
| Bug fix?      | yes/no
| New feature?  | yes/no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | yes/no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Fix #... <!-- prefix each issue number with "Fix #", if any -->
| License       | MIT
| Doc PR        | -

After #31202 and #32056, the tearDown method keeps throwing deprecation notices about "Getting the container from a non-booted kernel". The reason is that resetting the test-container calls `$kernel->getContainer()` while the kernel has been shut down.

This fixes it and a few other glitches found meanwhile.

Commits
-------

8e16143 [FrameworkBundle] Dont reset the test container but the real one instead
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
This was referenced Nov 12, 2019
@TomasVotruba
Copy link
Contributor

TomasVotruba commented Nov 23, 2019

Pretty nasty BC break between 4.3 and 4.4, took me 2 hours to figure out 😫

This is the fix if anyone needs it: https://github.com/Symplify/Symplify/pull/1681/files

Tobion added a commit to Tobion/symfony that referenced this pull request Apr 22, 2020
Since 5.0 the parent implementation throws an exception anyway making the overwrites obsolete, see symfony#31202
Tobion added a commit to Tobion/symfony that referenced this pull request Apr 22, 2020
Since 5.0 the parent implementation throws an exception anyway making the overwrites obsolete, see symfony#31202
nicolas-grekas added a commit that referenced this pull request Apr 22, 2020
…s (Tobion)

This PR was merged into the 5.0 branch.

Discussion
----------

[FrameworkBundle] remove getContainer overwrites in tests

| Q             | A
| ------------- | ---
| Branch?       | 5.0
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       |
| Doc PR        |

Since 5.0 the parent implementation throws an exception anyway making the overwrites obsolete, see #31202

Commits
-------

5ef9390 remove getContainer overwrites in tests
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull request Apr 22, 2020
Since 5.0 the parent implementation throws an exception anyway making the overwrites obsolete, see symfony/symfony#31202
symfony-splitter pushed a commit to symfony/security-bundle that referenced this pull request Apr 22, 2020
Since 5.0 the parent implementation throws an exception anyway making the overwrites obsolete, see symfony/symfony#31202
vjandrea pushed a commit to vjandrea/symfony that referenced this pull request May 3, 2020
Since 5.0 the parent implementation throws an exception anyway making the overwrites obsolete, see symfony#31202
vjandrea pushed a commit to vjandrea/symfony that referenced this pull request May 3, 2020
Since 5.0 the parent implementation throws an exception anyway making the overwrites obsolete, see symfony#31202
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants