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

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

Already on GitHub?Sign in to your account

Merged

Conversation

@Simperfit
Copy link
Contributor

@SimperfitSimperfit commentedApr 23, 2019•
edited
Loading

QA
Branch?4.4
Bug fix?no
New feature?no
BC breaks?no
Deprecations?yes
Tests pass?yes
Fixed tickets#25920
LicenseMIT
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-grekasnicolas-grekas added this to thenext milestoneApr 23, 2019
@SimperfitSimperfitforce-pushed thedx/get-container-should-not-return-null branch 2 times, most recently from3d11b16 tob7e5040CompareApril 26, 2019 07:47
@SimperfitSimperfitforce-pushed thedx/get-container-should-not-return-null branch fromb7e5040 to6e010c3CompareApril 26, 2019 08:09
@derrabus
Copy link
Member

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

@SimperfitSimperfitforce-pushed thedx/get-container-should-not-return-null branch from6e010c3 to4bc8b7aCompareApril 29, 2019 05:51
@Simperfit
Copy link
ContributorAuthor

PR Updated.

@SimperfitSimperfitforce-pushed thedx/get-container-should-not-return-null branch 2 times, most recently from5e143c1 to54428ceCompareApril 29, 2019 07:46
@Simperfit
Copy link
ContributorAuthor

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

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

The deprecations seem to be triggered by theTestContainer that we use which calls thegetContainer() method of theKernel class even after the kernel was shut down when thereset() method is called.

@SimperfitSimperfitforce-pushed thedx/get-container-should-not-return-null branch fromd6e58b3 tobd6c8b0CompareMay 12, 2019 06:33
@Simperfit
Copy link
ContributorAuthor

Status: Needs Review

@SimperfitSimperfitforce-pushed thedx/get-container-should-not-return-null branch 2 times, most recently froma522f61 to1b67876CompareMay 18, 2019 06:31
@Simperfit
Copy link
ContributorAuthor

Test fixed !

Copy link
Member

@xabbuhxabbuh 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

@SimperfitSimperfitforce-pushed thedx/get-container-should-not-return-null branch 2 times, most recently from7a2b34e to50eda4bCompareAugust 9, 2019 11:48
@Simperfit
Copy link
ContributorAuthor

PR Rebased

@nicolas-grekasnicolas-grekasforce-pushed thedx/get-container-should-not-return-null branch from50eda4b toe169e1aCompareSeptember 27, 2019 10:06
@nicolas-grekas
Copy link
Member

PR rebased, ready.

@fabpot
Copy link
Member

Thank you@Simperfit.

fabpot added a commit that referenced this pull requestSep 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     <!-- seehttps://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
@fabpotfabpot merged commite169e1a intosymfony:4.4Sep 27, 2019
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
@nicolas-grekasnicolas-grekas modified the milestones:next,4.4Oct 27, 2019
This was referencedNov 12, 2019
@TomasVotruba
Copy link
Contributor

TomasVotruba commentedNov 23, 2019•
edited
Loading

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 requestApr 22, 2020
Since 5.0 the parent implementation throws an exception anyway making the overwrites obsolete, seesymfony#31202
Tobion added a commit to Tobion/symfony that referenced this pull requestApr 22, 2020
Since 5.0 the parent implementation throws an exception anyway making the overwrites obsolete, seesymfony#31202
nicolas-grekas added a commit that referenced this pull requestApr 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#31202Commits-------5ef9390 remove getContainer overwrites in tests
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull requestApr 22, 2020
Since 5.0 the parent implementation throws an exception anyway making the overwrites obsolete, seesymfony/symfony#31202
symfony-splitter pushed a commit to symfony/security-bundle that referenced this pull requestApr 22, 2020
Since 5.0 the parent implementation throws an exception anyway making the overwrites obsolete, seesymfony/symfony#31202
vjandrea pushed a commit to vjandrea/symfony that referenced this pull requestMay 3, 2020
Since 5.0 the parent implementation throws an exception anyway making the overwrites obsolete, seesymfony#31202
vjandrea pushed a commit to vjandrea/symfony that referenced this pull requestMay 3, 2020
Since 5.0 the parent implementation throws an exception anyway making the overwrites obsolete, seesymfony#31202
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@OskarStarkOskarStarkOskarStark requested changes

@fabpotfabpotfabpot approved these changes

@xabbuhxabbuhxabbuh 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.4

Development

Successfully merging this pull request may close these issues.

9 participants

@Simperfit@derrabus@ro0NL@xabbuh@nicolas-grekas@fabpot@TomasVotruba@OskarStark@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp