Skip to content
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

Issue#282: call setContainer on ContainerAwareInterface #293

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

SmeTskE
Copy link

@SmeTskE SmeTskE commented Oct 27, 2017

Autowired controllers break when not calling setContainer on
ContainerAwareInterface implementations.

Autowired controllers break when not calling setContainer on
ContainerAwareInterface implementations.
Copy link
Contributor

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

The code looks good. Is it covered though?

@SmeTskE
Copy link
Author

SmeTskE commented Oct 27, 2017

Honestly not really familiar with unit tests, can you give me some pointers? I'll try to write some soon.

@greg0ire
Copy link
Contributor

I'm on my phone all this week end so it won't be easy . Read .travis.yml to see how to read them, and make a deliberate mistake in the existing part of the code you modified. If it is covered, the tests should break.

@SmeTskE
Copy link
Author

SmeTskE commented Oct 27, 2017

When I add throw new \Exception('deliberate mistake'); to the first line of the instantiateController(), the local tests fail on the following test functions:

  • Tests/Functional/AutomaticControllerInjectionsTest.php testInjections()
  • Tests/Functional/ControllerResolverTest.php testLookupMethodIsCorrectlyImplemented()
  • Tests/Functional/ControllerResolverTest.php testLookupMethodAndAopProxy()
  • Tests/Functional/ControllerResolverTest.php testAopProxyWhenNoDiMetadata()

Same effect when I add the throw right before the return.

My guess is that the code is covered, then? :)
Any way I can provide proof, or should I extend the tests?

@greg0ire
Copy link
Contributor

It means the existing code is covered indeed :) you might want to try throwing inside the if statements now, and check that they are covered too, and add more tests if they are not.

@SmeTskE
Copy link
Author

SmeTskE commented Oct 27, 2017

First and third if-statements aren't covered. I'll see what I can do with tests tomorrow.

Thanks for the feedback!

@SmeTskE
Copy link
Author

SmeTskE commented Oct 28, 2017

I can't really wrap my head around the fact the tests fail for php 5.x and not for 7.x

When I try to debug the test for the autowired controller on 7.x, the code is executed properly, but whenever I debug the controller on 5.x, it seems the code isn't executed and the test fails telling me the Route could not be found.

The HttpKernel/ControllerResolver::createController() does not get executed on 5.x with the current config (and thus the instantiateController() isn't called either).

Any ideas?

@greg0ire
Copy link
Contributor

You could try generating traces with xdebug_start_trace() and compare them

@SmeTskE
Copy link
Author

SmeTskE commented Oct 31, 2017

The third if is now covered.

The first if not yet. When would a Controller already be set on the Container, but not yet be instantiated? In a sub request a new Container would be created, no? I can't think of another scenario in which the first if ($this->container->has($class)) would validate.

Any idea?

@greg0ire
Copy link
Contributor

Maybe if a controller fowards to itself? Not sure if what I am saying is clever or utterly stupid...

@SmeTskE
Copy link
Author

SmeTskE commented Oct 31, 2017

It seems during a forward another Container is instantiated for the sub-request while forwarding. I'll try to investigate further.

@kmarques
Copy link

Any news about this pull request which may fix the issue about Symfony 3.3 Auto-wiring on controllers ?

@GuilhemN
Copy link
Collaborator

Is this ready to merge? Or are there still some things to cover with tests?

@SmeTskE
Copy link
Author

SmeTskE commented Jan 25, 2018

There's still one check to be written, but I don't know how to reproduce it with a test. #293 (comment)

Any feedback would be appreciated.

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

Successfully merging this pull request may close these issues.

5 participants