[FrameworkBundle] do not stumble upon empty tags #16932
Conversation
xabbuh
commented
Dec 9, 2015
| Q | A |
|---|---|
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #16908 |
| License | MIT |
| Doc PR |
| @@ -49,4 +50,15 @@ public function testProcess() | |||
|
|
|||
| $pass->process($container); | |||
| } | |||
|
|
|||
| public function testEmptyTagNameIsIgnored() | |||
There was a problem hiding this comment.
this test is missing as assertion which is not good practice
|
I don't agree with this implementation. Either we say, empty tags are ignored. Then it should be ignored in |
|
i simply extracted the empty tag by debugging inside |
be7f453 to
c6a7c45
Compare
|
I had a look at this again and I think the actual issue is that the |
| @@ -59,7 +59,7 @@ public function process(ContainerBuilder $container) | |||
|
|
|||
| foreach ($container->findUnusedTags() as $tag) { | |||
| // skip whitelisted tags | |||
| if (in_array($tag, $this->whitelist)) { | |||
| if (empty($tag) || in_array($tag, $this->whitelist)) { | |||
There was a problem hiding this comment.
this misses '0' as well. IMO an empty tag should still be reported as "unused". only the $candidates calculation should be skipped for empty tags.
|
I changed the implementation to skip only the check for similar tags when the current tag name is |
|
|
||
| if (false !== strpos($definedTag, $tag) || levenshtein($tag, $definedTag) <= strlen($tag) / 3) { | ||
| $candidates[] = $definedTag; | ||
| if (null !== $tag && '' !== $tag) { |
There was a problem hiding this comment.
findUnusedTags has @return string[]. So $tag can never be null.
|
If #16956 is merged I don't think this change here has an relevance in reality as the tag name cannot be empty anymore. |
|
Can we close this one then? |
|
I think so. I'll finish #16956 tonight and then we don't need this anymore. |