Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[DI] deprecate booting the kernel twices#32056

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@Simperfit
Copy link
Contributor

@SimperfitSimperfit commentedJun 15, 2019•
edited
Loading

QA
Branch?4.4
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?yes
Tests pass?yes
Fixed tickets#31233
LicenseMIT
Doc PR?

This adds a check to see if the kernel has been booted twices in a single test, and throw a deprecation

@nicolas-grekasnicolas-grekas added this to the4.2 milestoneJun 15, 2019
@SimperfitSimperfitforce-pushed thebugfix/throw-is-the-kernel-has-already-been-booted-in-test-before branch from25f9522 tob849aabCompareJune 17, 2019 08:41
@Koc
Copy link
Contributor

Koc commentedJun 18, 2019

This looks like BC-break

@xabbuh
Copy link
Member

And I agree with@Koc, this has potential to break lots of tests if I don't miss anything. We could deprecate this in 4.4 though and throw in 5.0 then.

@xabbuh
Copy link
Member

By the way, our test suite contains an example of a valid test that would fail with this change (see the failing CI jobs).

Simperfit reacted with thumbs up emoji

@SimperfitSimperfit changed the base branch from4.2 to4.4June 19, 2019 05:37
@SimperfitSimperfitforce-pushed thebugfix/throw-is-the-kernel-has-already-been-booted-in-test-before branch fromb849aab to29a08eeCompareJune 19, 2019 05:38
@Simperfit
Copy link
ContributorAuthor

PR rebase with 4.4, tests fixed and deprecation added.

@SimperfitSimperfitforce-pushed thebugfix/throw-is-the-kernel-has-already-been-booted-in-test-before branch from29a08ee tob5aabd9CompareJune 19, 2019 05:39
@SimperfitSimperfit changed the title[DI] throw an exception when the kernel has been booted twices[DI] deprecate booting the kernel twicesJun 20, 2019
@SimperfitSimperfitforce-pushed thebugfix/throw-is-the-kernel-has-already-been-booted-in-test-before branch fromb5aabd9 to905bec4CompareJune 21, 2019 20:25
@Simperfit
Copy link
ContributorAuthor

cc@fabpot

@fabpot
Copy link
Member

Thank you@Simperfit.

@fabpotfabpot merged commit905bec4 intosymfony:4.4Jul 8, 2019
fabpot added a commit that referenced this pull requestJul 8, 2019
This PR was merged into the 4.4 branch.Discussion----------[DI] deprecate booting the kernel twices| Q             | A| ------------- | ---| Branch?       | 4.4| Bug fix?      | yes| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->| BC breaks?    | no     <!-- seehttps://symfony.com/bc -->| Deprecations? | yes <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->| Fixed tickets |#31233   <!-- #-prefixed issue number(s), if any -->| License       | MIT| Doc PR        | ? <!-- required for new features --><!--Replace this notice by a short README for your feature/bugfix. This will help peopleunderstand your PR and can be used as a start for the documentation.Additionally (seehttps://symfony.com/roadmap): - Bug fixes must be submitted against the lowest maintained 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 branch 4.4. - Legacy code removals go to the master branch.-->This adds a check to see if the kernel has been booted twices in a single test, and throw a deprecationCommits-------905bec4 [DI] throw an exception when the kernel has been booted twices
@teohhanhui
Copy link
Contributor

teohhanhui commentedAug 26, 2019•
edited
Loading

This means we cannot boot the kernel, set some state in a service (e.g. fixture data in memory), then create the client to test requests.

WDYT about not (re)booting the kernel if it's already booted?

Edit: Never mind. This wouldn't work ascreateClient takes kernel options, and we can't be sure that the already booted kernel was booted with the requested options. 😞

nicolas-grekas added a commit that referenced this pull requestOct 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
@acasademont
Copy link
Contributor

acasademont commentedNov 21, 2019•
edited
Loading

Hi@Simperfit We just upgraded to SF 4.4 and now there's this deprecation notice, but tbh, we don't know how to fix it. The docs say that you specifically need to create 2 clients here, but this is now not allowed. Actually, doing what the docs say leads to an exception in SF 5

https://symfony.com/doc/current/testing/insulating_clients.html

What's the proper upgrade path if we need multiple clients?

glhms reacted with thumbs up emoji

@chalasr
Copy link
Member

@acasademont Please open a new issue. Thank you

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

Reviewers

@xabbuhxabbuhxabbuh left review comments

@fabpotfabpotfabpot approved these changes

+1 more reviewer

@ro0NLro0NLro0NL left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.2

Development

Successfully merging this pull request may close these issues.

10 participants

@Simperfit@Koc@xabbuh@fabpot@teohhanhui@acasademont@chalasr@ro0NL@nicolas-grekas@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp