Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Support for ServiceProviderInterface #252

Open
wants to merge 2 commits into
base: 1.2.x
Choose a base branch
from

Conversation

Wirone
Copy link
Contributor

@Wirone Wirone commented Feb 8, 2022

@ondrejmirtes I've added support for ServiceProviderInterface and checked it against our codebase - after my changes and implementing this interface in our locators (mentioned here) I don't have errors, so rule is working properly.

But I had tough time with this mostly because of test scenario provided in #151 which IMHO does not work as expected. I started by adding ExampleController::privateServiceFromServiceProvider() but wanted to make it fail and I couldn't. After several tries I just checked what will happen if I remove $isServiceLocator from ContainerInterfacePrivateServiceRule and... tests were still green. So I've just removed those services from tests/Rules/Symfony/container.xml completely and I would like to provide proper test cases for service locator/provider but I don't know how 😅 I've tried to create some example files, loaded in new test method within ContainerInterfacePrivateServiceRuleTest, but I did not manage to achieve failing scenario, which could be fixed by my changes. Could you help me with this (or @lookyman)?

It does not check what it should, even after removing `ServiceLocator` condition in `ContainerInterfacePrivateServiceRule` tests are green.
Accessing services from `ServiceProviderInterface` (which extends `ContainerInterface`) should not trigger "Service is private".
@Wirone
Copy link
Contributor Author

Wirone commented Jun 22, 2022

@ondrejmirtes @lookyman any news on this?

@Wirone
Copy link
Contributor Author

Wirone commented Sep 9, 2022

@ondrejmirtes @lookyman friendly ping 😉

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

I don't understand why this is tested by removing code from tests. Typically you should test things by adding more code?

@Wirone
Copy link
Contributor Author

Wirone commented Sep 21, 2022

@ondrejmirtes I described it in the PR's description 🤷‍♂️ I really would like to provide proper test cases, but I had hard time with that and just asked for help here.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants