[WIP] Remove discovery dependency #55
Conversation
|
Also, I am open for better names, I just picked factory in order to clearly separate from discovery. |
| */ | ||
| private function verifyPuliInstalled(ContainerBuilder $container) | ||
| { | ||
| if (false === $container->has('puli.discovery')) { |
There was a problem hiding this comment.
Since puli.discovery is part of another bundle this if statement will always be true. This code has to be moved to a compiler pass, or you could check if the bundle is enabled.
There was a problem hiding this comment.
I am not sure I understand why it would be true.
Although I am not against moving this to a compiler pass. Can we load a compiler pass from the extension where we check the necessity of discovery usage? Or should we move the whole discovery stuff to a compiler pass?
There was a problem hiding this comment.
In a bundle's extension class you can not see services registered in other bundles. That is why $container->has('puli.discovery') will always be false.
I suggest to save the $useDiscovery variable in a container parameter and then handle the rest in a compiler pass.
If we check if discovery is necessary in a compiler pass we need to inspect httplug.client.default, httplug.message_factory.default etc to see if they are using the standard class or not. That may be more elegant because we do not use container parameters but I believe it requires more hard coded values.
There was a problem hiding this comment.
What if we do the following: remove the fallback services and move them to a compiler pass. Then we can check in the compiler if all the services has been defined in the extension or not? (This requires to know in what order are the compilers and extensions are loaded). If any of the services are not registered, register it with the factory (and register the factory as well).
There was a problem hiding this comment.
Awesome! That would be better and easier to read/understand.
This requires to know in what order are the compilers and extensions are loaded
Different bundles extensions loads independently, order is not important. After all extensions are loaded Symfony starts with the compiler passes. (The "last minute change before compiling the container") Our compiler passes loads in the order we specify in our HttplugBundle.php file. Since the puli.discovery service is defined in an extension we are safe to verify its existens in our compiler pass.
There was a problem hiding this comment.
The order is important because of checking what services has been registered, which needs fallback, etc. But I think it is not a problem then.
sagikazarmark
force-pushed
the
httplug_factory
branch
from
cc63d13
to
fc351e8
Compare
| * | ||
| * @test | ||
| */ | ||
| public function compilation_should_not_fail_with_empty_container() |
There was a problem hiding this comment.
Not sure if this is a good idea. What are the symfony best practices for this?
There was a problem hiding this comment.
I only know that Symfony best practices does not tell you anything about this. There is not event a cookbook entry about how you test configuration, extension or your compiler passes.
There was a problem hiding this comment.
well, unit tests should be straightforward. and the functional test is already covered if we do anything where we use the factories.
so the problem here is that if we run this test we get an error, right? do we not install the puli things on travis, would this not work?
There was a problem hiding this comment.
It is not about installation. In this test, a container is built with only this compiler pass. And there is a burnt-in test which tests the thing against an empty container. If there is any httplug service which is not defined nor discovery is installed, the intended behaviour is to get an exception during compile.
There was a problem hiding this comment.
is discovery not installed on travis? if its optional, i would add a check and skip the tests if discovery is not available.
There was a problem hiding this comment.
Discovery is removed in this PR. Do you mean Puli?
Yes, Puli is installed, but this test only tests this specific compiler pass.
There was a problem hiding this comment.
sorry. jetlagged brain in action ;-)
right. so i think we should have one test that makes sure we get a decent error message when the puli bundle is not available and nothing is configured explicitly. and 2 more tests, one for explicit configuration and one for when puli is available.
There was a problem hiding this comment.
These cases are already tested.
sagikazarmark
force-pushed
the
httplug_factory
branch
2 times, most recently
from
5d81a53
to
f6fdd6c
Compare
|
Squashed and rebased, ready for merge IMO. Calling for a final review. |
|
Note to self: the URL in exception must be changed. |
| @@ -0,0 +1,107 @@ | |||
| <?php | |||
|
|
|||
| namespace Http\HttplugBundle\Util; | |||
There was a problem hiding this comment.
I do not like the Util namespace. It feels like the old days when you had a folder called functions. Maybe do something more descriptive like Discovery?
There was a problem hiding this comment.
Not sure if we even need that namespace
POC how to not be dependent on discovery Fix unit tests Fix factory method Remove duplicated service Move discovery to compiler pass Applied fixes from StyleCI Fix puli executable Use class constant Fix puli command order Fix lowest dependency issue Fix HHVM tests Improve tests
sagikazarmark
force-pushed
the
httplug_factory
branch
from
f6fdd6c
to
4463f6c
Compare
|
Awesome @sagikazarmark Im happy to merge when the tests passes. |
[WIP] Remove discovery dependency
Includes #54
Unfortunately I couldn't fix functional tests, although I added the symfony bundle to the test AppKernel.
Any ideas?