Skip to content

[FrameworkBundle] Add @return TestContainer to KernelTestCase::getContainer() #44695

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

fancyweb
Copy link
Contributor

Q A
Branch? 6.1
Bug fix? no
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

From what I understood, test.service_container can only be an instance of TestContainer. Changing the return type allows autocompletion for useful methods such as getServiceIds() and getRemovedIds() (I didn't understand why they didn't showed up in my test). That's a DX improvement, can it be applied on a lower branch?

@nicolas-grekas
Copy link
Member

That's a BC break, but we can do it by using @return instead. And that's an improvement as you said, so 6.1 :)

@fancyweb fancyweb force-pushed the fwb/kernel-test-case-get-container-return branch from 8f8682b to e7730dc Compare December 18, 2021 09:24
@fancyweb fancyweb changed the title [FrameworkBundle] Change KernelTestCase::getContainer() return type to TestContainer [FrameworkBundle] Add @return TestContainer to KernelTestCase::getContainer() Dec 18, 2021
@fancyweb
Copy link
Contributor Author

Damn I was sure this class was internal 😅 I tested with @return and it works too so it's fine for me 😄

@nicolas-grekas
Copy link
Member

One test to fix and good to me.

@fancyweb fancyweb force-pushed the fwb/kernel-test-case-get-container-return branch 2 times, most recently from fbbe6b9 to f36e6f3 Compare December 18, 2021 10:18
@fancyweb fancyweb force-pushed the fwb/kernel-test-case-get-container-return branch from f36e6f3 to a6bc80c Compare December 18, 2021 10:23
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

(once green)

@fancyweb fancyweb force-pushed the fwb/kernel-test-case-get-container-return branch from a6bc80c to 812619c Compare December 18, 2021 10:37
@fancyweb fancyweb merged commit e458492 into symfony:6.1 Dec 18, 2021
@fancyweb fancyweb deleted the fwb/kernel-test-case-get-container-return branch December 18, 2021 10:55
nicolas-grekas added a commit that referenced this pull request May 30, 2022
…rnal class (xabbuh)

This PR was merged into the 6.1 branch.

Discussion
----------

[FrameworkBundle] update docblock to not expose the internal class

| Q             | A
| ------------- | ---
| Branch?       | 6.1
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #46483
| License       | MIT
| Doc PR        |

The change was introduced in #44695 motivated by the idea to be able to use the `getServiceIds()` and `getRemovedIds()` methods which are Symfony-specific methods. Exposing the `TestContainer` has the drawback of static analysis tools complaining about making use of internal classes. Since the interesting methods are part of the base `Container` class which is not internal I propose to change the docblock to return an instance of this class instead.

Commits
-------

f30db82 update docblock to not expose the internal class
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.

3 participants