[FrameworkBundle] PropertyInfo support #15966
Conversation
dunglas
commented
Sep 28, 2015
| Q | A |
|---|---|
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | n/a |
| License | MIT |
| Doc PR | not yet |
| * | ||
| * @return array | ||
| */ | ||
| private function findAndSortTaggedServices($tagName, ContainerBuilder $container) |
There was a problem hiding this comment.
Any way to factorize this method as you use it in all compiler pass. Might be useful for others.
There was a problem hiding this comment.
It would be better to have it in a separate class but in the Serializer class the behavior is not exactly the same (an error is thrown if normalizer is added, this is not desirable here).
|
Issues raised by @stof fixed. |
|
ping @symfony/deciders |
| </service> | ||
|
|
||
| <!-- Extractor --> | ||
| <service id="property_info.reflection_extractor" public="false"> |
There was a problem hiding this comment.
missing property class
<service id="property_info.reflection_extractor" class="Symfony\Component\PropertyInfo\ReflectionExtractor" public="false" >|
Thank you for your review @aitboudad... I'll fix all those issues and test it in real conditions. Status: Needs Work |
|
Status: Needs Review |
| } | ||
| } | ||
|
|
||
| if (0 === count($sortedServices)) { |
There was a problem hiding this comment.
if (empty($sortedServices)) {
return array();
}
// or maybe even
if (!$sortedServices) {
return array();
}There was a problem hiding this comment.
Count is (IIRC) less efficient if it comes to just checking that array is not empty, it would use more memory to check (run thought whole array) just to be sure it contains something, while empty checks are much more simple.
There was a problem hiding this comment.
Thanks for the explanation.
22d5d95 to
790dd5a
Compare
|
Rebased. |
|
|
||
| $container = $this->getMock('Symfony\Component\DependencyInjection\ContainerBuilder', array('findTaggedServiceIds')); | ||
|
|
||
| $container->expects($this->atLeastOnce()) |
There was a problem hiding this comment.
I suggest we avoid checking number of calls with stubs and use $this->any() instead. It's not important how many times a stubbed method is called. It's important that if it's called, it returns the stubbed value. Relying on number of calls to stubbed methods makes refactorings harder. It ties tests to the implementation.
There's a similar case in the next test.
|
Thank you @dunglas. |